Important changes to forums and questions
All forums and questions are now archived. To start a new conversation or read the latest updates go to forums.mbed.com.
7 years, 4 months ago.
getting 4 LEDs to blink 5 times in succession.
I am trying to get 4 external LEDs to blink 5 times in succession. I am having trouble with a counter. LED1 blinks 5 times then moves to LED2 and blinks 5 times and so on through LED4. I have a counter in the code but as you can tell it doesn't do anything. Thank you for your help!!
- include "mbed.h"
DigitalOut led1(p5); define digital output for pin 5 DigitalOut led2(p6); define digital output for pin 6 DigitalOut led3(p7); define digital output for pin 7 DigitalOut led4(p8); define digital output for pin 8
int main() { int counter = 0; while(1){ for(int i=0; i<4; i++) switch (i){ case 1:if (i<10) {led1 =!led1;wait(0.2); i++;} if (counter >5); for(int j=0; j<4; j++) switch (j) { case 2: if (j<10) {led2 =!led2;wait(0.2); j++;} if (counter >5); for(int k=0; k<4; k++) switch (k) { case 3: if (k<10) {led3 =!led3;wait(0.2); k++;} if (counter >5); for(int g=0; g<4; g++) switch (g) { case 4: if (g<10) {led4 =!led4;wait(0.2); g++;} if (counter >5);
} } } } }
}
2 Answers
7 years, 4 months ago.
Please use <<code>> and <</code>> tags around your posted code to keep it readable.
This is pretty convoluted.. Several issues here:
- every case in a switch should end with a 'break'
- if (counter >5); doesnt do anything. The test is followed by a semicolon so it is an empty test. Also, best to always use {} around both branches of a test.
- you are incrementing i in the for-loop and again as part of the if (i<10) statement. That wont work, - same for other vars j,k and g.
Try something like this instead:
#include "mbed.h"
DigitalOut led1(p5); //define digital output for pin 5
DigitalOut led2(p6); //define digital output for pin 6
DigitalOut led3(p7); //define digital output for pin 7
DigitalOut led4(p8); //define digital output for pin 8
int main() {
int counter = 0;
while(1) {
for (int led=1; led<5; led++) // loop through all 4 leds
switch (led) { // select the desired led
case 1: for (counter=0; counter<10; counter++) { //toggle 10 times, means blink on 5 times
led1 =!led1;
wait(0.2);
}
led1=0; //make sure led is always off
break;
case 2: for (counter=0; counter<10; counter++) {
led2 =!led2;
wait(0.2);
}
led2=0;
break;
case 3: for (counter=0; counter<10; counter++) {
led3 =!led3;
wait(0.2);
}
led3=0;
break;
case 4: for (counter=0; counter<10; counter++) {
led4 =!led4;
wait(0.2);
}
led4=0;
break;
default: // oops, should never get here...
led1=1;
led2=1;
led3=1;
led4=1;
break;
} // switch led
} // for all leds
} //while (1)
} // main
Obviously there are other ways to do this more efficiently..
7 years, 4 months ago.
The key points were made by Wim - the DigitalIn and DigitalOut declarations must be global, if you want the results to last beyond the life of the procedure, and there is a BusOut- thanks Wim. Moving BusOut from a suggestion to an example is a good move too, thanks Andy. If you check your original comment, Andy, you will find the words "absurd claim" there in black and white, so you might put that together with your last response. It was there and in quotes because you used the term, even if you think you did not. Not exactly what promotes friendly communication, so you might bear that in mind in future. When you lads post code, it stays looking reasonable instead of being blank-compressed, or whatever the process is that mashes it all together. Presumably that is a response to the use of <<code>> and <</code>>. Yes, comment flags at the start of numerous lines of my example were deleted by the posting process - some improvement to that process would be good. If internal declarations implicit in the DIgitalIn and DigitalOut statements produced static declarations, instead of "automatic" storage, then presumably the statements could be internal to a procedure. When the direction of a specific pin needs to be changed by a field user of a system, I would prefer not to have to issue new firmware, so there should be some more or less obvious way to do that. I would localize the I/O references to a module like LEDRGB, so I would make the global variable names required by the current implementation reflect that, with LEDRGB_led1, etc. For small examples, it makes no difference, but management of code in large systems that survive any length of time should be disciplined, and give clarity. I don't think there is any value in my original posted code remaining here - what you two have done certainly lends clarity to the process for readers such as I who are relatively new to the site. This edit deletes my example code, since your comments have resolved the issue, and illustrated improvement with the busout option as well. Thanks again.
Dan.Mackie@DrSCADA.com
I/O pins are clustered in groups of 16 or 32 per port and can be accessed in one operation for that cluster. You can also set or reset them individualy. The DigitalOut is the mbed implementation to set one pin at a time. It can not set multiple pins at the same time so you will see delay between two pins when setting them to high sequentially. Obviously modifying one pin should not change any other pins on the same port or on any other port. I dont think it does, but please share an example so it can be investigated and fixed if necessary. There is also a BusOut method to set multiple pins at the same time. I only used the reset for every pin at the end of the loop to make sure the led is off no matter what end value you use for the counter.
posted by 07 Dec 2016Dan, I'm not sure what testing you've done but I think you need to go and do it again because you are wrong. Each IO pin can be set independently using the standard libraries. Been there, done that, produced systems that wouldn't work if it wasn't the case.
If you wish to claim the libraries are fundamentally broken in a way that would make them unusable then please point out the lines of code in the library that are incorrect. You seem to claim that there is a fundamental issue in the libraries but somehow in several years no one else has ever noticed it before. At the very least provide some evidence beyond a comment in the code that is part of the answer to a specific question to backup your absurd claim.
I'm guessing that comment is there to cope with the fact that the initial state isn't being defined and as a fail safe in case the number of changes is ever changed to an odd number. The code goes on to turn all the LEDs on if it gets into an invalid state.
posted by 07 Dec 2016Again, please use <<code>> and <</code>> tags around your posted code to keep it readable.
I think this is your code:
#include "mbed.h"
DigitalOut aled(LED1);
DigitalIn mybutton(USER_BUTTON);
DigitalOut bled(LED2);
int maina() {
int isbut;
while(1) {
isbut = mybutton;
if (isbut == 0) {
aled = 0;
bled = 1;
};
if (isbut == 1) {
aled = 1;
bled = 0;
}
wait(0.2);// 200 ms
}
}
void LEDRGB(char led, int onoff) {
//=== Status LEDs
DigitalOut led1(LED1); //PB_0
DigitalOut led2(LED2); //PB_7
DigitalOut led3(LED3); //PB_14
if (led == 'G') {
led1 = onoff;
};
if (led == 'B') {
led2 = onoff;
};
if (led == 'R') {
led3 = onoff;
};
}
DigitalIn mybutton(USER_BUTTON);
int main () {
int button, oldbutton, count=0;
LEDRGB ('R', 1);
LEDRGB ('G', 1);
LEDRGB ('B', 1);
wait (2.0);
while (1) {
if (count > 7) count = 0;
oldbutton = button;
button = mybutton;
if (button != oldbutton) {
count = count + 1;
}
if (count == 0) {
LEDRGB ('R', 0);
LEDRGB ('B', 0);
LEDRGB ('G', 0);
};
if (count == 1) {
LEDRGB ('R', 0);
LEDRGB ('B', 0);
LEDRGB ('G', 1);
};
if (count == 2) {
LEDRGB ('R', 0);
LEDRGB ('B', 1);
LEDRGB ('G', 0);
};
if (count == 3) {
LEDRGB ('R', 0);
LEDRGB ('B', 1);
LEDRGB ('G', 1);
};
if (count == 4) {
LEDRGB ('R', 1);
LEDRGB ('B', 0);
LEDRGB ('G', 0);
};
if (count == 5) {
LEDRGB ('R', 1);
LEDRGB ('B', 0);
LEDRGB ('G', 1);
};
if (count == 6) {
LEDRGB ('R', 1);
LEDRGB ('B', 1);
LEDRGB ('G', 0);
};
if (count == 7) {
LEDRGB ('R', 1);
LEDRGB ('B', 1);
LEDRGB ('G', 1);
};
}
}
The main issue here is that you declare the status LEDs inside the LEDRGB function. What happens is that your main calls LEDRGB, the DigitalOuts are declared and the pins are set. The code then exits LEDRGB and destroys the DigitalOut objects since they dont live outside the scope of the function! The pins will return to their default state (DigitalIn) and the LEDs will probably turn off or still float at the previous value for a short while. Your code continues creating and destroying the DigitalOuts at very high speed since main calls LEDRGB again and again even if nothing changes. Setting the LEDs should be done only when the count has changed.
NB There could also be an issue with the previously declared DigitalOut aled and bled which are overloaded onto the same pins. Perhaps that code is actually commented out in your original but this is no longer visible since the comment was lost in the posted code.
Try this code instead:
#include "mbed.h"
//=== Status LEDs now in global scope
DigitalOut led1(LED1); //PB_0
DigitalOut led2(LED2); //PB_7
DigitalOut led3(LED3); //PB_14
DigitalIn mybutton(USER_BUTTON);
void LEDRGB(char led, int onoff) {
//function only modifies the LED value, does not touch the DigitalOut declaration
if (led == 'G') {
led1 = onoff;
};
if (led == 'B') {
led2 = onoff;
};
if (led == 'R') {
led3 = onoff;
};
}
int main () {
int button, oldbutton, count=0;
LEDRGB ('R', 1);
LEDRGB ('G', 1);
LEDRGB ('B', 1);
wait (2.0);
while (1) {
if (count > 7) count = 0;
oldbutton = button;
button = mybutton;
if (button != oldbutton) {
count = count + 1;
}
if (count == 0) {
LEDRGB ('R', 0);
LEDRGB ('B', 0);
LEDRGB ('G', 0);
};
if (count == 1) {
LEDRGB ('R', 0);
LEDRGB ('B', 0);
LEDRGB ('G', 1);
};
if (count == 2) {
LEDRGB ('R', 0);
LEDRGB ('B', 1);
LEDRGB ('G', 0);
};
if (count == 3) {
LEDRGB ('R', 0);
LEDRGB ('B', 1);
LEDRGB ('G', 1);
};
if (count == 4) {
LEDRGB ('R', 1);
LEDRGB ('B', 0);
LEDRGB ('G', 0);
};
if (count == 5) {
LEDRGB ('R', 1);
LEDRGB ('B', 0);
LEDRGB ('G', 1);
};
if (count == 6) {
LEDRGB ('R', 1);
LEDRGB ('B', 1);
LEDRGB ('G', 0);
};
if (count == 7) {
LEDRGB ('R', 1);
LEDRGB ('B', 1);
LEDRGB ('G', 1);
};
}
}
It's " "absurd" " (not sure why you put quotes around a word I didn't use) to state that you were wrong when you make sweeping claims which I know to be false without any evidence to support them? Clearly you own a different dictionary to the one I use.
It looks like Wim has fixed all the bugs in your code. The style is indeed a little odd, there is no need for a ; after a } but that's not going to break anything. And personally I would have used a switch rather than a series of if (count == ...) statements. But again that's not going to stop anything working.
You could also use a BusOut rather than 3 DigitalOuts. In that situation your code becomes:
#include "mbed.h"
BusOut leds(LED1,LED2,LED3);
DigitalIn mybutton(USER_BUTTON);
int main () {
int button = 0
int oldbutton = 0;
int count=0;
leds = 7;
wait (2.0);
leds = 0;
while (1) {
if (count > 7) count = 0;
oldbutton = button;
button = mybutton;
if (button != oldbutton) {
count = count + 1;
leds = count;
}
}