speeduino icon indicating copy to clipboard operation
speeduino copied to clipboard

Stall reset changes

Open DeionSi opened this issue 2 years ago • 3 comments

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.

DeionSi avatar Jul 29 '22 16:07 DeionSi

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.

noisymime avatar Aug 16 '22 00:08 noisymime

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.

DeionSi avatar Aug 25 '22 20:08 DeionSi

The comment isn't entirely complete there. The issue that would be faced is when:

  1. toothLastToothTime gets set using micros() just after approx. 1 hour, 11 minutes, 34 seconds
  2. micros() rolls over
  3. If the next mainloop happens before the next tooth, currentLoopTime will be a very small number and toothLastToothTime will be very large. This is not handled currently in isDecoderStalled() and will cause a very large and incorrect value for timeToLastTooth

noisymime avatar Aug 25 '22 23:08 noisymime

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:

  1. Let's assume toothLastToothTime is ULONG_MAX-1 (4 294 967 294)
  2. 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.

DeionSi avatar Jan 03 '23 18:01 DeionSi