Strategy for flashing LEDs

Projects Programming
1

I'm looking for some feedback on my proposed strategy for flashing LEDs on an R4 Uno Minima.
And this is beyond the basic examples using delay and millis.
The essential point is that the application code will be doing other things which may, or may not, affect how the flashing happens.

A simple scenario. I'm using the built in CAN bus controller to connect into an existing automation network which can send out a command instructing my connected node (or other nodes) to control an LED:

  • on ,
  • off ,
  • flash at around 1 Hz 50% duty cycle,
  • flash faster at around 2Hz, or
  • flash very fast at around 4Hz.

Inside my loop I'm monitoring button presses, reading DS1820 style temp sensors, doing some floating point maths (potentially slow even with the Minima's FPU) etc and then sending messages back onto the CAN bus. So all of these are non trivial operations and can affect LED timing.

I have written a C++ class to encapsulate the node.

My approach is to put a public method to update the elapsed time, and a public class member to read the state of the LED. Psuedo code:

class CanbusNode
{
public:
  bool feedbackLED = false;
  void updateLEDTime(unsigned long ticks);

....

I'd then call this function as the last statement in my loop procedure and set the physical LED based on the boolean class member

  node CanbusNode;
  // initialisation etc elsewhere

void loop() {
  // do all the other time consuming stuff
  node.updateLEDTime(millis());
  digitalWrite(node->feedbackLED, HIGH);
}

Inside of the updateLEDTime function I will compare the incoming ticks parameter to a class level variable and if sufficient time has elapsed and the mode is one of the flashing types, then toggle the boolean value from true to false etc.

Note that the mode is set by a different incoming Canbus message (off,on,flash slow, flash fast, flash very fast) .

Does this sound like a sensible strategy?
Are there any suggestions for improvements ?

2

Totally.

Here I wonder

digitalWrite(node->feedbackLED, HIGH);

if you didn't mean to not always write HIGH to some LED.

FWIW, those are snippets or something, not pseudocode.

You say

// do all the other time consuming stuff

Obvsly if this consumes too much time, you won't get what you want from the idea. Dunno what you are needing to do every loop, usually things are written so there really is no time consuming thing done in a way that disrupts the ability to loop at a good clip.

a7

1 Like
3

I don't write in C++ but I do use OOP principles and some of the C++ stuff that has leaked into regular C.

You might make the status LED thing an object or class or whatever all its own, such a thing might find use all over the place. I have an indicator thing that when poked flashes briefly, turns itself off. So you can invoke a stab of light and not need to worry. I use them for showing program activity when the serial monitor is not available or being used otherwise.

Also, recently on these fora was a protracted discussion of how to do the update thing to all objects that need it, Imma look now and link it here.

a7

1 Like
4

Sorry, that was a typo. Thanks for catching.
It should read

digitalWrite(LEDPinNumber, node->feedbackLED);

And yes I'll do some testing to ensure the time consuming stuff does not over whelm the user interaction aspects.
E.g. if reading the temp takes more than one LED flash cycle then I'd need to see how that could be split across two runs through loop, and only do it every 10 seconds or so.

Yes, that sounds like a great idea. I can imagine it will be used in more than one node type.

FWW, my background is 30+ years writing line of business Windows software so I have good skills in architecture, analysis, decomposition into objects / code etc.
But at this stage I'm not considering anything like RTOS - should that be on my list or does it bring more problems with it given the chip is a single core in any case.

5

I work at the very low end, and I like to write as much of what makes something work as I can. For the Arduino boards I use, UNO, Mega, Nano and so forth, and what I do with them, using even the lightest weight RTOS seems like overkill.

With internet stuff and some of the more capable boards (great time to be alive!) I can see that a point would come where learning a well-respected RTOS and using it would make sense.

Opinions will vary. :expressionless:

a7

6

Hello fastbike

Consider:

//https://forum.arduino.cc/t/strategy-for-flashing-leds/1260933
//https://europe1.discourse-cdn.com/arduino/original/4X/7/e/0/7e0ee1e51f1df32e30893550c85f0dd33244fb0e.jpeg
#define ProjectName "Strategy for flashing LEDs"
#define NotesOnRelease "Arduino MEGA tested"

// make names
enum TimerEvent {NotExpired, Expired};
enum TimerControl {Halt, Run};
enum OutPutAction {Off, On, Flash1, Flash2, Flash4};

// make variables
uint32_t currentMillis = millis();
constexpr uint8_t LedPin {9};
constexpr uint32_t FlashTimes[]  {0, 0, 1000, 500, 250};

// make structures
//-----------------------------------------
struct TIMER
{
  uint32_t interval;
  uint8_t control;
  uint32_t now;
  uint8_t expired(uint32_t currentMillis)
  {
    uint8_t timerEvent = currentMillis - now >= interval and control;
    if (timerEvent == Expired) now = currentMillis;
    return timerEvent;
  }
};
struct FLASHME
{
  uint8_t pin;
  TIMER   blinkMe;
  void make()
  {
    pinMode (pin, OUTPUT);
  }
  void action(uint8_t doAction)
  {
    switch (doAction)
    {
      case Off ... On:
        digitalWrite(pin, doAction);
        blinkMe.control = Halt;
        break;

      case Flash1 ... Flash4:
        blinkMe.control = Run;
        blinkMe.now = currentMillis;
        blinkMe.interval = FlashTimes[doAction];
        digitalWrite(pin, digitalRead(pin) ? LOW : HIGH);
        break;
    }
  }
  void run()
  {
    if (blinkMe.expired(currentMillis) == Expired)
    {
      digitalWrite(pin, digitalRead(pin) ? LOW : HIGH);
    }
  }
};
FLASHME flashLed{LedPin,0,Halt,0};
//-----------------------------------------
// make support
void heartBeat(const uint8_t LedPin, uint32_t currentMillis)
{
  static bool setUp = false;
  if (setUp == false) pinMode (LedPin, OUTPUT), setUp = true;
  digitalWrite(LedPin, (currentMillis / 500) % 2);
}
// make application
void setup()
{
  Serial.begin(115200);
  for (uint8_t n = 0; n < 32; n++) Serial.println("");
  Serial.print("Source: "), Serial.println(__FILE__);
  Serial.print(ProjectName), Serial.print(" - "), Serial.println(NotesOnRelease);
  flashLed.make();
  delay(2000);
  Serial.println(" =-> and off we go\n");
}
void loop()
{
  currentMillis = millis();
  heartBeat(LED_BUILTIN, currentMillis);
  keyBoard();
  flashLed.run();
}
//------
void keyBoard()
{
  static bool setUp = false;
  if (setUp == false)
  {
    setUp = true;
    Serial.println(F(" attenzione: following commands are available"));
    Serial.println(F(" [r]= reset "));
    Serial.println(F(" [1]= led off "));
    Serial.println(F(" [2]= led on "));
    Serial.println(F(" [3]= led blink 1 Hz"));
    Serial.println(F(" [4]= led blink 2 Hz"));
    Serial.println(F(" [5]= led blink 4 Hz"));
    Serial.println("");
    delay(1000);
  }
  if (Serial.available())
  {
    switch (Serial.read())
    {
      case 'r':
        asm volatile (" jmp 0");
        break;
      case '1':
        flashLed.action(Off);
        break;
      case '2':
        flashLed.action(On);
        break;
      case '3':
        flashLed.action(Flash1);
        break;
      case '4':
        flashLed.action(Flash2);
        break;
      case '5':
        flashLed.action(Flash4);
        break;
    }
  }
}

Have a nice day and enjoy coding in C++.

1 Like
7

Thanks @paulpaulson
That looks interesting. Before I read your suggestion I had created a solution, which works, but probably reflects my coding background in other high level languages, but translated to CPP. BTW this is on an R4 Uno which I'm using for prototyping before moving over to a custom board with the same Renases RA4M1 32 bit processor.
Anyway here's my code

/* Velbus Widgets */

#ifndef VelbusWidgets_H
#define VelbusWidgets_H
#include "VelbusConst.h"

/* feedback LED display */
typedef enum vmb_feedbackLEDStatus
{
  flmOff            = VMB_COMMAND_CLEAR_LED, //0xF5
  flmOn             = VMB_COMMAND_SET_LED, //0xF6
  flmSlowBlink      = VMB_COMMAND_SLOW_BLINK_LED, // 0xF7
  flmFastBlink      = VMB_COMMAND_FAST_BLINK_LED, // 0xF8
  flmVeryFastBlink  = VMB_COMMAND_VERY_FAST_BLINK_LED // 0xF9
};

namespace velbus
{
/* group of feedback LEDs */
class Velbus_LEDGroup
{
public:
  Velbus_LEDGroup(uint8_t size = 4);  
  ~Velbus_LEDGroup();    

  /* call this from the loop function */
  void processLEDs(uint32_t const ticks);
  
  /* call this to decide if feedback LED is ON at any given moment */
  bool getFeedbackLED(uint8_t index);
  /* update the LED status, first argument is 01, 02, 04, 08 etc, second is the Velbus message id "VMB_COMMAND_CLEAR_LED"  etc*/
  void updateLEDStatus(uint8_t bitValue ,uint8_t Status);

private:
  uint8_t m_size;
  uint32_t m_feedbackLEDClock[3] = {0, 0, 0};  
  bool * m_feedbackLED;    
  vmb_feedbackLEDStatus * m_feedbackLEDStatus;
  uint32_t m_lastCheckedLEDTime = 0;
};
}
#endif //VelbusWidgets_H

and the implementation of the class

#include "Arduino.h"
#include "VelbusWidgets.h"

namespace velbus
{
Velbus_LEDGroup::Velbus_LEDGroup(uint8_t size) 
 : m_size{ size }
{
  m_feedbackLED = new bool[m_size];
  m_feedbackLEDStatus = new vmb_feedbackLEDStatus[m_size];
}

Velbus_LEDGroup::~Velbus_LEDGroup() 
{
  delete [] m_feedbackLED;
  delete [] m_feedbackLEDStatus;
}

bool Velbus_LEDGroup::getFeedbackLED(uint8_t index)
{ 
  return m_feedbackLED[index];
}

void Velbus_LEDGroup::updateLEDStatus(uint8_t bitValue ,uint8_t Status)
{
  for (int i = 0; i < sizeof(m_feedbackLEDStatus); i++)
  { 
    if ((bitValue >> i) == 1)
    { 
      m_feedbackLEDStatus[i] = static_cast<vmb_feedbackLEDStatus>(Status);
    }
  }
}

void Velbus_LEDGroup::processLEDs(uint32_t const ticks)
{ 
  /* only updates every 50m sec */
  if ((ticks - m_lastCheckedLEDTime) < 50)
  {  
    return;
  }
  m_lastCheckedLEDTime = ticks;
  
  /**** set the change flags */
  /* slow blink is 1 sec on, 1 sec off */
  bool changeSlow = ((ticks - m_feedbackLEDClock[0]) > 1000);
  if (changeSlow){
    m_feedbackLEDClock[0] = ticks;
    }
  /* fast blink is 500mSec on 500mSec off */
  bool changeFast = ((ticks - m_feedbackLEDClock[1]) > 500);
  if (changeFast){
    m_feedbackLEDClock[1] = ticks;
    }
  /* very fast blink is 250mSec on, 250mSec off */
  bool changeVeryFast = ((ticks - m_feedbackLEDClock[2]) > 250);
  if (changeVeryFast) {
    m_feedbackLEDClock[2] =ticks;
  } 
  
  /**** set the LED registers */
  for (int i = 0; i < sizeof(m_feedbackLEDStatus); i++)
  { 
    switch (m_feedbackLEDStatus[i])
    { 
      case VMB_COMMAND_CLEAR_LED: 
        m_feedbackLED[i] = false;
        break;
      case VMB_COMMAND_SET_LED: 
        m_feedbackLED[i] = true;
        break;
      case VMB_COMMAND_SLOW_BLINK_LED:  
        if (changeSlow){
          m_feedbackLED[i] = ! m_feedbackLED[i];
          }
        break;
      case VMB_COMMAND_FAST_BLINK_LED:  
        if (changeFast){
          m_feedbackLED[i] = ! m_feedbackLED[i];
          }
        break;
      case VMB_COMMAND_VERY_FAST_BLINK_LED: 
        if (changeVeryFast){
          m_feedbackLED[i] = ! m_feedbackLED[i];
          }
        break;
    }  // end switch
  } // end for
}
} /* end namespace */

I then use an instance of this class in other classes that implement some of the functionality in my network of nodes (home automation light switches etc).
I'll take another look at your code as it seems to be lighter weight which may be important as I add additional functionality.

8

I'm sure I don't understand why you use a bit value instead of an index.

void Velbus_LEDGroup::updateLEDStatus(uint8_t bitValue ,uint8_t Status)

You provide for seeing if a LED is on or off right now by index. I would expect to fix the behaviour of a given LED by the same index.

@paulpaulson's code is lighter weight or whatever; someone working with very limited resources could probably come in somewhat lower, at the expense of what I am not sure.

I think developing the changeSlow and similar booleans could be very simplified or even skipped entirely by using a counter and doing some work with the low order bits.

We can't try your code. I don't know if there is more missing than VelbusConst.h. Certainly if you were to publish this a complete example that we could run ourselves would be nice.

I am not likely to need that, though. I am more a "Our Friend the Beaver" than a "Juxtabranchial Organ Secretions in the Higher Mollusks" kinda coder. :expressionless:

a7

2 Likes
9

The code in this helper class is called by a message loop that is pulling messages off a CAN bus.

The high level protocol that is being used has the first byte as the "command" LED off, on, slow blink, fast blink, very fast blink hence my unusual vales for the enum vmb_feedbackLEDStatus

The second byte of the payload sets the index of the LED as a bit wise value so that accounts for the unusual signature of the function.

I missed out quite a bit (several thousand LoC) :slight_smile:

The constants that will help to make more sense are

#define VMB_COMMAND_CLEAR_LED                 0xF5
#define VMB_COMMAND_SET_LED                   0xF6
#define VMB_COMMAND_SLOW_BLINK_LED            0xF7
#define VMB_COMMAND_FAST_BLINK_LED            0xF8
#define VMB_COMMAND_VERY_FAST_BLINK_LED       0xF9

I will follow up with this suggestion. It seems overly cumbersome as written, even if it works.

10

I've refactored my state checking code to use the struct as suggested by @paulpaulson and refactored the logic as suggested by @alto777

   enum class vmb_flashSpeed: int
  {
    fsSlow, fsFast, fsVeryFast
  };
  struct /* timer for slow, fast, very fast flash */
  {
    uint16_t interval;
    bool invert = false;
    uint32_t elapsed = 0;
    void expired(uint32_t ticks)
    { 
      invert = ((ticks - elapsed) > interval);
      if (invert) elapsed = ticks;
    }
    void update(bool * feedbackLED)
    {
      if (invert) *feedbackLED = ! *feedbackLED;
    }
  } m_flashTimers[3] = {{1000}, {500}, {250}};

I'm not sure how to size the array of struct based on the enum for the flashing speed, so I hard coded it to 3.

The implementation is now much simpler, I can index into the timer array using the enum, althogh the cast is ugly - I'm a Pascal programmer by choice and would have been able to size the array with the enum and index with no casts required.

void Velbus_LEDGroup::setFeedbackLEDClock(uint32_t const ticks)
{ 
  /* only updates every 50m sec, handles overflow of timer */
  if ((ticks - m_lastCheckedLEDTime) < 50)
    return;

  m_lastCheckedLEDTime = ticks;
  
  /**** set the change flags */
  for (int i = 0; i < 3; i++) 
    m_flashTimers[i].expired(ticks);
  
  /**** set the LED registers */
  for (int i = 0; i < sizeof(m_feedbackLEDStatus); i++)
  { 
    switch (m_feedbackLEDStatus[i])
    { 
      case VMB_COMMAND_CLEAR_LED: 
        m_feedbackLED[i] = false;
        break;
      case VMB_COMMAND_SET_LED: 
        m_feedbackLED[i] = true;
        break;
      case VMB_COMMAND_SLOW_BLINK_LED:  
        m_flashTimers[(int)vmb_flashSpeed::fsSlow].update(&m_feedbackLED[i]);
        break;
      case VMB_COMMAND_FAST_BLINK_LED:  
        m_flashTimers[(int)vmb_flashSpeed::fsFast].update(&m_feedbackLED[i]);
        break;
      case VMB_COMMAND_VERY_FAST_BLINK_LED: 
        m_flashTimers[(int)vmb_flashSpeed::fsVeryFast].update(&m_feedbackLED[i]);
        break;
    }  // end switch
  } // end for
}

Many thanks for the help

1 Like
11

I'm here more for the "should I use a while void, or an if loop" ppl, but this is something I have thought about, so I have taken a moment before y'all know what to write a demo of the beginnings of my idea for a LED object that is light weight.

This currently is not using any features of C//C++ that I would not expect the better noobs to have mastered. It does use things learned over some time of squeezing things that shouldn't fit into sketches (and programs, haha) for microprocessors.

A mask controls the duty cycle. By setting multiple bits we can also do blinking. In the object this may never become, winking could also be done the same way.

Try it here:


Wokwi_badge UA Status LED Experiment


// https://wokwi.com/projects/398309878002672641
// https://forum.arduino.cc/t/strategy-for-flashing-leds/1260933

// preliminary test for status LED object

const byte leds[] = {2, 3, 4, 5,};
const byte type[] = {0b100, 0b1000, 0b11000, 0b111000,};

const byte nLEDs = sizeof leds / sizeof *leds;

void setup() {
  Serial.begin(115200);
  Serial.println("|nbit flash world!");

  for (int ii = 0; ii < nLEDs; ii++) pinMode(leds[ii], OUTPUT);
}

# define RATE 64

void loop() {
  static unsigned long lastTime;

  unsigned long now = millis();

  if (now - lastTime < RATE) return;
  lastTime = now;

  static byte flashCounter;
  flashCounter++;

  for (int ii = 0; ii < nLEDs; ii++) {
    bool thisLED = (flashCounter & type[ii]) == type[ii];
    digitalWrite(leds[ii], thisLED ? HIGH : LOW);
  }
}

HTH

a7

12

You've explained where they come from. In the class you wrote, the values don't matter.

Yes. But both these design choices bind the flash LED class to your larger program, reducing the general applicability.

This is why it is hard to write a simple sketch that shows how to use the class.

Look at any library that does button handling. An example is simple to write. The class knows, and need not know, anything about the context in which it will be used, making it useful in whatever circumstances its functionality is what you need.

It is a divide and conquer idea. Here we see thread titles like "how do you control a servo with a potentiometer" and the advice is to figure out each, and it comes down to sending one number from the thing that figured out the pot to the other thing which makes the servo go. The number could go anywhere, or come from anywhere. Testing each part is straight ahead. One inquirer actually insisted that these were not entirely separable problems.

I could challenge myself to write another method that just does what I want, let me talk about N things by referring to them by index 0 .. N - 1, a C idea that is found in many languages.

a7

13

I am writing a library to allow custom made modules to interact with an existing protocol.
Sure I could abstract more of the generic LED flashing functionality into another library, but I would then have to write a wrapper around that for my current (CAN bus protocol) library.

One of the other requirements for my end applications is to detect push button presses ( typical use case in parenthesis):

  • single rapid press/release (toggle state of light on/off)
  • long press, followed by a release after some elapsed threshold (trigger some other action, maybe start a timer etc)
  • sequence of N clicks within time period T (trigger an alarm when 4 clicks within a second)
  • detecting that button is being held down (activate dimming mode for a light circuit)

I settled on the EasyButton library which gave me most of what I wanted. However I needed to fork it to add a callback that is fired when the button is held down. I also added a pointer to the originating button in the callbacks (so an array of buttons can be controlled) and added an id data field to the button object so the index can be more easily obtained. All good stuff and reasonably generic.

All good fun and lots to learn.

14

For this requirement:

millis() will surely do.

What is time-consuming is if your DS1820 style temp sensors,

whatever "style" in this conetxt means if these sensors reallly are DS18B20 sensors

Sending a request and waiting (in a blocking manner) for the answer of the onewire-sensors will take aproximately 750 milliseconds at 12bit resolution.

These onewire-requests knock-out everything else that shall be done fast and with short latency.
There are techniques to do the DS18B20-requests non-blocking.

15

Yes, I vaguely recall working with these about a decade ago. So will need to brush up on the asynchronous way to get a reading out of them.

16

It seems like you have worked out a solution, but I went through a similar thought process about LED flashing a few years ago and came out in a different place. I wrote a library that lets you instantiate an LED object (or many such objects) with flexible timing to indicate the status of something. For example, on one LED you might want to only give one quick flash every ten seconds to show all is well, saving power. If a state change occurs, the same LED could flash, say, 3 longer flashes every 5 seconds. It can handle as many states and sequences as you like on one or more LEDs. It can even handle whether you source or sink to turn your LED on, allowing for flexible wiring without needing to rewrite your code. After including the library and instantiating the object/s, you call begin() in setup(), and then just repeatedly call Update() in loop() while in the appropriate state.

I've no idea if it simplifies things for you, but anyone else reading this thread may also find it useful.

https://dthirteen.com/status-flash/

It was my first ever library, so please be kind. :slight_smile:

1 Like
17

This could be handy, thanks for sharing.
A future requirement will be to wink an LED every 10 seconds or so, during the night, to indicate the location of the switch panel in a darkened room. So I'm interested in seeing how you solved the problem.

18

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.