Non Blocking Code Issue: Virtual LED Fireplace Project

Hi
My Arduino Uno project involves making a virtual fireplace using LED strips; yellow and red and incorporates end illuminated plastic formed ‘flames’. The virtual ‘fire’ goes through four phases in sequence successfully and I have tried to implement a finite state approach using a Case statement. A handset can be used to put the’fire’ into standby. I have used a bitwise PORTD approach to drive the LED banks - it is effective. Fire Phases 1 to 3 use a 100mSec delay between each PORTD write using the delay() function. Since I wanted control over the individual LED flicker effects during each cycle I have used a binary array and PROGMEM to store the data elements. The use of ‘Count_up’ and ‘Count down’ to iterate through an array block effectively reduces the number of data elements needed.

My problem is I now want to refine the solution to use non blocking code. I have attempted this in ‘FIRE_PHASE_4’ in the ‘for loop’ which reads the array byte and writes it to PORTD. Problem is, despite my use of millis() as shown, the for loop executes at machine speed! I can see why in retrospect but I cannot despite hours of trying various approaches using millis() resolved it.

So, is it possible to incorporate a delay using non-blocking code into a ‘For-Loop’ as shown in the context of my project in this way, or do I need an alternative approach? I want to understand/solve this problem before I embark on my next project involving real time sensors.

Of necessity I have had to remove much of the program data and the finite_state code for Phases 1,2 and 3 and handset Case statements to get below the post’s limit of 9000 characters.

Many thanks in anticipation of your help.

#include <SoftwareSerial.h>
#include <boarddefs.h> //IRRemote includes
#include <IRremote.h>
#include <IRremoteInt.h>

//IR Receiver
int RECV_PIN = 11;  //Input signal from PIR IC D11
IRrecv irrecv(RECV_PIN);
decode_results results;

//State machine variables
#define TEST_MODE       0
#define MACHINE_STANDBY 1
#define FIRE_PHASE_1    2
#define FIRE_PHASE_2    3
#define FIRE_PHASE_3    4
#define FIRE_PHASE_4    5
#define RESET           6

#define COUNT_UP        1
#define COUNT_DOWN      2
#define X4_LOW          250
#define X4_HIGH         319

uint8_t finite_state; //State variable
uint8_t count_state;  //Count up or down status

const long int FIRE_PHASE4_DURATION =  30000;
const long int FLAME_FLICKER_DELAY   = 100; // 100 mSec

unsigned long previousMillis;
unsigned long currentMillis = millis();//Square wave 50% duty cycle - Must be within function
unsigned long previousMillis_1;
unsigned long currentMillis_1 = millis();//Square wave 50% duty cycle - Must be within function
unsigned long previousMillis_2;
unsigned long currentMillis_2 = millis();//Square wave 50% duty cycle - Must be within function

int A0_REDLED = 9;  //PWM Pins: 3,5,6,9,10 and 11
int A1_REDLED = 10;
byte data;
int x = X4_LOW;

//+=========================================================================================

const byte BitMap_test[320] PROGMEM  = {   // store in program memory to save RAM
 
  //Yellow Flickering Fire Starting
  //FIRE_PHASE_1
  B10100100,                        //[13] Array Element
  B01001000, B00110100, B10101000, B01010000, B10001100, B10010000, B00100100, // [20]
  B10001000, B01000000, B00101000, B01000000, B10110000, B00000100, B10011000,
  B01000000, B00101100, B11000000, // [30]
  B00100000, B00110000, B00000100, B00010000, B00010100, B00101000, B00010000,
  B00100000, B00110000, B00001000, // [40]
  B00010000, B10001000, B00010000, B10000000, B00010000, B01001000, B00000100,
  B01001000, B00100000, B00001000, // [50]
  B10100000, B00000000, B10000000, B01000100, B00101000, B01010100, B00101000,
  B00011000, B00000000, B00000100, //[60]
  B00100100, B00001000, B00100100, B00000100, B00100100, B01100000, B00000000,
  B11000000, B00000000, B11001000, //[70]
  B00000100, B10100100, B00010000, B10101000, B00010000, B00100000, B00010000,
  B00100000, B00000000, B00000100, //[80]
  B00001000, B10000000, B00010000, B01000000, B00001000, B01000100, B10101000,
  B01010100, B00100000, B01010100, //[90]
  B00100100, B00000100, B00001000, B00000000, B10000000, B10000000, B00000000,
  B00110100, B00000000, B00100100, //[100] Array Element

//Also separate data for Fire phases 2,3 and 4
//Fire_PHASE_4 range would be elements 250 to 319.


//Setup ------------------------------------------------------------------------------------------
void setup() {


  irrecv.enableIRIn(); // Start the receiver
  Serial.begin(9600);
  pinMode(A0, OUTPUT);
  pinMode(A1, OUTPUT);
  analogWrite(A0_REDLED, 0);
  analogWrite(A1_REDLED, 0);

  //PORT D - Pins 0 to 7 (Tx and Rx Serial Mon. Port status must NOT be modified)
  DDRD = B11111100; //See above configuration table
  previousMillis = 0;
  previousMillis_1 = 0;
  previousMillis_2 = 0;

  finite_state = MACHINE_STANDBY;
  count_state =  COUNT_UP;
}//End of Setup+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

//LOOP +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
void loop() {
  currentMillis   = millis(); //Fire Phase duration timer
  currentMillis_1 = millis(); //Flicker timer: Count up
  currentMillis_2 = millis(); //Flicker timer: Count down

  int y = 0;
  readIRDecoder(); //Monitor IR command decoder

  switch (finite_state)
  {
    case MACHINE_STANDBY;

    case FIRE_PHASE_1;

    case FIRE_PHASE_2;
	

    case FIRE_PHASE_4:
      analogWrite(A0_REDLED, 200);
      analogWrite(A1_REDLED, 200);
      if ((currentMillis - previousMillis) >= FIRE_PHASE4_DURATION)
      {
        previousMillis = currentMillis;
        finite_state = MACHINE_STANDBY;
        break;
      } else {
        //Handset Key: '4'
        Serial.println("FIRE_PHASE_4");

        //Serial.print(data, DEC) + Serial.print("  ") + Serial.print(data, HEX) + Serial.print("  ")+ Serial.println(x);
        //for (int j = 1; j < 5; j++) {
        switch (count_state)
        {
          case COUNT_UP:
            if ((currentMillis_1 - previousMillis_1) >= FLAME_FLICKER_DELAY)
            {
              previousMillis_1 = currentMillis_1;
              for (x = 250; x < 319; x++)
              {
                Serial.println("Count Up");
                data = pgm_read_byte (&BitMap_test[x]); // fetch data from program memory
                PORTD = data;
                Serial.print(data, DEC) + Serial.print("  ") + Serial.print(data, HEX) + Serial.print("  ") + Serial.println(x);
                Serial.print("Count-Up") + Serial.println(x);
                x++;
              }// End FLicker Delay

              count_state = COUNT_DOWN;
              break;

            case COUNT_DOWN:
              if ((currentMillis_2 - previousMillis_2) >= FLAME_FLICKER_DELAY)
              {
                previousMillis_2 = currentMillis_2;
                for (x = 319; x > 250; x--)
                {
                  Serial.println("Count Down");
                  data = pgm_read_byte (&BitMap_test[x]); // fetch data from program memory
                  //delay(200);
                  PORTD = data;
                  Serial.print(data, DEC) + Serial.print("  ") + Serial.print(data, HEX) + Serial.print("  ") + Serial.println(x);
                  Serial.print("Count-Down") + Serial.println(x);
                }
                x--;
              }//

              count_state = COUNT_UP;
              break;

            } //End Count Case

            finite_state = MACHINE_STANDBY;
            break;
        }
      }//End switch


    default:
      break;

  }//End of Switch

}  //END LOOP

is it possible to incorporate a delay using non-blocking code into a 'For-Loop' as shown in the context of my project in this way, or do I need an alternative approach?

Mixing for loops and millis() timing is not a good idea because the code is "locked" in the for loop until it is complete (yes, I know that there are ways out of it)

What you need to do is to let the loop() function do what it does best, ie looping. Stay in state 4 and use millis() timing for the flame flicker. Emulate a for loop by incrementing or decrementing a variable and testing explicitly for when it reaches its limits either way.

  previousMillis = 0;
  previousMillis_1 = 0;
  previousMillis_2 = 0;

Variable names are there to help you, not to confuse you :wink: Give them a more usefull name or at least number hem al :wink: Or better, use an array if[ you really need 3 variables.

I mean

 currentMillis   = millis(); //Fire Phase duration timer
  currentMillis_1 = millis(); //Flicker timer: Count up
  currentMillis_2 = millis(); //Flicker timer: Count down

Are they really going to be different :wink:

And a tip, you can initialize them when you create them, no need to do that in setup().

To the problem, you’re millis is in the wrong place.
Now you loop over all data every FLAME_FLICKER_DELAY. You want to only ececute 1 step of that loop every FLAME_FLICKER_DELAY :wink:

Some more general advise:

  1. Functions! Split the code into easy to understand and small bits. That’s a lot easier to debug

  2. You ONLY set the direction (input or output) of Port D. Don’t you want to drive them as output? Aka, use PORTD instead? But don’t forget to set them as output with DDRD first :wink:

  3. Like I said, variable names are there to help you. Standard in Arduino is toCamelCaseVariables, UpperCamelCaseConsts and ALL_CAP_FOR_MACROS. It’s up to you what style you pick but try to stick to it. Now you mix and match

  4. To add to that, use more descriptive names. Names like finite_state, x, data and A0_REDLED don’t say anything useful. Use for example flameState and RedLedPin.

  5. Even more, you dn’t need to make every variable a global :wink: For example data could be local.

  6. Why do you mi macros (#define) and const? const variables are the saver C++ alternative to things like

#define X4_LOW          250

And for states a nice, save and eas to understand way is to define a enum

enum fireState_t {S_TEST_MODE, S_MACHINE_STANDBY, S_FIRE_PHASE_1, S_FIRE_PHASE_2, S_FIRE_PHASE_3, S_FIRE_PHASE_4, S_RESET};

I did prefix them as a precaution they already exists. Especially with very generic names like RESET there is a big change it is already defined.

  1. What board do you use? It used to be pretty useless to define PROMEM without including #include <avr/pgmspace.h> on AVR baed boards but this might changed.

  2. Instead of putting 1 large array into PROGMEM you could split it. Or at least make the start position of each phase a constant you can use. Now with things like

for (x = 319; x > 250; x--)

the numbers just seem random and it’s hard to maintain the code if the sequence gets longer ot shorter.

  1. If you still want to use the other pins of Port D you can mask it. (At leas, it seems you don’t use pin 0 and pin 1 of Port D.) Do
//instead of
PORTD= data; 
//do 
PORTD= (data & 0b11111100) | (DDRD & ~0b11111100);
//or
PORTD|= data & 0b11111100;
PORTD&= data | ~0b11111100;

Although it might be even easier to not port manipulate it but just use digitalWrite(). You can still use a "bitmap’ in PROGMEM.

data = readBitmap(); //aka, just be sure to fill data with the new bitmap
for(byte i = 0; i < ; i++){
 digitalWrite(LedPin[i], data & 0x01);
 data >>= 1;
}

Thank you both for your valued replies.
UKHelibob - I have just adopted your approach and it now works well. Thank you so much!

Septillion: - Wow, much to ponder through here. Implimenting these changes will take a while longer but see the points you make and will adopt them as I progress my Arduino programming.
I am using an Arduino Nano but have not found it necessary to include the <avr/pgmspace.h> include file - solution works fine.
LSB bits 0 and 1 on the PORTD are not used because they correspond to the TX/Rx Serial monitor modem comm lines and I use for diagnostics. As it happens, on this project, I really only need 6 output ONLY pins on PORTD, but note your point about masking options.

You say ‘millis()’ is in the wrong place in my program. Can you advise where the statement should be properly located please?
Also, can you indicate with example how I could split the large array into say 3 parts using PROGMEM; say, fireStartPhase, fireMiddlePhase and fireEndPhase.
Working on your suggestions now.

Thanks for your time in responding. I appreciate your helpful advice.

In pseudo code you now have:

if(millisPassed()){
  for(byte i = 0; i < allCases; i++){
    preformCase(i);
  }
}

But that does all cases once millis true. What you should do is:

if(millisPassed()){
  preformCase(nextCase);
  nextCase++; // to remember where you where for the next time millis has passed
}

And about PROGMEM, you can just put 3 different arrays into progmem.

const byte BitmapStartPhase[100] =  {data};
const byte BitmapMiddlePhase[100] =  {data};
const byte BitmapEndPhase[100] =  {data};

But I think in a single array can be useful (because you can reuse code to run it) but store the start position of each phase as a variable so it’s easy (and in a single location) to change.

The demo Several Things at a Time illustrates the use of millis() to manage timing without blocking. It may help with understanding the technique.

...R