LED flashing sequence

Question

I have a volatile unsigned char array LedState[9] variable which is shared between threads. Each index in the array denotes a state. According to each state, the LED will flash in different sequences. One thread sets the state in the array and another thread based on the array index will flash the LEDs. Each state maintains a set of arrays ontimer and offtimer.

unsigned long TimersForBlueLedOn[] = {100,200,500,1,0,0,100,200,500};

unsigned long TimersForBlueLedOff[] = {100,200,500,0,0,0,100,200,500};

In the main thread, I traverse the each state in the array and check if the state is turned on or not. If state is on, I flash the LED for the timer values corresponding to the state.

For eg: state 2 has to be ON for 500ms and OFF for ms. We continue to be in state 2 till state 3 is set. State 3 has ON timer as 1 and no OFF timer, which implies LED should be ON all the time.

State 3 is the base state, that is any state after state 3, should blink according to the timer and should go back to state 3.
For eg, Led blue is turned on after state 3, when state 6 is set, the leds should blink for 100ms ON and 100ms OFF. The leds should blink till state 6 is turned off and go back to state 3. So basically it is a priority based. If state 7 is ON as well, after finishing state 6, it should blink state 7 till state 7 is OFF and should go back to state 3.

My problem is the blink looks like flicker as the state 3 is always set. I need to have stateless transition. I am unable to turn off state 3 based on the next state.

    void TurnOnLed(ModemState state) {
    LEDState[state] = 1;
}

void TurnOffLed(ModemState state) {
    LEDState[state] = 0;
}

unsigned char CheckLedState(unsigned char state) {
    return LEDState[state];
}
void GetLedStateVar(LEDStateVar *pLS) {
    unsigned char state = pLS->State;
    pLS->LongflashCode = INVALID_VAL;
    switch(state) {
          case ModemTurnOn:
              pLS->LED = Blue;
              pLS->OnTimer  = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
          case ModemInit:
              pLS->LED = Blue;
              pLS->OnTimer  = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
          case GSMConnected:
              pLS->LED = Blue;
              pLS->OnTimer  = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
          case GPRSOn:
              pLS->LED = Blue;
              pLS->OnTimer  = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
         case ServerNotConnected:
              pLS->LED = Green;
              pLS->OnTimer = TimersForGreenLedOn[state];
              pLS->OffTimer = TimersForGreenLedOff[state];
              break;
         case SwUpdateDownload:
              pLS->LED = Blue;
              pLS->OnTimer = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
         case SwUpdateRestart:
              pLS->LED = Blue;
              pLS->OnTimer = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
         case SwUpdateNewVersion:
              pLS->LED = Blue;
              pLS->OnTimer = TimersForBlueLedOn[state];
              pLS->OffTimer = TimersForBlueLedOff[state];
              break;
}

void FlashBlueLed(LEDStateVar *pLSV) {

    if(pLSV->OnTimer == 1) {
              SetLEDBlue(1);          
      }else  {

          if(GetElapsedTime(&BlueFlashTimer) >  pLSV->OnTimer * MILLI_SECONDS) {
             if(!GetLEDBlue()) {
                 SetLEDBlue(1);
                 StartTimer(&BlueFlashTimer);
             }
          }

          if(GetElapsedTime(&BlueFlashTimer) > pLSV->OffTimer * MILLI_SECONDS) {
            if(GetLEDBlue()) {
                SetLEDBlue(0);
                StartTimer(&BlueFlashTimer);
             }
          }
    }
}   

 for(unsigned char i=0; i< FLASHSTATES; ++i) {
      LF.State = i;
      GetLedStateVar(&LF);

      //Flashcode not complete but the state has been reset
      if(i == LastBlueState || i == LastGreenState) {
          if(LF.LED == Blue) {  // BLUE LED
              FlashBlueLed(&LF);

          }else if(LF.LED == Green) {
              FlashGreenLed(&LF);
          }
      } else if(CheckLedState(i) && LF.OnTimer) {

          if(LF.LED == Blue) {  // BLUE LED
              if(LastBlueState == INVALID_VAL) {
                  FlashBlueLed(&LF);
              }
          } else if(LF.LED == Green) {  // GREEN LED
              if(LastGreenState == INVALID_VAL) {
                  FlashGreenLed(&LF);
              }
          } else if(LF.LED == Both) { //BOTH GREEN AND BLUE LED
              FlashBothLeds(&LF);
          }
      }
    }

This smells like "flaghetti": a tonne of boolean flags setting various states here and there. If so, you need to rewrite it into a proper state machine using enums and function pointers. — Lundin, Nov 30 '18 at 14:04
However, it's not really possible to determine the source of the bug based on what's posted here. — Lundin, Nov 30 '18 at 14:04
I expect a state machine to look more like: (1) what state are we currently in? (2) Based on our current state, should we change state? (3) What are the side effects of the desired state change, if any? — Tim Randall, Nov 30 '18 at 14:06
Can you show implementations of `GetLedStateVar` and all of the `Flash_x_Led` functions? I am wondering if your code is ever setting a value in your volatile array. _[it should not](https://stackoverflow.com/a/31457558/645128)_, but if it is, because they are used to control states, would cause indeterminate control of your LEDs. — ryyker, Nov 30 '18 at 14:07
_Each index in the array denotes a state._ The `volatile` array has room for only 5 states, but you mention up to 7 states in your description. — ryyker, Nov 30 '18 at 14:12
I have added more code now. Unfortunately I cannot use a state machine as the there might more states added or removed as it goes along. So it needs to be a stateteless transition from one state to another state. — GreenCoder, Nov 30 '18 at 14:23
The more I look at this, the less sense it makes. Why does `TurnOnLed` take a `state` as a parameter, and not something identifying an LED? Please post a [mcve] that defines the enumerations and constants used here. — Tim Randall, Nov 30 '18 at 14:30
Tim - TurnOnLed(state) is used to set a state. It is a multithreading program. This state could be set by any thread. The main thread checks for the state which has been set and gets the timer values for that state and flashes accordingly. — GreenCoder, Nov 30 '18 at 14:39

ryyker · Answer 1 · 2018-12-03T13:31:34.850

At this point there is still missing information for a complete analysis, but for now, these improvements can be suggested.

1) By assigning a value of 0 to the LEDState variable, you are writing to a volatile:

void TurnOffLed(ModemState state) {
    LEDState[state] = 0;
}

You should not do that. These should be read only. And anyway there is nothing in your described approach indicating volatile is needed. (Unless LEDState[i] is being accessed by your hardware, or some other process outside of your application.)
The fact that this variable array needs to be updated by various threads within your program does not necessitate that it be made volatile. The fact that your array has a dedicated element for each thread is sufficient, and as long as you strictly limit access to each element to its respective thread, updating the states using this method will work. But this approach does not support using volatile.

More about volatile HERE

2) You state that Each index in the array denotes a state. The volatile array has room for only 5 states, but you mention up to 7 states in your description. (Later, in comments you say there are 9 states.) Define clearly how many states there are, then change the array to support all of your states: (note, array is not created as volatile, not does it need to be.)

#define MAX_STATES 9
unsigned char array LedState[MAX_STATES];

3) consider using a switch() statement for your state machine.

... 
switch(i)  {
    case LastBlueState:
    case LastGreenState:        
        // do something
        break;
    case <some other state>
        // do something
        break;
        ...

TurnOnLed(state) is used to set a state. It is a multithreading program. This state could be set by any thread. Hence have declared it as volatile. The main thread checks for the state which has been set in the array LedState and gets the timer values for that state and flashes accordingly. For the number states, I use the macro and have defined in my code. Just to make it look simpler I had put in numbers and in process made a typo, instead of typing 9 i had given 5 as the number of states. Sorry for that. — GreenCoder, Nov 30 '18 at 14:41
@GreenCoder- typical use for the `volatile` type is to declare a variable that can be written to by a process that your code does not have access to, then read by your code to get its value. Mostly useful in embedded code, it is often it is used to indicate the state of some kind of hardware, i.e. count of revolutions of an encoder, or the value of a digital output, etc. It is not typical (nor recommended) that `volatile` types be updated by your code. Only read. — ryyker, Nov 30 '18 at 16:05

LED flashing sequence

1 Answers1