Assistance with code involving millis and a potentiometer

I’m looking for assistance with a sketch I’m running on an Arduino Micro. It uses a series of 8 leds hooked up to a shift register and a potentiometer. The goal of the project is to light the leds in a particular sequence with differing on/off durations that are controlled by the potentiometer. The desired result is when the potentiometer is adjusted in one direction, the speed of the sequence of lights will speed up and the duration that each leds is on will shorten. When the pot is slid in the opposite direction, it will have the opposite effect. My current sketch produces the desired effect but only prior to adjustments made on the pot. It behaves correctly at any starting value from the pot but only at initialization. When the pot is adjusted in either direction, the behavior of the leds becomes erratic. They lose their sequence and random leds will stay lit constantly. Any advice on how to restructure the code that will allow for proper updating through the pot would be greatly appreciated. If you need additional information or clarification, please let me know. Thanks in advance.

#include <ShiftRegister74HC595.h>
ShiftRegister74HC595<5> sr(A2, A3, 4); 

const int POT = A0;
const byte leds [8] = {18, 22, 17, 16, 20, 19, 23, 21};
unsigned long currentMillis;
unsigned long previousMillis[8] = {0, 0, 0, 0, 0, 0, 0, 0};
byte ledState[8] = {1, 1, 1, 1, 1, 1, 1, 1};
const int POT_LOW = 0 ;
const int POT_HIGH = 1023;
const int SLOW = 1000;
const int FAST = 50;

void setup() {
  pinMode(POT, INPUT);
  Serial.begin(9600);
}

void loop() {
  currentMillis = millis();
  int rate = readPot();
  Serial.println(rate);
  led(rate);
}

void led(int parameter) {
  unsigned long durationOn = 0;
  unsigned long durationOff = 0;
  unsigned long interval = 0;
  durationOn = ((parameter / 10UL) * 2UL);
  durationOff = ((parameter * 4UL) - (durationOn));
  interval = (parameter / 2UL);

  for (int  i = 0; i <= 7; i++) {
    if (currentMillis >= interval * i) {
      if (ledState[i] == HIGH) {
        if (currentMillis - (previousMillis[i] + (interval * i)) >= durationOn) {
          ledState[i] = LOW;
          previousMillis[i] += durationOn;
        }
      } else {
        if (currentMillis - (previousMillis[i] + (interval * i)) >= durationOff) {
          ledState[i] = HIGH;
          previousMillis[i] += durationOff;
        }
      }
      sr.set(leds[i], ledState[i]);
    }
  }
}

int readPot() {
  int potValue = analogRead(A0);
  potValue = map(potValue, POT_LOW, POT_HIGH, SLOW, FAST);
  return potValue;
}

You have a rather complicated calculation om time, millis(). Check how initial values affect the code and whether they need to be altered when the pot is moved.

Use Serial Monitor in IDE and Serial.print from the code to tell what's going on in the code.

I won’t say about the rest but this section here has one obvious error and potential for worse as interval changes.

  for (int  i = 0; i <= 7; i++) {
    if (currentMillis >= interval * i) {      // <<<=== perhaps previous millis should be part of that?
      if (ledState[i] == HIGH) {
        if (currentMillis - (previousMillis[i] + (interval * i)) >= durationOn) {
          ledState[i] = LOW;
          previousMillis[i] += durationOn;
        }
      } else {
        if (currentMillis - (previousMillis[i] + (interval * i)) >= durationOff) {
          ledState[i] = HIGH;
          previousMillis[i] += durationOff;
        }
      }
      sr.set(leds[i], ledState[i]);
    }
  }

GoForSmoke:
I won't say about the rest but this section here has one obvious error and potential for worse as interval changes.

Be an angel and explain ...

...R

It makes sense that previousMillis should be factored in there, similar to below. Thanks for pointing that out. While, making that correction helps, my sketch clearly remains flawed as it still does not behave as desired. The change does prevent lights from remaining on, but the timing of the sequence still falls apart once the pot is adjusted.

GoForSmoke:
I won't say about the rest

Could you take a moment to comment on the rest? My concern is that it's more than just some minor tweaks to be made to my existing sketch and may, in fact, require an entirely different approach. If it is, I'm not averse to that.

I'm simply unclear on how to proceed.

Thanks.

What is the particular sequence that the leds should do ? Can you show a youtube video of it ?

The others have already mentioned problems with the sketch. To do it right you should not add something to a millis value or compare a millis value with a number. That means the 'interval * i' that you do with 'currentMillis' and 'previousMillis' is not okay. When removing that, the base of the sketch is gone and that means that you should try again with a different approach.

I think there should be only one millis() timer, since the leds do not blink independent of each other. They follow a sequence.
Changing the 'durationOn', 'durationOff' and 'interval' according to a potentiometer is no problem.
We have to find a new way for the sequence.

So, a flawed approach. I was hopeful that wasn't the case but it's what I suspected. I'm not sure how to get the adjustable staggered sequence I desire with a single timer, but then again I have limited experience with millis. The sequence has separate on/off durations and staggered start points. Ideally adjusting the pot will control the length of the overall cycle and the on/off lengths of the leds. The one thing in my favor is the all the leds will always behave the same in relation to one another. I managed to get the desired effect with my faulty code through trial and error but clearly it's not the correct way.

Video attached for good measure. Essentially it's a single direction larson scanner but there is a delay between subsequent leds turning on.

LED Sequence

Thanks.

Start with the Blink Without Delay: https://www.arduino.cc/en/tutorial/BlinkWithoutDelay.
Add different on and off durations: millis_different_on_and_off_times.ino.
Add the speed control.
Add a counter, I want to call this a ‘index’ because it is used for the array.
That’s all.

Either use ledState or count to 16 instead of counting to 8. I like the ledState, so let’s keep that.

The analogRead() does not need a pinMode(A0,INPUT) as you can see in the page about analogRead().

This next sketch is only an indication, I did not even think about it. It is just a rough idea. Can you finish it ?

const int POT = A0;
const byte leds[8] = {18, 22, 17, 16, 20, 19, 23, 21};
byte ledState[8] = {1, 1, 1, 1, 1, 1, 1, 1};
unsigned long previousMillis;
unsigned long interval = 100; // after 100 milliseconds, the leds start
int index = 0;   // the index or the counter for the led

void setup()
{
  Serial.begin(9600);
}

void loop()
{
  unsigned long currentMillis = millis();
  
  if( currentMillis - previousMillis >= interval)
  {
    previousMillis = currentMillis;
    
    if( ledState[index] == 1) // led was on ?
    {
      ledState[index] = 0;
      turn led "leds[index]" off
      interval = 400;

      // Ready with this led, advance to next led
      index++;
      if( index >= 8)
      {
        index = 0;
      }
    }
    else
    {
      ledState[index] = 1;
      turn led "leds[index]" on
      interval = 500;
    }
  }
}

Should the leds “bounce” from left to right and then from right to left ? Then an extra variable is needed that indicates if ‘index’ is counting up or down. That variable can be the up or down itself by making it -1 or +1

int delta = 1;   // start with counting up
int index = 0;  // start with first led
...
index += delta;
if( index >= 8)
{
  delta = -1;  // change direction.
  index = 6;  // for the sequence ...5, 6, 7, 6, 5...
}
if( index < 0)
{
  delta = 1;   // change direction.
  index = 1;  // for the sequence ...2, 1, 0, 1, 2...
}

Should there be a “trace” of dimmed leds behind the led that is on ? Then you can implement some kind of software PWM. The shift-register is fast enough to do that.

What about sound effects ?

Are you using 5 cascaded shift registers ? If not, then check how to use the ShiftRegister74HC595 library.

The <5> is for 5 cascaded shift registers: ShiftRegister74HC595<5> sr(A2, A3, 4);

Thank you. With a fresh approach and your first sketch, I was able to get a successful sequence that is controlled reliably by the pot.

Koepel:
Are you using 5 cascaded shift registers

Yes, I'm working with 5 shift registers. The sequence you helped me with is just one of a number of led sequences in the overall project.

Thanks again for your help. I appreciate the time you spent on your explanation and examples.

Why do you need a library for shift registers? Even the Arduino Foundations examples don't need a library.

Something to know is that all the leds on an output shift register share the same current limited at about 40mA. The pins all need limiting resistors and what to pick depends on how many leds will be lit in the same instant, maximum. If just one then 220R will let that one shine bright but if it's eight leds then even with 220R resistors, all 8 will be less bright so might as well pick a resistor to match the led at 5mA (40/8). Or use a transistor (and resistor) to power each led bright.

What do you want your leds to be able to do? Blink in sequence? Because then I would change led number after timeout (turn it OFF and the next one ON) and reset start time. There would be one interval to change when the pot is turned.


Bookmark this if you haven't already. Keep it open in a browser tab when you use the IDE, refer to anything you don't know well if only to get positive memory reinforcement, not holding wrong ideas and being sure. Looking up teaches almost as well as taking notes.
https://www.arduino.cc/reference/en/

Another good link to have.

PS -- Learn Bitmath! Start simple, get powerful.
https://playground.arduino.cc/Code/BitMath/

This is binary, count with 0's and 1's. Contrast to decimal with 0 to 9. Which is simpler?

GoForSmoke:
Why do you need a library for shift registers?

While a library for shift registers certainly isn't necessary, it does simplifies things.

Thanks for the sound advice at the end of your post.


I have a follow-up question ...
I have a couple different examples of how to blink an led at different intervals now. My question is how would one code a "delay" that delays the blinking cycle from starting at initialization for the following example:

// Courtesy of UKHeliBob 

unsigned long periods[] = {1000, 2000};
char * messages[] = {"ZERO", "ONE"};
const byte ledPin = 13;
byte ledState = 0;
unsigned long currentMillis;
unsigned long startMillis;

void setup()
{
  Serial.begin(115200);
  pinMode(ledPin, OUTPUT);
}

void loop()
{
  currentMillis = millis();
  if (currentMillis - startMillis >= periods[ledState % 2])
  {
    ledState++;
    digitalWrite(ledPin, ledState % 2);
    Serial.println(messages[ledState % 2]);
    startMillis = currentMillis;
  }
}

A standard non-blocking delay:

if (delayRunning && ((millis() - delayStart) >= DELAY_TIME)) {
    delayRunning = false;
    // Execute an event
}

doesn't work or at least not how I've experimented with it. Does the startMillis variable need to be initialized with a value equal to the delay or is that improper technique and I'm looking at this all wrong? All the examples I've found using the delay example above (or similar) are not delaying a time dependent event within ... I'm guessing there's a reason for that.

How would one go about delaying the blink cycle cited above for say 500 ms, with the led starting the cycle lit.

Thanks for any help.

Here is a simple example that goes 1 level deep at
See in the comments the delay code that void BlinkPattern() replaces does have fewer lines…
and then try adding a stop/go button to both, hehehe.

// add-a-sketch_un-delay 2018 by GoForSmoke @ Arduino.cc Forum
// Free for use, Apr 30/18 by GFS. Compiled on Arduino IDE 1.6.9.
// This sketch shows a general method to get rid of delays in code.
// You could upgrade code with delays to work with add-a-sketch.

#include <avr/io.h>
#include "Arduino.h"

const byte ledPin = 13;
unsigned long delayStart, delayWait;

void setup()
{
  Serial.begin( 115200 );
  Serial.println( F( "\n\n\n  Un-Delay Example, free by GoForSmoke\n" ));
  Serial.println( F( "This sketch shows how to get rid of delays in code.\n" ));

  pinMode( ledPin, OUTPUT );
};


/* The section of the original sketch with delays:
 * 
 * digitalWrite( ledPin, HIGH );   --  0
 * delay( 500 );
 * digitalWrite( ledPin, LOW );    --  1
 * delay( 250 );
 * digitalWrite( ledPin, HIGH );   --  2
 * delay( 250 );
 * digitalWrite( ledPin, LOW );    --  3
 * delay( 250 );
 * digitalWrite( ledPin, HIGH );   --  4
 * delay( 1000 );
 * digitalWrite( ledPin, LOW );    --  5
 * delay( 1000 );
 */

byte blinkStep; // state tracking for BlinkPattern() below

void BlinkPattern()
{
  // This one-shot timer replaces every delay() removed in one spot.  
  // start of one-shot timer
  if ( delayWait > 0 ) // one-shot timer only runs when set
  {
    if ( millis() - delayStart < delayWait )
    {
      return; // instead of blocking, the undelayed function returns
    }
    else
    {
      delayWait = 0; // time's up! turn off the timer and run the blinkStep case
    }
  }
  // end of one-shot timer

  // here each case has a timed wait but cases could change Step on pin or serial events.
  switch( blinkStep )  // runs the case numbered in blinkStep
  {
    case 0 :
    digitalWrite( ledPin, HIGH );
    Serial.println( F( "Case 0 doing something unspecified here at " ));
    Serial.println( delayStart = millis()); // able to set a var to a value I pass to function
    delayWait = 500; // for the next half second, this function will return on entry.
    blinkStep = 1;   // when the switch-case runs again it will be case 1 that runs
    break; // exit switch-case

    case 1 :
    digitalWrite( ledPin, LOW );
    Serial.println( F( "Case 1 doing something unspecified here at " ));
    Serial.println( delayStart = millis());
    delayWait = 250;
    blinkStep = 2;
    break;

    case 2 :
    digitalWrite( ledPin, HIGH );
    Serial.println( F( "Case 2 doing something unspecified here at " ));
    Serial.println( delayStart = millis());
    delayWait = 250;
    blinkStep = 3;
    break;

    case 3 :
    digitalWrite( ledPin, LOW );
    Serial.println( F( "Case 3 doing something unspecified here at " ));
    Serial.println( delayStart = millis());
    delayWait = 250;
    blinkStep = 4;
    break;

    case 4 :
    digitalWrite( ledPin, HIGH );
    Serial.println( F( "Case 4 doing something unspecified here at " ));
    Serial.println( delayStart = millis());
    delayWait = 1000;
    blinkStep = 5;
    break;

    case 5 :
    digitalWrite( ledPin, LOW );
    Serial.print( F( "Case 5 doing something unspecified here at " ));
    Serial.println( delayStart = millis());
    delayWait = 1000;
    blinkStep = 0;
    break;
  }
}


void loop()  // runs over and over, see how often
{            
  BlinkPattern();
}

I put 1 “timer” for all the times as the 1st thing the function runs. The code sets a wait-until time somewhere in the switch-case and the switch case don’t get run until the wait is over. If the wait is zero there is no wait. The faster the function runs, the more responsive it is.

I didn't want to add this to the last post. Once you've run that sketch and traced out how the source executed (cause it -is- simple) it's time to show you how responsive is responsive.

Here is another add-a-sketch, they are made to copy parts of one to another and still run. This one counts loops.

// add-a-sketch_loop_counter 2018 by GoForSmoke @ Arduino.cc Forum
// Free for use, Apr 29/2018 by GFS. Compiled on Arduino IDE 1.6.9.
// This sketch counts times that loop has run each second and prints it.
// It uses the void LoopCounter() function that does not block other code.

#define microsInOneSecond 1000000UL

void setup()
{
  Serial.begin( 115200 );
  Serial.println( F( "\n\n\n  Loop Counter, free by GoForSmoke\n" ));
  Serial.println( F( "This sketch counts times that loop has run each second and prints it." ));
}

void LoopCounter() // tells the average response speed of void loop()
{ // inside a function, static variables keep their value from run to run
  static unsigned long count, countStartMicros; // only this function sees these

  count++; // adds 1 to count after any use in an expression, here it just adds 1.
  if ( micros() - countStartMicros >= microsInOneSecond ) // 1 second
  {
    countStartMicros += microsInOneSecond; // for a regular second
    Serial.println( count ); // 32-bit binary into decimal text = many micros
    count = 0; // don't forget to reset the counter 
  }
}

void loop()  // runs over and over, see how often with LoopCounter()
{
  LoopCounter(); // the function runs as a task, the optimizer will inline the code.
}

Copy some lines to the undelay sketch:

// put up top
#define microsInOneSecond 1000000UL
// put above void setup()
void LoopCounter() // tells the average response speed of void loop()
{ // inside a function, static variables keep their value from run to run
  static unsigned long count, countStartMicros; // only this function sees these

  count++; // adds 1 to count after any use in an expression, here it just adds 1.
  if ( micros() - countStartMicros >= microsInOneSecond ) // 1 second
  {
    countStartMicros += microsInOneSecond; // for a regular second
    Serial.println( count ); // 32-bit binary into decimal text = many micros
    count = 0; // don't forget to reset the counter 
  }
}
// put in void loop()
  LoopCounter(); // the function runs as a task, the optimizer will inline the code.

You can do this with any non-blocking sketch, add tasks because that what runs in void loop() is tasks the asy way. What the loop counter gives you is a way to measure the load you code is rolling along. When your loop() is averaging > 50000 per second, your code can be pretty freaking responsive.

You may need to set Serial Monitor to 115200 you haven't already, slow serial invites output jams.

I wrote code for money almost 20 years. Using small tasks beats the crud out of building logic-structure castles especially after new features get wedged in, the 80's recipe for spaghetti code. Save yourself! Do go that route!