loop logic within a function call

Hi,

Basically, I'm just trying to set up a loop whereby one push-button is used to randomly select one of five LEDs, ten times. The program is to interrogate (loop round) after each press of the button and check when it has reached ten presses at which point it should drop out of the loop and reset back to the initial condition, namely the green ready LED is on and it's ready to go again.

I think this is quite simple but I'm not sure if I should be taking a different approach (completely) to this problem as I've fiddled around with different approaches including while loops and different orderings but still no joy. So any suggestions you may have as to what I am doing wrong or how to do it right would be greatly appreciated.

P.S. Attached is a screenshot of the code and error in question.

The error message is pretty self-explanatory.

You cannot use break in anything bar a switch or loop. You've attempted to use it within an if()?

Also, your for loop is going from x=0, while x < 10, but x is never incremented? So effectively it will cycle infinitely.

If you want to "break" out of a function, just do a "return;"

tammytam:
The error message is pretty self-explanatory.

You cannot use break in anything bar a switch or loop. You've attempted to use it within an if()?

Also, your for loop is going from x=0, while x < 10, but x is never incremented? So effectively it will cycle infinitely.

Hi,
According to the Arduino reference page on "Breaks" you can do this within an if statement.
Also, I have done the increment but it is within my function routine "beacon_select".

Thanks anyway.

It would be much easier if you posted your entire code rather than give us an image of it. You declare and initialise a local variable called x in your function, it has nothing to do with your loop counter. It is out of scope.

As for the break, you can use it within an if(), so long as that if resides within a loop.

ie this is valid:

for( int i=0; i<10; i++ )
{
  if( i == 8 )
    break;
}

According to the Arduino reference page on "Breaks" you can do this within an if statement.

But the "if" will always be inside a loop construct.

In your beacon_select() code you define a new int (int x = x+1;). The x in the if() statement will not be visible to the beacon_select() code.

I would re-write the code to use a for() loop with an exit condition and beacon_select to return a bool.

Using 'break' should be avoided as it is poor form.

e.g.:
for (int i=0; i<10 && !beacon_slected; i++){...}

Using 'break' should be avoided as it is poor form.

Shhh, don't tell the guys who wrote the C libraries, and besides, it is de rigueur for getting out of "case"s :wink:

According to the Arduino reference page on "Breaks" you can do this within an if statement.

When I looked it said

break is used to exit from a do, for, or while loop, bypassing the normal loop condition. It is also used to exit from a switch statement.

The break in the first example on the reference page if breaking from the for loop, not the if

I have done the increment but it is within my function routine "beacon_select".

I would be fascinated to see what you have done and how. Whatever it is it is not a for loop as I understand it. Please post your whole code rather than pictures of some of it.

AWOL:
Shhh, don't tell the guys who wrote the C libraries, and besides, it is de rigueur for getting out of "case"s :wink:

What do you mean? I've been using goto's to exit my cases for years :stuck_out_tongue:

Thanks for the info. The following code compiles but doesn't work. I know that is obvious as there's no returning of a boolean from the function for the loop to test. Please can you explain what you meant and if the for loop should be inside or outside of the function. It's been a long day :frowning:

const int buttonPin = 2;
const int readyLed  = 3; // Green LED means ready
const int ledPin1   = 4; // 1 to 5 five red LED beacons
const int ledPin2   = 5;
const int ledPin3   = 6;
const int ledPin4   = 7;
const int ledPin5   = 8;
const int ledBeacons [] = {4, 4, 5, 5, 6, 6, 7, 7, 8, 8};  //five LED pin designators replicated twice in the array
int buttonState = 0;

void setup() {
  
  Serial.begin (9600);
  randomSeed (analogRead(0));
  pinMode (readyLed, OUTPUT);
  pinMode (ledPin1, OUTPUT);
  pinMode (ledPin2, OUTPUT);
  pinMode (ledPin3, OUTPUT);
  pinMode (ledPin4, OUTPUT);
  pinMode (ledPin5, OUTPUT);
  pinMode (buttonPin, INPUT);
}

void loop() {
   //===== Check the state of the button and run the routine only if button is pressed =====   
   
   buttonState = digitalRead(buttonPin);
   if (buttonState == LOW) {
     digitalWrite(readyLed, HIGH);
   }
   
   else {   
       for (int x = 0; x < 10 &&!beacon_select; x++) {
       digitalWrite(readyLed, LOW);
       beacon_select();
       }
   } 
}

//===== my function to select randomly lit LED beacons =====

void beacon_select() {            
  int r = random (1,11);               
  int beacon = ledBeacons [r];    
  Serial.println (beacon);            //use serial window for diagnostic checks only
  digitalWrite (beacon, HIGH);     //display the activated LED for 0.5s on the prototyping board
  delay (500);
  digitalWrite (beacon, LOW);
  
  //return;                              //return back to the for loop at line 31 
}
int r = random (1,11);

Should be:

int r = random (0,10);

But that's not the real cause of your problem.

We could doctor this code and shoehorn it to fit, but honestly I think it needs a structure change.

Can I get some clarification on what is needed? You press the button, then the program lights up a random LED, you press it again, it lights up another while keeping the prior one lit. Continue in this fashion until all are lit and it breaks out and starts again?

Am I on the money? Or is that wrong?

       for (int x = 0; x < 10 &&!beacon_select; x++) {

You have used the name of the function without the brackets () after it. This is a pointer. Testing it like this, you are testing the existence of the function. In english, the loop continues only if x is less than ten and the beacon_select pointer is a null.

The function itself is declared as void, so you can't return a value from it. Perhaps you meant to declare it as boolean and then it can return true or false to let the controlling loop know something?

tammytam:

int r = random (1,11);

Should be:

int r = random (0,10);

But that's not the real cause of your problem.

We could doctor this code and shoehorn it to fit, but honestly I think it needs a structure change.

Can I get some clarification on what is needed? You press the button, then the program lights up a random LED, you press it again, it lights up another while keeping the prior one lit. Continue in this fashion until all are lit and it breaks out and starts again?

Am I on the money? Or is that wrong?

Thanks for the correction on the int r. I was wondering why when it did work it returned a "0" every now and again (a null element in the array perhaps...?). One problem down two to go.

I agree, I think my prog needs a rethink.

Can a function like the one I have created, namely "beacon_select" carry out a logical process and pass that information back into a loop outside of the function?

The idea of this project is best described as an analogy: You have a bag of ten counters, two sets each numbered 1 to 5 (ie; five LEDs). A 6th LED is lit but is only used to indicate that the system is ready.
A counter is drawn out randomly, one at a time (ie; on each button press) and it's corresponding LED lights for two seconds then goes out again. After all ten counters are out of the bag, the green LED lights and the process can start again.

Yes it is possible for a function to pass information back, that what the return is for, a return can return any type, a pointer to a whole array of information if you so wished.

Here is a modification of your code which might work the way you want. don't have any way of testing it at the moment, but it should be close:

const int buttonPin = 2;
const int readyLed  = 3; // Green LED means ready
const int ledPin1   = 4; // 1 to 5 five red LED beacons
const int ledPin2   = 5;
const int ledPin3   = 6;
const int ledPin4   = 7;
const int ledPin5   = 8;
const int ledBeacons [] = {4, 4, 5, 5, 6, 6, 7, 7, 8, 8};  //five LED pin designators replicated twice in the array
int buttonState = 0;
bool g_led_beacons_been_activated[10];
int g_button_press_counter = 0;

void setup() {
  
  Serial.begin (9600);
  randomSeed (analogRead(0));
  pinMode (readyLed, OUTPUT);
  pinMode (ledPin1, OUTPUT);
  pinMode (ledPin2, OUTPUT);
  pinMode (ledPin3, OUTPUT);
  pinMode (ledPin4, OUTPUT);
  pinMode (ledPin5, OUTPUT);
  pinMode (buttonPin, INPUT);

  memset( g_led_beacons_been_activated, 0, 10 );
}

void loop() {
   //===== Check the state of the button and run the routine only if button is pressed =====   
   
	buttonState = digitalRead(buttonPin);
	if (buttonState == LOW) 
	{
		digitalWrite(readyLed, HIGH);
	}
	else 
	{   
		if( g_button_press_counter == 10 )
		{
			// start again yay! ...
			memset( g_led_beacons_been_activated, 0, 10 );
		}

		digitalWrite(readyLed, LOW);
		beacon_select();
		g_button_press_counter++;
	} 
}

//===== my function to select randomly lit LED beacons =====

void beacon_select() 
{            
	int r = random (0,10);               
	while( g_led_beacons_been_activated[r] )
		r = random (0,10);
	int beacon = ledBeacons [r];    
	g_led_beacons_been_activated[r] = true;
	Serial.println (beacon);            //use serial window for diagnostic checks only
	digitalWrite (beacon, HIGH);     //display the activated LED for 0.5s on the prototyping board
	delay (500);
	digitalWrite (beacon, LOW);
}

Much appreciated Tammytam.

Although that code does compile, it does exactly what I had the circuit doing in an earlier version without all of that extra code. But I will go through it carefully, step by step and see what it is you are trying to achieve here.

Thank you for your time and diligence.

Sorry Walltree,

Just read through it, again can't test it, but I forgot an important line:

if( g_button_press_counter == 10 )
{
	// start again yay! ...
	memset( g_led_beacons_been_activated, 0, 10 );
	g_button_press_counter = 0;	// **** This line is paramount, add it please 
}

Try that and let us know if it does what you want, and if not, what it is doing.

Operations is the same as before.

  • Green LED lit and ready (all red LEDs out) - as it should be
  • Short press of the button (which sends a logic high to pin 2 on the Uno)
  • One of the five red LEDs lights at random for .5s then goes out (as it should) but the Green LED comes straight back on again. It should only come back on again after the tenth time.

--
!DOH! is a function of Homer

Didn't realise that was a requirement ;), try this:

const int buttonPin = 2;
const int readyLed  = 3; // Green LED means ready
const int ledPin1   = 4; // 1 to 5 five red LED beacons
const int ledPin2   = 5;
const int ledPin3   = 6;
const int ledPin4   = 7;
const int ledPin5   = 8;
const int ledBeacons [] = {4, 4, 5, 5, 6, 6, 7, 7, 8, 8};  //five LED pin designators replicated twice in the array
int buttonState = 0;
bool g_led_beacons_been_activated[10];
int g_button_press_counter = 0;

void setup() {
  
  Serial.begin (9600);
  randomSeed (analogRead(0));
  pinMode (readyLed, OUTPUT);
  pinMode (ledPin1, OUTPUT);
  pinMode (ledPin2, OUTPUT);
  pinMode (ledPin3, OUTPUT);
  pinMode (ledPin4, OUTPUT);
  pinMode (ledPin5, OUTPUT);
  pinMode (buttonPin, INPUT);

  memset( g_led_beacons_been_activated, 0, 10 );
}

void loop() {
   //===== Check the state of the button and run the routine only if button is pressed =====   
   
	if( g_button_press_counter == 0 )
	{
		digitalWrite(readyLed, HIGH);
	}
	else
	{
		digitalWrite(readyLed, LOW);
	}
 
	buttonState = digitalRead(buttonPin);

	if (buttonState == HIGH) 
	{   
		if( g_button_press_counter == 10 )
		{
			// start again yay! ...
			memset( g_led_beacons_been_activated, 0, 10 );
			g_button_press_counter = 0;	// This line is paramount, add it please 
		}

		beacon_select();
		g_button_press_counter++;
	} 
}

//===== my function to select randomly lit LED beacons =====

void beacon_select() 
{            
	int r = random (0,10);               
	while( g_led_beacons_been_activated[r] )
		r = random (0,10);
	int beacon = ledBeacons [r];    
	g_led_beacons_been_activated[r] = true;
	Serial.println (beacon);            //use serial window for diagnostic checks only
	digitalWrite (beacon, HIGH);     //display the activated LED for 0.5s on the prototyping board
	delay (500);
	digitalWrite (beacon, LOW);
}

It's almost there!!!

The green ready light does stay off during the process (as it should) but never comes back on again, even after the 10th execution (press).

Out of interest, what is memset and how does this work and please can you also explain the boolean that you initialised at the beginning and how that works.

If we get this nailed then you should be re-ranked straight to guru status.

:wink: