Should I remove blocking code when it is an "edit" section

Hi All

Been a long day here at lockdown however I have managed to put together a working LCD clock with digital buttons and editable time and date.

I am working on my Spa controller project to try and learn programming using state machine concepts and the clock is a small part of the project. I got the majority of the code from SM Ching’s DS1307 RTC LCD digital clock but modified it to work with my I2c LCD and 4 digital buttons.

Hardware
I2C LCD
DS1307RTC
Arduino UNO
4 digital buttons

One question I have is, I have been trying to do non-blocking code and have added in a “heartbeat” section. However in the code there is one blocking section that blocks while you adjust the time or date before exiting back to the loop. (note I have taken the program out of the loop so it is standalone and simply called from the loop as LCD_Clock_Function(); )

The blocking code is

while (1) { // loop forever
if (isTimeout()) { //exit loop if timeout occurred
return -1;
}

Should I try and remove this blocking code using something like flags or does having the odd piece of blocking code not matter in your opinion as it has a time out function.

Attached is the code, sorry it is a bit messy, tomorrows job is to try and clean it up, add some tags, and make it a bit more efficient to suit the overall project. It wouldn’t let me inline post it as it was over 9000 characters. Not sure if there is a trick around that

As in all of my questions here I’m looking for some guidance rather than a fixit service.

The blocking code is on line 300. Get rid of it or accept it?

LCD_Clock_Display.ino (15.3 KB)

Should I try and remove this blocking code using something like flags or does having the odd piece of blocking code not matter in your opinion as it has a time out function.

In my opinion you should never have blocking code, with the possible exception of something in startup that only blocks for a second or two while something initialises itself.

I get the feeling that you already know you ought to get rid of the blocking code and you just want someone to agree with you :)

Once you get into the habit of writing non-blocking code it becomes easy to do everywhere.

++Karma; // For the progress you've made and for trying get into goo practices.

As in all of my questions here I'm looking for some guidance rather than a fixit service.

If that is all, there is no need to look at the actual code. The answer is simply : "Do you want your processor to do anything else while you are waiting for user input ? and the answer is probably not. The clock is ticking in the background anyway (the RTC does) the LCD is being updated (i assume) to make the user input visible. What else is there.. If you want to do this mainly as an exercise then of course you should get rid of it. If you enter the user-input section through some button press, then you should expand on that, rather than switching to a section of code where the program is being blocked. That way you can more easily add parts to your code if you want to. For instance if you want to add an alarm, the alarm can go off during user input as well.

What else is there.. If you want to do this mainly as an exercise then of course you should get rid of it.

Once you get into the habit of writing non-blocking code it becomes easy to do everywhere.

Thanks for the replies, you are probably right that in this particular instance of adjusting the time, which would be done very infrequently, blocking is acceptable, the processor wouldn't need to do anything else.

However as you have both pointed out, getting rid of it is better practice.

As a beginner you see a lot of examples out their with "while" statements but then see well written code (larryd helped me recently) who say avoid "while" whenever possible.

Thanks, tomorrows project, remove blocking code and carry on.

Cheers Al

Avoid "while" whenever possible.

I would say use 'while' with care. The only place I can think of at the moment* where I consider 'while' to be acceptable is:

  while (Serial.available() > 0) {
// Save serial data somewhere
  }

My thinking is that serial data comes in slowly compared to the speed the processor can deal with it, so the processor will not spend much time emptying the serial receive buffer. However, the serial buffer must be emptied reasonably promptly otherwise it will overflow, which is why other code must not be blocking when you use serial because you need to attend to any incoming data promptly.

*I stress 'think of at the moment', I am not suggesting there are not other valid uses of 'while', just that right now this is the only example I can think of.

*I stress 'think of at the moment', I am not suggesting there are not other valid uses of 'while',

Understood ! atm. for me the most obvious use is when testing for more than one condition (although it can of course be done in a for loop as well. just seems less obvious) Also really as a loop there is no real difference between for and while in this way

uint8_t i=10;
while (i>0) {
(...)
i--;
}

It just spreads the function out over a few lines, but has the same functionality (and compiles to the same i'd say) And yes any condition which tests the result of another function call (as Serial.available() is a function call) Or... and lest we forget, if the processor should be parked until a condition is met, when the result depends on an interrupt function. Particularly when other parts of the code disable interrupts. In fact i use 'while() quite a lot to loop, in parsing functions, or even loops that could be done with 'for' Using "while(1) ; on the other hand is the same as for(;;) ; That just creates an endless loop.

Using "while(1) ; on the other hand is the same as for(;;) ; That just creates an endless loop.

Yes, I use those in 'normal' (non Arduino) C for the main loop, as, probably, do most people. (I find smiley faces don't work too well in C or C++ though...)

Consider this: if your spa is heating up while you're in it and the program enters your blocking code, what is going to happen?

And one little comment: get rid of String (capital S) objects; they will can eventually bite you in the butt with unexpected behaviour at run time.

(I find smiley faces don't work too well in C or C++ though...)

sorry i didn't put any space, but the IDE doesn't convert ; ) into a smiley

Just put [nobbc] and[/nobbc] around it ;)

The compiler will not like it, though :D

Ok almost 12 hours of trying to make it work and I failed :o . Some of that was researching how to substitute string for String() for a beginner that is not as simple as it sounds but I see the concept of
char *char strcpy strcat etc and also why String() can cause issues in regards to stack and heap etc, it will take me a awhile to work that all out.

However my problem is I couldn’t get the rid of the blocking code. I changed the program into a sort of state machine and added flags but I couldn’t get it to work. Looking for some help as to where I have gone wrong.

It seems to be this part of the code but I can’t see it. Full code attached. Now when i select left or right buttons for date or time edit it goes into some form error as the “proofOfLife” led goes to a 2 second interval which is probably the event_timeout.

//////////////////////////////////////////////////////////////////////////
// read input from button switches
//////////////////////////////////////////////////////////////////////////
int get_button_input(int val, byte pos_x, int min, int max) {

if (currentMillis < previousMillis + event_timeout) {   // time out  currentMillis from Loop, previusMillis from button_input

  blink_text(pos_x, int2str(val));
  byte lcd_key = key_press();   // read the buttons

  switch (lcd_key) {
    case btnUP:
      val++;
      if (val > max) val = min;
      break;
    case btnDOWN:
      val--;
      if (val < min) val = max;
      break;
    case btnRIGHT:
      lcd.setCursor(pos_x, SECOND_LINE);
      lcd.print(int2str(val));
      return val;
      break;
  }

  if (lcd_key != btnNONE) { //if a button is pressed
    previousMillis = millis(); // reset timeout so that the timeout will not occur
  }

}
}

LCD_Clock_Display.ino (17.3 KB)

Hello Allan, I've had a quick read through your code but the only thing that stands out is how lovely and neat it is! I wish all people new to programming wrote such neat code.

I'm not the best at reading other people's code and I have to go out shortly, I hope someone else will see what I cannot see.

Ok almost 12 hours of trying to make it work and I failed.

In my experience that will make the pleasure of fixing it all the better. In any case, after 12 hours the best thing you can do to find the problem is walk away from it for a while.

Thanks Perry,

I’ve walked away for tonight visiting my friend Sauvignon Blanc.

Will tackle again tomorrow and maybe it becomes clearer.

Cheers Al

I don't have your libraries so I can't compile your sketch, but I would expect the compiler to be warning you about that function. If it isn't, I suggest that you adjust the warning level in preferences.

The function is supposed to return an int, but it only explicitly does so if you press the right button. If not, whatever happens to be on the stack will be used, probably with unexpected results.

Always return something at the end. Maybe val, maybe -1 if nothing was pressed. I think you're assuming that until you press the rightbtn, nothing will be returned. That is not true.

It also means that you need to revisit your logic. if you adjust val in the function, that adjustment is lost when the function returns. I think you need to get input for hours repeatedly, capturing the new value as it's adjusted and move on after the rightbtn is pressed.

Thanks WildbIll

I'm sure you are right about the flaw in the logic. Being a self taught newbie there are probably some basics that I have missed. Will go back and relook at it step by step. maybe add in some serial.print to see whats going on

When it compiles there are no errors but I have also made so many changes I really need to go 10 steps and start from the basics as it no functions.

I'll see if I can get it going today

cheers Al

Allan_Pritchard: When it compiles there are no errors

Indeed, but there may be warnings that you're not seeing. The IDE defaults to something less than show me all of them to try and defend you from a flood of messages from the compiler that will be incomprehensible on day one. As you get the hang of things though, those warnings can be really helpful.

Found Them :slight_smile: I’ll work on these

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino: In function 'byte key_press()':

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:252:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

   if (millis() != previous_time)

       ~~~~~~~~~^~~~~~~~~~~~~~~~

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino: In function 'boolean check_time(byte, byte, byte)':

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:554:30: warning: comparison is always false due to limited range of data type [-Wtype-limits]

   if ((hours > 23) || (hours < 0)) return false;

                        ~~~~~~^~~

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:555:34: warning: comparison is always false due to limited range of data type [-Wtype-limits]

   if ((minutes > 59) || (minutes < 0)) return false;

                          ~~~~~~~~^~~

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:556:34: warning: comparison is always false due to limited range of data type [-Wtype-limits]

   if ((seconds > 59) || (seconds < 0)) return false;

                          ~~~~~~~~^~~

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino: In function 'int get_button_input(int, byte, int, int)':

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:376:1: warning: control reaches end of non-void function [-Wreturn-type]

 }

 ^

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino: In function 'byte read_LCD_buttons()':

C:\Users\pritcharda\Documents\Arduino\LCD_Clock_Display\LCD_Clock_Display.ino:522:1: warning: control reaches end of non-void function [-Wreturn-type]

 }

 ^

Ok i think I have worked out the issue. I don’t expect you to wade through the code so just tell me if conceptually I’m on the right track, if possible. Code is attached

Using the example of setting the date from the sketch

Because I enter the mState “EditDate” it then calls the set_date() function in a continuous loop, because the code is not blocked, which is a good thing.

The set_date calls the function get_button_input() once again over and over because its looping in mState EditDate. This was where the original while loop blocked.

I think the issue is that my state machine needs another state for “InputValue” so while it is in that state it is just dealing with the values of YY:MM:DD then returns that as

this_date = “D” + int2str(years % 100) + int2str(months) + int2str(days); // which is a String() but I haven’t dealt with that issue yet to covert to strings

it can do this as non blocking as it is just dealing with the value passed. I think I need to go to state editdate → pass Year to InputValue State → back to EditDate → pass month to InputValue etc etc

does that sound right ?

State machine below

void stateMachine()
{
  switch (mState)
  {
    case Startup:
      {
        Serial.println("Startup    State");

          if (currentMillis - powerUpMillis > powerUpDelay) {
          LCD_Clock_Function();           // should check and return mstate running or startup if rtc failed
          
        }
      }
      break;

    case Running:
      {
      Serial.println("Running  State");  
      RTC.read(tm);
      show_time(FIRST_LINE);        // print time to LCD
      show_date(SECOND_LINE);       //Print date to LCD
      }
      break;

    case EditTime:
      {
      Serial.println("EditTime State");
      setTimeFlag = true;  
  //    lcd.clear();
      lcd.print("Set Time");  
      set_time();
      }
      break;

    case EditDate:
      {
      Serial.println("EditDate State");  
      setDateFlag = true;
 //     lcd.clear();
      lcd.print("Set Date");  
      set_date();
      }
      break;

    case InputValue:
    {
      // need to add the get_button_input() here to return val to case UpDate
    }
      
    case UpDate:
    {
    Serial.println("Update    State");    
       save_settings(this_date);
    }
     break; 
  }

}// end state machine

This is where the code falls over as the original code enters a While Loop at get_button_input() however I am cycling back through now.

void set_date() {
//                      11111
//           0123456789012345
  lcd.setCursor(0,0);
  lcd.print("Set Date        ");
  show_date(SECOND_LINE);          // Display the date, 
  
  int years = -1;                  // set ints toprogress 
  int months = -1;
  int days = -1;


// need to change to mState InputValue 

    //get_button_input(initial value, x_position, min value, max value)

    years = get_button_input(tmYearToCalendar(tm.Year), 12, 0, 2099); //year ranged from 0 to 2099

    if (years != -1) {
      months = get_button_input(tm.Month, 9, 1, 12); //month ranged from 1 to 12
    }

    if (months != -1) {
      byte lastday = get_lastday(years, months); //last day of a month
      days = get_button_input(tm.Day, 6, 1, lastday);
    }

    if (days != -1) {
      this_date = "D" + int2str(years % 100) + int2str(months) + int2str(days);
      setDateFlag = true;
      mState = UpDate;
    }
    
}

This was the code that had the while loop which allowed the user to adjust “val” before the btnRIGHT returned it to the set_date() which then asked for the next value.

//////////////////////////////////////////////////////////////////////////
// read input from button switches
//////////////////////////////////////////////////////////////////////////
int get_button_input(int val, byte pos_x, int min, int max) {

if (currentMillis < previousMillis + event_timeout) {   // time out  currentMillis from Loop, previusMillis from button_input

Serial.println(previousMillis);
Serial.println(currentMillis);

 byte lcd_key = key_press();   // read the buttons

  switch (lcd_key) {
    case btnUP:
      val++;
      if (val > max) val = min;
      break;
    case btnDOWN:
      val--;
      if (val < min) val = max;
      break;
    case btnRIGHT:
      lcd.setCursor(pos_x, SECOND_LINE);
      lcd.print(int2str(val));
      return val;
      break;
  }
//}

  blink_text(pos_x, int2str(val));

  if (lcd_key != btnNONE) { //if a button is pressed
    previousMillis = millis(); // reset timeout so that the timeout will not occur
  }
}
return -1;

}

original while loop for reference

//////////////////////////////////////////////////////////////////////////
// read input from button switches
//////////////////////////////////////////////////////////////////////////
int get_button_input(int val, byte pos_x, int min, int max) {
  previousMillis = millis(); // reset timeout so that the timeout will not occur
  while (1) { // loop forver
    if (isTimeout()) { //exit loop if timeout occurred
      return -1;
    }

    byte lcd_key = key_press();   // read the buttons
    switch (lcd_key) {
      case btnUP:
        val++;
        if (val > max) val = min;
        break;
      case btnDOWN:
        val--;
        if (val < min) val = max;
        break;
      case btnRIGHT:
        lcd.setCursor(pos_x, SECOND_LINE);
        lcd.print(int2str(val));
        return val;
        break;
    }
    blink_text(pos_x, int2str(val));

    if (lcd_key != btnNONE) { //if a button is pressed
      previousMillis = millis(); // reset timeout so that the timeout will not occur
    }
  }
}

LCD_Clock_Display.ino (19.5 KB)

I'm not sure you need another state, EditDate seems like it should be enough. You just need a global somewhere to accumulate the digits that make up the date until you press the rightbtn. And that global gets nulled out as you transition into EditDate state.

For your purposes too, I don't think it much matters if the edit date code does block - the alarm and time display functions are rather pointless if you're changing the time or date. No harm doing it though and good practice for a case when it is important.

Thanks Bill

Late here; but take on board your comments. Will tackle again tomorrow

100% agree that in my particular project blocking would be fine while you update the clock but its kind of a mission to solve during lockdown. I'm new to to programming so its all about up-skilling.

Cheers Al