Zero timing data and yet still my LED blinks.

[bongobreak]

Hello good people of Arduino Land!

This question relates to my ongoing development of my water drop controller code.

The most recent sketch was posted here yesterday:
(In two halves - 9000 char limit)

LINK:

https://forum.arduino.cc/index.php?topic=689635.15

The code works very well - I've had lots of really helpful advice along the way, but since the most recent improvements I have seen some puzzling results that I can't nail down.

I should point out that when I began this journey, I was advised to stop using delay(), so I did just that, and now there are absolutely no delay() functions anywhere in my sketch...which I'm rather proud of...c'mon, I'm a beginner!

Hardware:

• Arduino Nano clone (Elegoo, with Uno BL)
• I²C 1602 LCD (generic)
• Keyes KY-040 rotary encoder
• Seven tactile switches - six for value select, one for start button

Summary of operation:

• Six time values (milliseconds) are entered via a single rotary encoder on the fly
• These are the water drop sizes and pauses between the drops.

Order:

• Pause One

• Drop One

• Pause Two

• Drop Two

• Pause Three

• Drop Three
  (Arbitrary names)

• Single push button to start the drop sequence - blinking an LED in this case (standing in for a 12VDC solenoid valve)

• Change values (or not)

• repeat with changed data or same if unchanged

The Problem.

If a zero value is entered, then I would expect to see no activity for that particular part of the sequence.
However, this is not what happens.

I'm still seeing a sizeable blink, even when there is a zero value entered into a drop size field.

Furthermore, if I set all time values to zero, I still see three rapid blinks, with a tiny pause in between them.

This has me very puzzled!

An earlier version of the code (admittedly rather poorly written, with some fairly sizeable programming errors due to lack of knowledge) seemed to work fine, ie. no blinks for zero time values.

The older (incorrectly coded, but working - 'old wonky') version of the code can be found here:

https://forum.arduino.cc/index.php?topic=689635.1

(It starts at post #1, and ends on post #3)

It seemed that when I implemented the 'switch-case' function, rather than the clumsy and just plain wrong way I did it before, it has introduced some kind of delays into the timing data.

I decided to pare down my code to it's bare minimum, to see if that is still working correctly, and it seems fine.

Here is the test code:

const byte ledPin = 4;
const byte buttonPin = 2;

bool buttonState = HIGH;
bool buttonStateLast = HIGH;
bool ledReady = false;

//unsigned long value[6] = {0, 0, 0, 0, 0, 0}; // test for zero time
unsigned long value[6] = {500, 100, 1000, 500, 1500, 2000};

unsigned long previousMillis = 0;

void setup() {

  Serial.begin(9600);

  pinMode(buttonPin, INPUT_PULLUP);
  pinMode(ledPin, OUTPUT);
  digitalWrite(ledPin, LOW);

}

void loop() {

  buttonState = digitalRead(buttonPin);
  if (buttonState != buttonStateLast) {
    if (buttonState == LOW) {
      ledReady = true;
      previousMillis = millis();
    }
    buttonStateLast = buttonState;
  }

  if (ledReady) {
    static int ndx = 0;
    if (ndx < 6) {
      if (millis() - previousMillis >= value[ndx]) {
        previousMillis = millis();
        digitalWrite(ledPin, ! digitalRead(ledPin));
        ndx++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
    }
  }
}

I decided to buy a proper, 'grown-up' oscilloscope too (well...a better one than the 'starter' scope I have), so I attach an image, which clearly shows that for zero data entered (except for the last value - 100ms), there is clearly two pulses (and two pauses) that shouldn't be there.

It's not the best image, but it's just visible!

My scope is set to 100ms/div, so those 'pulses' seem to be about ~25ms long.
My Shako 12VDC valves have a response time of ~20ms, so these pulses are going to be a problem.
And also the 100ms pulse at the end of the sequence, seems to be too long aswell...though I'm sure this is related to the overall timing error somewhere.

I wonder if some kind expert wouldn't mind casting an eye over this conundrum, and perhaps nudging me in the right direction...I'm not sure what else to try, other than to go back to the (incorrect) code I was using before...which would really be a shame since I'd much prefer to get this code right!

Many thanks folks.

[gcjr]

i think you need to change the output state when the button is pushed so that the first delay period of 500 causes a change in the output rather that just delaying before the 1st change

and if you do that, you want an odd number of delay period in order to end at the same starting state

[bongobreak]

gcjr:
i think you need to change the output state when the button is pushed so that the first delay period of 500 causes a change in the output rather that just delaying before the 1st change

and if you do that, you want an odd number of delay period in order to end at the same starting state

Thank you for the reply @gcjr.

I'm not quite sure I follow you however.
The test sketch above was just for me to confirm that the problem I have, isn't related to the core piece of code that blinks the LED (and later will operate the valve(s)).
It is indeed working as expected, moreover, the unsigned long value[6] array data is purely arbitrary and for test purposes only.

My full, main sketch (not the test), contains this very same piece of code, and it works very well with an earlier (badly written) version of the sketch.
Owing to a lack of knowledge on my part, to ensure that a buttonPress() function was held, I was using digitalWrite() to hold the corresponding button pin LOW after the button was pressed...which I realize is not the right way to do it!
But when I implemented the correct 'switch-case' system to select button presses, it seemed to introduce this weird timing error.

Could it be that the switch case takes a long to time (in Arduino terms) to poll every time?
If this is the case, then how do I remedy this?

[gcjr]

weird timing error.

what exactly is the error

the scope trace looks correct to me

[bongobreak]

gcjr:
what exactly is the error

the scope trace looks correct to me

Thanks again for responding greg.

I know I'm not explaining it very well - I'm quite inexperienced, please forgive me.

I would expect the scope to yield a single pulse of 100ms only.
As if I placed the following timing data into the array in the test:

unsigned long value[6] = {0, 0, 0, 0, 0, 100};

This was indeed the timing data that I entered in the main sketch, and yet I see those two short pulses, which shouldn't be there.

When I used the older version of the sketch (with the incorrect way to call the buttonPress() functions) it worked fine...no pulses for zeros entered.

But when I coded it the right way, using switch-case, I see timing errors, or pulses that shouldn't be there - shown in the scope trace.

I hope this is clear, sorry if I am still not explaining it very well.

Edit

Here is a scope image of what I would expect to see with the timing data mentioned earlier:
unsigned long value[6] = {0, 0, 0, 0, 0, 100};

This was taken using the old sketch - the incorrect sketch, which I would dearly love to avoid using, since it uses some really clunky programming that I know is really bad form!

And this is a scope image of the same timing data, {0, 0, 0, 0, 0, 100} taken using the test sketch posted earlier in the thread:

Again, it's nice, clean 100ms pulse, so my older, wonky sketch yields the same, decent results as the bare-minimum test sketch.

I'm stumped!

[cattledog]

As if I placed the following timing data into the array in the test:
unsigned long value[6] = {0, 0, 0, 0, 0, 100};
This was indeed the timing data that I entered in the main sketch, and yet I see those two short pulses, which shouldn't be there.

I would recommend that in the main sketch, you print out the six values of value[] when you enter the ledReady conditional.

Given the rather complex way those values get set in the full code referenced in the other thread, its best to check.

[bongobreak]

cattledog:
I would recommend that in the main sketch, you print out the six values of value[] when you enter the ledReady conditional.

Given the rather complex way those values get set in the full code referenced in the other thread, its best to check.

Thanks for the reply @cattledog.

Using the (now familiar) timing data:
unsigned long value[6] = {0, 0, 0, 0, 0, 100};

I have placed a Serial.print() in the main sketch, as you suggest, like so:

  if (ledReady) {
    static int ndx = 0;
    if (ndx < 6) {
      if (millis() - previousMillis >= value[ndx]) {
        previousMillis = millis();
        digitalWrite(ledPin, ! digitalRead(ledPin));
        Serial.print(value[ndx]);
        Serial.print(",");
        ndx ++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
    }
  }

...and the timing data shown on the serial monitor is:

0,0,0,0,0,100

So at least the correct timing data is being displayed, but the erroneous pulses are still there.

Edit

I have just tried it with the old (wonky) sketch, and I get the exact same result, but of course no erroneous pulses.

[cattledog]

It's quite possible that something in the loop is taking the 25 ms, and with this led statement you will get alternating on and off.

if (millis() - previousMillis >= value[ndx]) {
        previousMillis = millis();
        digitalWrite(ledPin, ! digitalRead(ledPin));
        Serial.print(value[ndx]);
        Serial.print(",");
        ndx ++;
      }

If you are trying to generate 0 delay timing between drops, do you want them to be on or off?

You might consider recrafting values[ndx] to be a two dimensional array with the timing and the state. Then, instead of alternating the led drive state, specify it directly.

[bongobreak]

cattledog:
It's quite possible that something in the loop is taking the 25 ms, and with this led statement you will get alternating on and off.

if (millis() - previousMillis >= value[ndx]) {

previousMillis = millis();
        digitalWrite(ledPin, ! digitalRead(ledPin));
        Serial.print(value[ndx]);
        Serial.print(",");
        ndx ++;
      }

This makes sense.

Through a (naive beginners) process of elimination, I can only assume it has something to do with the switch-case addition to the code.
I can't explain how it works perfectly when I used the clumsy (and just plain wrong) series of digitalWrite() commands to hold the button pins HIGH or LOW depending on which buttonPress() function I was calling.

When I posted that sketch (referenced above as 'old wonky sketch') I know I probably induced a great deal of vomiting (figuratively speaking) from the experts unfortunate enough to see it...and I certainly felt 'dirty' doing it myself - it just felt wrong...but it did work!

If you are trying to generate 0 delay timing between drops, do you want them to be on or off?

The delays between the pulses (I almost don't want to use the word 'delay' now!!) are indeed indicated by the LED being off, or LOW - this is how I'll be driving my solenoid valves eventually.
And it follows that when the LED is on or HIGH, the valve is open.

You might consider recrafting values[ndx] to be a two dimensional array with the timing and the state. Then, instead of alternating the led drive state, specify it directly.

A two-dimensional array you say?
I've only just learned one-dimensional arrays a few days ago!
(And probably not very deeply either!)

It does make sense though, and I will certainly begin learning.

I still can't quite understand how these ~25ms pauses (I think I prefer the word 'pause'! ) crept in there though.
I've tried to keep the main loop() as minimal as possible, so as to reduce the likelihood of timing errors.
Indeed I would expect errors in the order of microseconds, but 25ms per value[] field is huge!

Anyway, thanks for the time you've spent helping me through this @cattledog, it's most appreciated!
I am turning in now - nearly 2:00am here - my tired, old brains are beginning to malfunction!

I'll pick this up again tomorrow.

Cheers

[alto777]

This is quite bizarre.

It's tired and I'm getting late, but I had to hack this a bit. Finally I added a mechanism so the sequence is played out within one call to loop: the while statement I added means the sequence is done within its body, not through repeated calls to loop. This seems to have made the timing more rational. I sped up the baud rate to reduce any timing effect of printing.

const byte ledPin = LED_BUILTIN;
const byte buttonPin = 2;

bool buttonState = HIGH;
bool buttonStateLast = HIGH;
bool ledReady = false;

unsigned long value[6] = {0, 0, 0, 0, 0, 100}; // test for zero time

// unsigned long value[6] = {500, 500, 1000, 1000, 2000, 2000};

// unsigned long value[6] = {50, 100, 200, 300, 150, 200};

unsigned long previousMillis = 0;

void setup() {
  Serial.begin(115200);

  pinMode(buttonPin, INPUT_PULLUP);
  pinMode(ledPin, OUTPUT);
  digitalWrite(ledPin, LOW);

  digitalWrite(ledPin, HIGH);
  delay(1000);
  digitalWrite(ledPin, LOW);
  Serial.println("     setup and ready to rumble");
}

void loop() {
  static unsigned long lastToggle;
  static int ndx = 0;

  buttonState = digitalRead(buttonPin);
  if (buttonState != buttonStateLast) {
    if (buttonState == LOW) {
      ledReady = true;
      Serial.println("launching sequence...");

      lastToggle = micros();
      
      previousMillis = millis();
    }
    buttonStateLast = buttonState;
  }

  if (ledReady) {


while (ledReady) {

    if (ndx < 6) {
      if (millis() - previousMillis >= value[ndx]) {
        previousMillis = millis();

		Serial.println(micros() - lastToggle);
		lastToggle = micros();
		
        digitalWrite(ledPin, ! digitalRead(ledPin));
        ndx++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
      Serial.println("sequence finished...");
    }
}

  }
}

You also see I added some printing to see what's what (no oscilloscope, haha). I got this, which shows just ~20 microseconds between LED state changes, as expected:

     setup and ready to rumble
launching sequence...
20
16
16
16
16
99584
sequence finished...

One thing I note is that there is no real button debouncing. When loop() was the looping mechanism I saw multiple "launching sequence" printed before any of the sequence. loop() came around so quick it caught multiple button changes… with the wile I stuck in there, the button is effectively ignored until the sequence is completed.

In your original code, there is a great opportunity to make all 6 button handlers into just one function. The whole idea of arrays is to facilitate this. You have [1] all over in handler2, [2] all over in handler 3, &c. Make these variable, pass a number to the function and use it as the index where you've now hard coded it 6 times.

-> Also, it's time for you to start thinking of zero as a perfectly good number. It takes some time to get accustomed to, but C/C++ arrays run from zero, so if I have N things, I usually number them 0 to N - 1. <-

True there are other slight differences. These can be handled by additional arrays, so for example the x position is not hard coede in every function, it is a variable, like

lcd.setCursor(valueXPosition[buttonNumber], 0);
lcd.print(value[buttonNumber]);

if buttonNumber was the argument to the function. Any other differences can be handled the same way.

The section that clears the ">" could instead first call a separate function that clears them all. the next lines of code could prints the ">" where you want it viz:

lcd.setCursor(crusorXPostion[bnuttonNumber], crusorYPosition[buttonNumber]);
lcd.print(">");

You just have to set up the arrays.

The beauty is that now you can change (or fix) the handling of all 6 button handling by tinkering with just one function, and you can move stuff around by playing with the arrays instead of the code in one of six functions.

And of course here is where it starts to think of a structure to hold all the information about each button. But parallel arrays work just fine.

Now I've gone past time to watch another episode of "How to Get Away With Murder".

a7

[bongobreak]

Hi a7, hope you are well.

alto777:
This is quite bizarre.

I agree!

It's tired and I'm getting late, but I had to hack this a bit. Finally I added a mechanism so the sequence is played out within one call to loop: the while statement I added means the sequence is done within its body, not through repeated calls to loop. This seems to have made the timing more rational. I sped up the baud rate to reduce any timing effect of printing.

const byte ledPin = LED_BUILTIN;

const byte buttonPin = 2;

bool buttonState = HIGH;
bool buttonStateLast = HIGH;
bool ledReady = false;

unsigned long value[6] = {0, 0, 0, 0, 0, 100}; // test for zero time

// unsigned long value[6] = {500, 500, 1000, 1000, 2000, 2000};

// unsigned long value[6] = {50, 100, 200, 300, 150, 200};

unsigned long previousMillis = 0;

void setup() {
 Serial.begin(115200);

pinMode(buttonPin, INPUT_PULLUP);
 pinMode(ledPin, OUTPUT);
 digitalWrite(ledPin, LOW);

digitalWrite(ledPin, HIGH);
 delay(1000);
 digitalWrite(ledPin, LOW);
 Serial.println("     setup and ready to rumble");
}

void loop() {
 static unsigned long lastToggle;
 static int ndx = 0;

buttonState = digitalRead(buttonPin);
 if (buttonState != buttonStateLast) {
   if (buttonState == LOW) {
     ledReady = true;
     Serial.println("launching sequence...");

lastToggle = micros();
     
     previousMillis = millis();
   }
   buttonStateLast = buttonState;
 }

if (ledReady) {

while (ledReady) {

if (ndx < 6) {
     if (millis() - previousMillis >= value[ndx]) {
       previousMillis = millis();

Serial.println(micros() - lastToggle);
lastToggle = micros();

digitalWrite(ledPin, ! digitalRead(ledPin));
       ndx++;
     }
   }
   else {
     ndx = 0;
     ledReady = false;
     Serial.println("sequence finished...");
   }
}

}
}

Thank you so much a7 for taking the time and trouble to analyse my code and offer advice, I really appreciate it immensely!

You also see I added some printing to see what's what (no oscilloscope, haha). I got this, which shows just ~20 microseconds between LED state changes, as expected:

     setup and ready to rumble

launching sequence...
20
16
16
16
16
99584
sequence finished...

Ahh, I completely forgot about micros(), how silly of me...I believe the kids would call that an 'Epic Fail'!!
Thanks for reminding me, it's very interesting.
As you say ~20 microseconds is to be expected, so the test code yields a reasonable result.

One thing I note is that there is no real button debouncing. When loop() was the looping mechanism I saw multiple "launching sequence" printed before any of the sequence. loop() came around so quick it caught multiple button changes… with the wile I stuck in there, the button is effectively ignored until the sequence is completed.

That's clever use of a while loop() - I must remember that!

I assume you mean no debouncing on the start button?
You're right, although I did place a 100nF cap, but that's quite crude...I should debounce it properly, again thanks for the reminder.

All these seemingly small details do indeed add up, and are sure to affect the overall performance...I must train my eyes to be more critical of these tiny details!

In your original code, there is a great opportunity to make all 6 button handlers into just one function. The whole idea of arrays is to facilitate this. You have [1] all over in handler2, [2] all over in handler 3, &c. Make these variable, pass a number to the function and use it as the index where you've now hard coded it 6 times.

You're quite right, this is definitely worthy of spending time to refine.
I'm slowly getting used to the concept of arrays - I have only really begun to learn about them a few days ago, so it's quite tricky, but I think I'm getting the hang of it.

In fact, I did try to implement a single function which handles all the button calls, but I got myself into knots again, so reverted back to individual functions and button calls until I understood it better.

It's probably time I learned how to do it the right way!

-> Also, it's time for you to start thinking of zero as a perfectly good number. It takes some time to get accustomed to, but C/C++ arrays run from zero, so if I have N things, I usually number them 0 to N - 1. <-

Indeed!
It's a tough habit to shift, but of course you're right.
Coming from a science background, I shouldn't avoid zero referenced data - I will try harder to shift my numerical data handling by '-1'

True there are other slight differences. These can be handled by additional arrays, so for example the x position is not hard coede in every function, it is a variable, like

lcd.setCursor(valueXPosition[buttonNumber], 0);
lcd.print(value[buttonNumber]);

if buttonNumber was the argument to the function. Any other differences can be handled the same way.

The section that clears the ">" could instead first call a separate function that clears them all. the next lines of code could prints the ">" where you want it viz:

lcd.setCursor(crusorXPostion[bnuttonNumber], crusorYPosition[buttonNumber]);
lcd.print(">");

You just have to set up the arrays.

This is very helpful a7, thank you.
This is what I would like to achieve - today hopefully.

I feel that I am close to getting it - indeed I have created arrays for cols[] and rows[] in my main sketch, so I just need to expand it to include arrays for cursorPos[] etc.

The beauty is that now you can change (or fix) the handling of all 6 button handling by tinkering with just one function, and you can move stuff around by playing with the arrays instead of the code in one of six functions.

And of course here is where it starts to think of a structure to hold all the information about each button. But parallel arrays work just fine.

Yes, that is where I would like to go eventually - in fact in a previous thread of mine, @Todd1962 kindly introduced me to the concept of 'structs', but I think it's still a little over my head at the moment, although I understand the concept - they're just multi-dimensional arrays aren't they?

Also, by parallel arrays, I assume you mean what I've been doing so far...for example:

int anyValue[6] = {0, 1, 2, 3, 4, 5};
int anyOtherValue[6] = {0, 10, 20, 30, 40, 50};

Now I've gone past time to watch another episode of "How to Get Away With Murder".

a7

Sorry you missed your show a7, hope there is a repeat that you can catch up with.

A huge thank you a7 for spending time with my perplexing issue - I think I have a clear idea of where I should be going now, and hopefully I will achieve some good results today.

I will study and attempt the things you suggest - especially refining the buttonPress() calls which could possibly be where my timing data is affected.

I'll keep you posted with updates soon.

Take care a7.

Cheers!

[alto777]

Looking again at the other thread I see that @ToddL1952 has written out the ideas I only hinted at, although with structs.

I was going to point out the other thing that happens: your switch/case of

case 3 :
buttonHandler3();
break;

totally disappears, now it’s just

buttonHandler(buttonNumber);

but again, he did all that nicely.

At a glance, struct tutorials and examples abound, but invariably seem rapidly to introduce a few more concepts at the same time. There is no problem (nor shame) using parallel arrays, TBH I am often too lazy or in a hurry to go the struct route, but structs are an important feature of the language and after you get over learning the syntax you will find them to be convenient.

The important point I guess is to keep an eye out for patterns of repeated code and use array variables, be they one array of structs or parallel arrays of, say, integers, to make things a bit tighter.

You noticed even the 6 lines of initialization and other places where he replaces that with a for loop over an index.

You are dealing with N = 6. Somewhere between N = 1 and N = 100 it will usually make sense to exploit arrays and loops rather than N lines of code or worse, N number of fairly identical functions.

For me, N is about 3, haha, and even when I’m doing an N = 1 kinda thing I might think a step ahead to consider it may be soon enough I’ll need another, and then another &c. and should just start with array variables right away.

Often here the clue we spot is variables named like

button1, button2, button3

a7

BTW, the show streams on Netflix, I just meant I’d stayed up past bedtime.

[bongobreak]

alto777:
Looking again at the other thread I see that @ToddL1952 has written out the ideas I only hinted at, although with structs.

I was going to point out the other thing that happens: your switch/case of

case 3 :
buttonHandler3();
break;

totally disappears, now it’s just

buttonHandler(buttonNumber);

but again, he did all that nicely.

I think I can see what you mean, so if I understand it correctly, I can get rid of the switch-case altogether, and use a simple if() statement and indexed for() loop as with the other repetitive tasks?

I have a feeling that this would indeed solve my issue, since the implementation of the switch-case is where I think my timing problem started.

At a glance, struct tutorials and examples abound, but invariably seem rapidly to introduce a few more concepts at the same time. There is no problem (nor shame) using parallel arrays, TBH I am often too lazy or in a hurry to go the struct route, but structs are an important feature of the language and after you get over learning the syntax you will find them to be convenient.

The important point I guess is to keep an eye out for patterns of repeated code and use array variables, be they one array of structs or parallel arrays of, say, integers, to make things a bit tighter.

Wise words indeed, I am starting to look at my code in a different way, looking for patterns etc., - it seems I've fully embraced arrays now, they really are superb!

You noticed even the 6 lines of initialization and other places where he replaces that with a for loop over an index.

You are dealing with N = 6. Somewhere between N = 1 and N = 100 it will usually make sense to exploit arrays and loops rather than N lines of code or worse, N number of fairly identical functions.

For me, N is about 3, haha, and even when I’m doing an N = 1 kinda thing I might think a step ahead to consider it may be soon enough I’ll need another, and then another &c. and should just start with array variables right away.

Often here the clue we spot is variables named like

button1, button2, button3

a7

Indeed, I must thank you yet again for encouraging me to look for patterns in my sketches, it's such a big step for me, and I really appreciate the efficiency of arrays now...I'm sure it won't be long til I get a handle on structs too.

Update

I have begun to implement your advice in your earlier post, namely everything is now zero-referenced, startButton is properly debounced, and I got rid of the last delay() statement - I forgot there was still one lingering in the void setup()!

I have begun to set up some cursorCols[] and cursorRows[] arrays, in preparation for refining the buttonPress() calls, in the hope that I can reduce all those repetitive functions, and simplify the whole sketch.

BTW, the show streams on Netflix, I just meant I’d stayed up past bedtime.

Glad to hear you can catch up - Netflix is a wonderful thing isn't it?

Thanks again a7.

[cattledog]

That's clever use of a while loop() - I must remember that!

The while loop() for the led sequence is essentially no different than using delay(). It is blocking.

You need to consider you needs. Do you have a program where you sequentially make adjustments, and then run some drops/leds, or do you have a program where you want to make adjustments when you are running the drops/leds. In the first case, delay() or while() is fine, although that architecture may limit the flexibility of what you can do to modify or enhance the program.

The buttonPressX() functions have significant lcd display management and the 25 ms may be reasonable. You can use the function timing techniques with micros to see how long they really take. It's possible that the toggling of the output you see is that it takes the time to perform the buttonPress fuction and then return to the ledReady block.

The entire toggle issue in time match needs to be addressed. Even if you use blocking architecture, but you want to have 0 timings then the alternating 20 microsecond pulse may be an issue.

[bongobreak]

cattledog:
The while loop() for the led sequence is essentially no different than using delay(). It is blocking.

Ahh, I understand, thanks @cattledog.
Perhaps I'm a little too eager (and maybe naive) in trying so hard to rid my code of all the delay() statements!
I suppose you could say that the 'Stop using delay()' lesson really made an impression on me, and maybe I've taken it a little too much to heart!

You need to consider you needs. Do you have a program where you sequentially make adjustments, and then run some drops/leds, or do you have a program where you want to make adjustments when you are running the drops/leds. In the first case, delay() or while() is fine, although that architecture may limit the flexibility of what you can do to modify or enhance the program.

Time value adjustments are only necessary while the program is 'waiting'.
However, I wouldn't want to 'close any doors' in the future, but since that will be the only way this system will eventually operate, I can't see a problem with following your former piece of advice.

I may want to add more valves in the future, or perhaps add more drops to the sequence, but I will always make time adjustments while the system is waiting.

The buttonPressX() functions have significant lcd display management and the 25 ms may be reasonable. You can use the function timing techniques with micros to see how long they really take. It's possible that the toggling of the output you see is that it takes the time to perform the buttonPress fuction and then return to the ledReady block.

I can see what you're saying - makes sense.
I will do some testing with micros() to try to learn just what it is I'm dealing with.

However, there were no erroneous delays in my older (badly coded) sketch, and that had a very similar structure, and indeed shared a lot of the lcd display management features.
(Although 'management' should be used in it's loosest possible sense where I am concerned! )

Nevertheless, I still find this phenomenon terribly vexing indeed!

The entire toggle issue in time match needs to be addressed. Even if you use blocking architecture, but you want to have 0 timings then the alternating 20 microsecond pulse may be an issue.

You're right - it does need to be addressed.
Right now, I'm trying to implement some of the suggestions made by @alto777 earlier in this thread.
I'm currently trying to condense all the repetitive elements into a single function with the use of arrays, and I'm getting some positive results.

I've removed the 'switch-case' system, which is what started me down this 'time delay error' path, and it looks encouraging, though I have a lot to learn still.

Many thanks @cattledog, I shall take on board all of your suggestions, and I'm very grateful for the time you've spent in offering help and advice.

Edit

Going 'off-the-air' for a while....tea-time!

Stay safe folks!

[cattledog]

Time value adjustments are only necessary while the program is 'waiting'.
However, I wouldn't want to 'close any doors' in the future, but since that will be the only way this system will eventually operate, I can't see a problem with following your former piece of advice.

Yes. If the adjustment and running periods are completely separate and sequential then there is no problem with making the run period blocking with the while(ledReady).

Clean up the program with the arrays for the repetitive sections, and address the toggle issue with 0 pause timing.

Let's see what you bring back.

[bongobreak]

cattledog:
Yes. If the adjustment and running periods are completely separate and sequential then there is no problem with making the run period blocking with the while(ledReady).

Clean up the program with the arrays for the repetitive sections, and address the toggle issue with 0 pause timing.

Let's see what you bring back.

Thanks again @cattledog, I appreciate your input.

As it turns out, I have decided not to use while(ledReady) afterall.

As far as I can understand, @alto777 used it in the test code because there wasn't any button debounce code, and I gather it was to mitigate that issue.
I have put in some debounce code in my main sketch, and it seems to work quite well now.

I feel like I have covered quite a lot of ground today, I now have a sketch that is cleaner, more efficient, and does not require any tedious, repetitive functions.

I have managed to condense all those functions into a single function, indexed with the relevant buttonPress#.

The entered timing data seems accurate too - at least as accurate as I need it.
I haven't checked it yet, but there are no more erroneous pulses, so I'd call that a win!

However, I now have yet another issue...which is a familiar one:

I cannot seem to 'hold' the operation of the function unless I'm holding the button down - it doesn't update the counter unless my finger is pressing the button in other words.

And also I haven't figured out how to change the cursor position either, but that's a minor issue.

FYI, I shall post the entire sketch as it is now (in the next post), and I'll keep thinking about how I'm going to get the buttonPress(#) function to 'hold' while I enter the timing data.

I know that's not the correct nomenclature, but I don't know how else to describe it!

[bongobreak]

Here is the complete sketch.
(Managed to get it into one, single post now....progress right? )

#include <digitalIOPerformance.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x27, 16, 2);

const byte encPinA = 2;
const byte encPinB = 12;
const byte encSw = 4;
const byte ledPin = 5;
const byte startButton = 3;

const byte buttonPin[6] = {6, 7, 8, 9, 10, 11};

bool startButtonState = HIGH;
bool startButtonStateLast = HIGH;

unsigned long buttonDebounceLast[6] = {0, 0, 0, 0, 0, 0};
unsigned long previousMillis = 0;
bool buttonState[6] = {HIGH, HIGH, HIGH, HIGH, HIGH, HIGH};

byte valueCols[6] = {1, 5, 9, 1, 5, 9};
byte valueRows[6] = {0, 0, 0, 1, 1, 1};

byte cursorCols[6] = {0, 4, 8, 0, 4, 8};
byte cursorRows[6] = {0, 0, 0, 1, 1, 1};

bool encSwState = HIGH;
bool encSwStateLast = HIGH;

bool ledReady = false;
bool buttonReady = false;

volatile bool countUpFlag = false;
volatile bool countDnFlag = false;

unsigned long value[6] = {0, 0, 0, 0, 0, 0};
unsigned long valueLast[6] = {0, 0, 0, 0, 0, 0};

void setup() {

 Serial.begin(115200);

 lcd.init();
 lcd.backlight();
 lcd.begin(16, 2);
 lcd.clear();

 pinMode(encPinA, INPUT_PULLUP);
 pinMode(encPinB, INPUT_PULLUP);
 pinMode(encSw, INPUT_PULLUP);

 for (int i = 0; i < 6; i++) {
   pinMode(buttonPin[i], INPUT_PULLUP);
 }

 pinMode(startButton, INPUT_PULLUP);
 pinMode(ledPin, OUTPUT);
 digitalWrite(ledPin, LOW);

 attachInterrupt(digitalPinToInterrupt(encPinA), encISR, LOW);

 static unsigned long previousMillis = millis();
 lcd.setCursor(0, 0);
 lcd.print("Drop Controller");
 lcd.setCursor(0, 1);
 lcd.print("***  START  ***");
 while (millis() < previousMillis + 1000UL) {
 }
 lcd.clear();

 // Display values
 for (int i = 0; i < 6; i++) {
   lcd.setCursor(valueCols[i], valueRows[i]);
   lcd.print(value[i]);
 }
}

void encISR () {

 static unsigned long debounceLast = 0;

 if (millis() - debounceLast > 5) {
   if (digitalRead(encPinB) == LOW) {
     countUpFlag = true;
   } else {
     countDnFlag = true;
   }
 }
 debounceLast = millis();
}

void loop() {

 // Read buttonPin#
 for (int i = 0; i < 6; i++) {
   buttonState[i] = digitalRead(buttonPin[i]);
 }

 // Wait for startButton press
 static unsigned long startButtonDebounceLast = 0;
 if (millis() - startButtonDebounceLast > 20) {
   startButtonState = digitalRead(startButton);
   if (startButtonState != startButtonStateLast) {
     if (startButtonState == LOW) {
       ledReady = true;
       previousMillis = millis();
     }
     startButtonStateLast = startButtonState;
   }
   startButtonDebounceLast = millis();
 }

 // Start drop sequence
 if (ledReady) {
   static int ndx = 0;
   if (ndx < 6) {
     if (millis() - previousMillis >= value[ndx]) {
       previousMillis = millis();
       digitalWrite(ledPin, ! digitalRead(ledPin));
       ndx ++;
     }
   }
   else {
     ndx = 0;
     ledReady = false;
   }
 }

 // Wait for one of six buttonPress#
 for (int i = 0; i < 6; i++) {
   if (millis() - buttonDebounceLast[i] > 5) {
     if (buttonState[i] == LOW) {
       buttonPress(i);
     }
     buttonDebounceLast[i] = millis();
   }
 }
}

void buttonPress(byte buttonValue) {

 // Display values
 for (int i = 0; i < 6; i++) {
   lcd.setCursor(valueCols[i], valueRows[i]);
   lcd.print(value[i]);
 }

 if (value[buttonValue] != valueLast[buttonValue]) {
   lcd.setCursor(valueCols[buttonValue], valueRows[buttonValue]);
   lcd.print(value[buttonValue]);
   lcd.setCursor(valueCols[buttonValue], valueRows[buttonValue]);
   lcd.print("   ");
   valueLast[buttonValue] = value[buttonValue];
 }
 value[buttonValue] = min(300, max(0, value[buttonValue]));

 encSwState = digitalRead(encSw);
 if (encSwState != encSwStateLast) {
   if (encSwState == LOW) {
     value[buttonValue] = 0;
   }
   encSwStateLast = encSwState;
 }

 if (countUpFlag) {
   value[buttonValue]++;
 }
 if (countDnFlag) {
   value[buttonValue]--;
 }
 countUpFlag = false;
 countDnFlag = false;
}

[cattledog]

Nice job getting things simplified with arrays.

I cannot seem to 'hold' the operation of the function unless I'm holding the button down - it doesn't update the counter unless my finger is pressing the button in other words.

The switch case allowed you to call a function continuously without the button being held down, but when you switched to calling buttonPress(i) within the button reading routine, you lost that. I suggest that you add an additional variable called buttonActive, and set it in the button reading routine and then call the function based on that. I've also reorganized the location of where you call some things for more clarity. The debounce routine after the reading for the 6 buttons doesn't make sense to me, but I've left it be for now.

I see this notice in the read me for digitalIOperformance.h. Why are you using this library?

IMPORTANT
With the release of Arduino 1.6, this library is obsolete and mostly a historical curiosity for now. It doesn't support the latest boards or the new core/library format, etc.

See if this does more of what you want.

//#include <digitalIOPerformance.h>
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x27, 16, 2);

const byte encPinA = 2;
const byte encPinB = 12;
const byte encSw = 4;
const byte ledPin = 5;
const byte startButton = 3;

const byte buttonPin[6] = {6, 7, 8, 9, 10, 11};
byte buttonActive = 0;//add new variable

bool startButtonState = HIGH;
bool startButtonStateLast = HIGH;

unsigned long buttonDebounceLast[6] = {0, 0, 0, 0, 0, 0};
unsigned long previousMillis = 0;
bool buttonState[6] = {HIGH, HIGH, HIGH, HIGH, HIGH, HIGH};

byte valueCols[6] = {1, 5, 9, 1, 5, 9};
byte valueRows[6] = {0, 0, 0, 1, 1, 1};

byte cursorCols[6] = {0, 4, 8, 0, 4, 8};
byte cursorRows[6] = {0, 0, 0, 1, 1, 1};

bool encSwState = HIGH;
bool encSwStateLast = HIGH;

bool ledReady = false;
bool buttonReady = false;

volatile bool countUpFlag = false;
volatile bool countDnFlag = false;

unsigned long value[6] = {0, 0, 0, 0, 0, 0};
unsigned long valueLast[6] = {0, 0, 0, 0, 0, 0};

void setup() {
  Serial.begin(115200);

  lcd.init();
  lcd.backlight();
  lcd.begin(16, 2);
  lcd.clear();

  pinMode(encPinA, INPUT_PULLUP);
  pinMode(encPinB, INPUT_PULLUP);
  pinMode(encSw, INPUT_PULLUP);

  for (int i = 0; i < 6; i++) {
    pinMode(buttonPin[i], INPUT_PULLUP);
  }

  pinMode(startButton, INPUT_PULLUP);
  pinMode(ledPin, OUTPUT);
  digitalWrite(ledPin, LOW);

  attachInterrupt(digitalPinToInterrupt(encPinA), encISR, LOW);

  static unsigned long previousMillis = millis();
  lcd.setCursor(0, 0);
  lcd.print("Drop Controller");
  lcd.setCursor(0, 1);
  lcd.print("***  START  ***");
  while (millis() < previousMillis + 1000UL) {
  }
  lcd.clear();

  // Display values
  for (int i = 0; i < 6; i++) {
    lcd.setCursor(valueCols[i], valueRows[i]);
    lcd.print(value[i]);
  }
}

void encISR () {

  static unsigned long debounceLast = 0;

  if (millis() - debounceLast > 5) {
    if (digitalRead(encPinB) == LOW) {
      countUpFlag = true;
    } else {
      countDnFlag = true;
    }
  }
  debounceLast = millis();
}

void loop() {
  // Wait for startButton press
  static unsigned long startButtonDebounceLast = 0;
  if (millis() - startButtonDebounceLast > 20) {
    startButtonState = digitalRead(startButton);
    if (startButtonState != startButtonStateLast) {
      if (startButtonState == LOW) {
        ledReady = true;
        previousMillis = millis();
      }
      startButtonStateLast = startButtonState;
    }
    startButtonDebounceLast = millis();
  }
  // Start drop sequence
  if (ledReady) {
    static int ndx = 0;
    if (ndx < 6) {
      if (millis() - previousMillis >= value[ndx]) {
        previousMillis = millis();
        digitalWrite(ledPin, ! digitalRead(ledPin));
        ndx ++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
    }
  }
  // Read buttonPin#
  for (int i = 0; i < 6; i++) {
    buttonState[i] = digitalRead(buttonPin[i]);
  }
  // Debounce button reading#
  for (int i = 0; i < 6; i++) {
    if (millis() - buttonDebounceLast[i] > 5) {
      if (buttonState[i] == LOW) {
        //buttonPress(i);
        buttonActive = i;
      }
      buttonDebounceLast[i] = millis();
    }
  }
  buttonPress(buttonActive);//call function every pass
}

void buttonPress(byte buttonValue) {
  // Display values
  for (int i = 0; i < 6; i++) {
    lcd.setCursor(valueCols[i], valueRows[i]);
    lcd.print(value[i]);
  }

  if (value[buttonValue] != valueLast[buttonValue]) {
    lcd.setCursor(valueCols[buttonValue], valueRows[buttonValue]);
    lcd.print(value[buttonValue]);
    lcd.setCursor(valueCols[buttonValue], valueRows[buttonValue]);
    lcd.print("   ");
    valueLast[buttonValue] = value[buttonValue];
  }
  value[buttonValue] = min(300, max(0, value[buttonValue]));

  encSwState = digitalRead(encSw);
  if (encSwState != encSwStateLast) {
    if (encSwState == LOW) {
      value[buttonValue] = 0;
    }
    encSwStateLast = encSwState;
  }

  if (countUpFlag) {
    value[buttonValue]++;
  }
  if (countDnFlag) {
    value[buttonValue]--;
  }
  countUpFlag = false;
  countDnFlag = false;
}

[bongobreak]

Sorry I wasn't able to get back to you sooner @cattledog.

cattledog:
Nice job getting things simplified with arrays.

Thank you most kindly @cattledog!
It really does mean a great deal to me!

The switch case allowed you to call a function continuously without the button being held down, but when you switched to calling buttonPress(i) within the button reading routine, you lost that. I suggest that you add an additional variable called buttonActive, and set it in the button reading routine and then call the function based on that. I've also reorganized the location of where you call some things for more clarity.

I think I understand it a bit better now, perhaps this is where my timing errors originated, namely the continuous calling through the switch-case introduced extra delays.

Thank you very much for re-organising my sketch too, it is indeed easier to read now - most appreciated.

I will study the changes you've made too - it now makes perfect sense using the new variable buttonActive...I somehow knew it wouldn't be hugely complex, but I just couldn't see it.
I was trying all sorts of remedies: flags etc., (my buttonReady flag variable can be deleted now) but couldn't seem to get anything to work.

The debounce routine after the reading for the 6 buttons doesn't make sense to me, but I've left it be for now.

I'm not sure I understand what you mean.
I see where you've added the comment:

//Debounce button reading#

Is this what you're referring to?
Does it serve any purpose, or could I remove/modify it?
I wonder if you wouldn't mind elaborating please?

In an earlier version, I placed a 100nF capacitor to GND on each button, as a crude debounce, but deleted them after trying the software debounce approach.
My original plan was to be the use an RC circuit with a Schmitt inverter (either 74HCT14 or 40106), but I wanted to learn how to debounce purely in software, to save PCB space.

(I have deleted it nevertheless, and it works fine, so I suppose it was useless afterall!)

....digitalIOperformance.h. Why are you using this library?

Long story short, I had become inspired by the terrific water drop photography of Martyn Currey, who has been developing his water drop controller for a number of years.
(His 'beginner' tutorials were very helpful to me when I was building my first generation, single valve drop controller)

His main website:

http://www.martyncurrey.com/

In some older posts where he details the earlier versions of his drop controller sketch - based around the 328P chip (Nano) - he talks about this library, and indeed acknowledges that it is obsolete but still viable*(?)* for 328P based systems - upon which I am basing my designs too.

However, he also says that he doesn't know if there is indeed a performance increase for pin response, so I suppose it's inconclusive.

I have to admit that every new sketch I start, I usually copy/paste a 'standard' header of libraries, and that one still remains...force of habit.

You're quite right though, it is no longer supported, but all my sketches still compile, so I just leave it in there...not good form perhaps!

For your information, here is the link to the page where he talks about it:
(Scroll down to: Libraries)

https://www.dropcontroller.com/downloads/

See if this does more of what you want.

Code omitted for space saving measures - code is as above.

This works now, thank you @cattledog!
Each buttonPress now yields the intended function and I am able to vary each parameter without holding the button down.
(I still need to address the cursor position aspect however, although I could just add an LED as an indicator for each button...but that would be cheating wouldn't it? )

However, our old friend 'erroneous delays' have re-entered the fray yet again!
Those annoying pulses when zero timing data is entered, are back!

I thought I'd got away without having to address it thoroughly, but being that I am a colossal idiot, I should have paid closer attention to your advice...sorry @cattledog!

I'm still unsure how to implement some kind of system to tell the Arduino to completely ignore any zero data that is entered, which is what you suggested a few posts ago.

I will have a good, long think about it.

Well, I've rambled on quite enough for now, thanks again @cattledog, your help and guidance is very much appreciated!