I’m glad you got your buttons fixed. Looks liks snap is best avoided!
Your code is possibly suffering from interrupts. I think. The data types you are holding your durations in are
long but the ATmega328 is
byte sized and doesn’t have 16 or 32 bit instructions as such.
Millis() turns interrupts off to update its
long variables, as it is done within the ISR for Timer/counter 0’s Overflow Interrupt. That allows the update to work correctly.
Your updates to the duration generate code to update 4 bytes from a value that is 4 bytes long. Between each byte, in interrupt can occur, changing the values in the bytes not yet copied over to your variables.
noInterrupts(); before, and
interrupts(); after, your assignments from
millis() to your duration variables.
I think that will solve those problems.
You are aware that
millis() will roll over at some point? That will affect things too. It leads to long periods of darnkess or light! (Rollover happens every 2^32 + 1 milliseconds.)
sorry, I was wrong! Don’t bother. While I wa correct that interrupts could be a problem, they are not because whenever you call
millis() interrupts are off while the 4 bytes are transferred:
unsigned long millis()
unsigned long m;
uint8_t oldSREG = SREG;
37c: 2f b7 in r18, 0x3f ; 63
// disable interrupts while we read timer0_millis or we might get an
// inconsistent value (e.g. in the middle of a write to timer0_millis)
37e: f8 94 cli
m = timer0_millis;
380: 80 91 09 01 lds r24, 0x0109 ; 0x800109 <timer0_millis>
384: 90 91 0a 01 lds r25, 0x010A ; 0x80010a <timer0_millis+0x1>
388: a0 91 0b 01 lds r26, 0x010B ; 0x80010b <timer0_millis+0x2>
38c: b0 91 0c 01 lds r27, 0x010C ; 0x80010c <timer0_millis+0x3>
SREG = oldSREG;
That’s the assembly code generated for the code in the
millis() function which fetches the value of
timer_0_millis into a variable named
m ready to be returned. You can see a
cli instruction above the fetching of 4 bytes in 4 separate instructions. The
SREG = oldSREG is where the interrupts are enabled again - assuming they were before they were turned off.
False alarm, sorry!
The problem looks to be in your code. There is an example of using
millis() rather than
delay() on the Arduino web at https://www.arduino.cc/en/tutorial/BlinkWithoutDelay which does work. Maybe you can study that code and see what’s different from yours?
You are pulling the
millis() value twice, well 4 times, in
loop() and each one will be very different. The value increments 1,000 times every second, so even a short time between getting the value and using it will cause it to be different, then when you assign the value to one of the duration variable, it will have moved on again.
You need to grab
millis() when you enter the loop, every time. You should, I think, also be holding the previous time that you toggled the LED. You can then check that millis (now) - millis(then) > duration to determine if it’s time that the LED was toggled. That’s what the Arduino example does.