speeduino
speeduino copied to clipboard
Stall reset changes
Move decoder stall checks and decoder variable resets to decoders.ino as separate functions. Primary purposes are for clarity and to allow for unit testing.
Also prevents interrupts when stall checking and resetting to prevent interrupt race issues.
Working as expected when bench-testing. 4 bytes reduced heap memory and 72 bytes smaller firmware. Loops/sec unchanged.
You need to keep the || (toothLastToothTime > currentLoopTime)
check in there somewhere. Without it you'll likely get a sync loss and hesitation when micros()
overflows after around 70 minutes.
Thanks for the comment, I hadn't really thought about that. However the isDecoderStalled function should be safe for micros() overflow. See https://www.gammon.com.au/millis
I believe (toothLastToothTime > currentLoopTime)
was for possible interrupts firing between getting the time and comparing as per the comment toothLastToothTime can be greater than currentLoopTime if a pulse occurs between getting the latest time and doing the comparison
. Since interrupts are now disabled during the check it is no longer a possibility.
The comment isn't entirely complete there. The issue that would be faced is when:
-
toothLastToothTime
gets set usingmicros()
just after approx. 1 hour, 11 minutes, 34 seconds -
micros()
rolls over - If the next mainloop happens before the next tooth,
currentLoopTime
will be a very small number andtoothLastToothTime
will be very large. This is not handled currently inisDecoderStalled()
and will cause a very large and incorrect value fortimeToLastTooth
I've added tests for isDecoderStalled. To be able to test I've added a "test injection" definition for micros_safe and micros to allow testing with different values. isDecoderStalled is also moved to header file to allow inline usage from test source files.
This pull request now has #976 as a prerequisite.
In response to your comment:
- Let's assume toothLastToothTime is ULONG_MAX-1 (4 294 967 294)
- micros() rolls over and is now 4
This code
uint32_t timeToLastTooth = (MICROS_SAFE_OR_TEST_INJECTION - toothLastToothTime);
if ( timeToLastTooth > MAX_STALL_TIME ) {
Would calculate as
uint32_t 6 = (4 - 4 294 967 294);
if ( 6 > 50000 ) {
As an uint32_t cannot be negative -4294967290 it will "underflow" all the way around to 6.
The tests test for this condition.