allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

[wpilib] Fix repeat TimedRobot callbacks on loop overrun

Open calcmogul opened this issue 2 years ago • 6 comments

If one of the *Init() functions takes several multiples of the nominal loop time, the callbacks after that will run, then increment their expiration time by the nominal loop time. Since the new expiration time is still in the past, this will cause the callback to get repeatedly run in quick succession until its expiration time catches up with the current time.

This change keeps incrementing the expiration time until it's in the future, which will avoid repeated runs. This doesn't delay other callbacks, so they'll get a chance to run once before their expiration times are corrected.

The other option is correcting all the expiration times at once, which would starve the other callbacks even longer so that the callback scheduling returns to a regular cadence sooner. The problem with this approach is if a previous callback keeps overrunning the start of the next callback, the next callback could potentially never get a chance to run.

calcmogul avatar Mar 15 '22 01:03 calcmogul

Thanks to @gerth2 for isolating the issue with https://github.com/gerth2/FastLoopMRE/blob/master/src/main/java/frc/robot/Robot.java.

calcmogul avatar Mar 15 '22 01:03 calcmogul

FWIW.... from a tasking perspective, this is a fairly major change in functionality to TimedRobot. Might be the fear-mongerer in me... but I'd suspect this could break someone in strange and subtle ways. Since it's patchable by users who care, I'd say this should wait to be merged till 2023.

gerth2 avatar Mar 15 '22 02:03 gerth2

Do we want to add unit tests for this functionality? Do we have sufficient tests that ensure it doesn't break existing behavior?

Starlight220 avatar May 31 '22 11:05 Starlight220

The fact CI is failing shows that it already broke existing behavior. This is likely due to nondeterminism in sim Notifiers.

calcmogul avatar May 31 '22 15:05 calcmogul

The initial robot loop is basically guaranteed to hit this as well.

ThadHouse avatar Dec 14 '22 07:12 ThadHouse

This is blocked on fixing simulation notifier race conditions.

calcmogul avatar Dec 07 '23 05:12 calcmogul