I seems to be always asking questions, but I am trying to improve the methods I use in my programming
Timing. I tend to use millis() to time events in my main loop, however someone recently questioned my implementation and I would like advice on why my method is 'wrong' and a better way to achieve the same result.
Is there a library dedicated to simple millis() timing events? I will look.
An example of my method (typed from memory...)
I will declare:
unsigned long PiezoBeep;
Just before the main loop:
PiezoBeep = millis();
**main loop:**
if (millis() - PiezoBeep > 5000) {
Freq = 800;
Duration = 100;
SFX ();
PiezoBeep = millis();
}
That looks OK as far as it goes but without a complete sketch there is no context
One small improvement would be to call millis() only once to get the current value at the start of loop() then use that value when needed rather than calling millis() multiple times
The compiler may well reduce the number of calls to millis() when optimising the code but it is good practice to do it explicitly
For one thing it removes the chance that the value may change between calls thus possibly producing inconsistent results. For another it means that you can use a meaningful name for the value such as currentTime so that you can write code that is easily understood such as
if (currentTime - previousRunTime >= requiredPeriod)
{
previousRunTime = currentTime;
//time to do something
}
I have a situation where I wish to trigger certain events in a timeline using millis();
I used the method I mentioned in my original post and it just does not want to work.
Can someone advise on a better method. I don't know how to adjust that code in post 3 to be relevant to my requirements.
My code is horrific now... all over the place and I am starting again. I'll type out my base idea and you can tell me why it's terrible:
uint8_t Event_1 = 15; // Event 1 trigger point in seconds
uint8_t Event_2 = 25; // Event 2 trigger point in seconds
unsigned long Event_length; // millis timer of entire event
void loop(void)
{
do {} while (digitalRead(Button)); // Wait here for start button
Event_length = millis();
do{
if ((millis() - Event_length) > (Event_1 * 100) { // Event 1 at 15 seconds
// Do something
break;
}
} while ((millis() - Event_length) < (30000);
do{
if ((millis() - Event_length) > (Event_2 * 100) { // Event 2 at 25 seconds
// Do something
break;
}
} while ((millis() - Event_length) < (30000);
}
This is an off the top of my head example, but how I have it laid out. I realise the do loop exit statements are a bit pointless.
My problem is the timing is all way off.
Even if I change the uint8_t statements to unsigned_long, it still doesn't work (what is the uint statement for unsigned long?)
The method you are using is no better than using delay()
FastLED will mess things up a bit, while sending data to the LEDs the millis interrupt is disabled. FastLED attempts to adjust for the lost millis count, but is not very precise.
The test code you posted works properly, but all the timed events are based off Event_length, so Event_1 will occur 1500mS later, then Event_2 will occur (25 - 15) * 100, or 1000mS after that. To get a 2500mS delay between Event_1 and Event_2 you would need to set Event_length to millis() after Event_1 completes.
No that is fine. I am taking into account the fact that they operate in real time based on Event_length.
Event 1 is 15000ms isn't it? (you have 1500ms). So I entirely realise that Event 2 is 10 seconds after Event 1.
The reason for this method is there is other stuff going on within the loops (LED control for instance). So I don't want to stop the process using delay.
Your do...while loop is blocking, so is no different from delay, unless you have some additional code within the do...while that implements your additional processing (and that would have to be in every do...while loop if you wanted it to run contenuously). See UKHeliBob's post for a better method, although that has problem with the two timers running independently of each other.
You may want to look into using a state machine, that may be a more appropriate method.
Sorry... brain is fried. Yes, my milliseconds math is correct in my code, and wrong here... derr.
There are other things within the do loop. The events are configurable at the start, so you can actually specify event 2 to operate before event 1 (if that makes sense).
The reason for this method was so default things can be updated (like the Leds) within that do loop, and certain events can be triggered exactly when required.
These events are very short (relay operations etc).
I have commented out the FASTLED commands and the timing works. I need to control the LEDS another way.
I hope not, or the compiler is way smarter than I think possible, or it would only be able to do in the narrowest of circumstances and still should not.
And it would def be wrong in the case of functions that aren't like millis(). I do not know if the compiler has any special knowledge about millis() that would allow it to do any optimising.
Calls to millis() separated by any code might return different numbers, and might be what is intended - but probably not. Unless it is a test of millis() to see it tick up or something.
So I agree, in a nice free running loop that "takes no time", millis() should be called once, ppl like
// unsigned long currentMillis() = millis(); // Oops, thx @Coding_Badly
unsigned long currentMillis = millis();
At the top. I use now as the variable name, just me doing me.
On the same hand, I usually digitalRead() anything I will need multiple times in the same time slice. That eliminates any problems a bouncing switch or glitchy signal might introduce. And leaves one place only to fix an upside down logic mistake, the kind I never make.
random8() and random16() are functions in the FastLed library if that library is used by the sketch that you found them in
/// Generate an 8-bit random number
LIB8STATIC uint8_t random8()
{
rand16seed = APPLY_FASTLED_RAND16_2053(rand16seed) + FASTLED_RAND16_13849;
// return the sum of the high and low bytes, for better
// mixing and non-sequential correlation
return (uint8_t)(((uint8_t)(rand16seed & 0xFF)) +
((uint8_t)(rand16seed >> 8)));
}
/// Generate a 16 bit random number
LIB8STATIC uint16_t random16()
{
rand16seed = APPLY_FASTLED_RAND16_2053(rand16seed) + FASTLED_RAND16_13849;
return rand16seed;
}
/// Generate an 8-bit random number between 0 and lim
/// @param lim the upper bound for the result
LIB8STATIC uint8_t random8(uint8_t lim)
{
uint8_t r = random8();
r = (r*lim) >> 8;
return r;
}
/// Generate an 8-bit random number in the given range
/// @param min the lower bound for the random number
/// @param lim the upper bound for the random number
LIB8STATIC uint8_t random8(uint8_t min, uint8_t lim)
{
uint8_t delta = lim - min;
uint8_t r = random8(delta) + min;
return r;
}
/// Generate an 16-bit random number between 0 and lim
/// @param lim the upper bound for the result
LIB8STATIC uint16_t random16( uint16_t lim)
{
uint16_t r = random16();
uint32_t p = (uint32_t)lim * (uint32_t)r;
r = p >> 16;
return r;
}
/// Generate an 16-bit random number in the given range
/// @param min the lower bound for the random number
/// @param lim the upper bound for the random number
LIB8STATIC uint16_t random16( uint16_t min, uint16_t lim)
{
uint16_t delta = lim - min;
uint16_t r = random16( delta) + min;
return r;
}
Overflow. Now I know the millis() resets/rolls over something like every 49 days or something.
But, my routine will run for years (I hope), and there is the remote, tiny possibility that someone initiates the routine right at the end of the millis() counter.
When it rolls over to zero, my loop would fail.
So, is it so horrific that I reset the millis()?
It basically sits there waiting for a button press. Why would resetting millis() at the start of the routine, immediately after a button press break the code or be such a forbidden thing?
If you write your code using subtraction to determine whether a timing period has ended and use unsigned variables (see post #9) then millis() rollover will not cause a problem unless the timing period is greater than 49 days. There is no need to reset millis()
