Fix NodeStatus flooding and GUI responsiveness issues - #88
Conversation
Bwooce
changed the title
f384a34 to
383ce8e
Compare
Bwooce
changed the title
|
Apologies for the churn here, on further testing there were...more problems. I've attempted to keep this PR seperate from the ACK/NACK change one to facilitate testing/validation that it's not broken the world before the ACK/NACK change gets merged. |
Bwooce
changed the title
a1794f7 to
d9acd2e
Compare
When SLCAN ACK/NACK waits were removed to support adapters without confirmation, the scheduler's timing assumptions broke. The periodic scheduler would re-register the next NodeStatus event before the current callback completed, causing a runaway loop when SLCAN frame transmission included time.sleep() calls. This fix prevents callback overlap by: - Adding callback_running flag to track execution state - Skipping overlapping executions and rescheduling for next period - Using try/finally to ensure flag is always reset Resolves 6000+ NodeStatus messages/second flooding issue while maintaining proper 1Hz timing and DroneCAN compliance.
- Fix periodic scheduler callback overlap preventing NodeStatus flooding - Add GUI responsiveness protection by limiting frames per spin cycle - Improve transfer reassembly with proactive cleanup and shorter timeouts - Fix dictionary iteration bug in transfer cleanup These changes resolve NodeStatus flooding and GUI freezing issues while improving frame decode reliability.
d9acd2e to
7011c14
Compare
|
@Bwooce can you describe how I can reproduce the issue? |
fallenmi
left a comment
fallenmi
left a comment
There was a problem hiding this comment.
The new callback_running guard does not stop the overdue callback loop described in this PR. sched.scheduler.run(blocking=False) executes due actions serially: this code registers the next absolute deadline before callback(), but finally clears the flag before the scheduler examines that event. When a callback takes at least one period, the next event is already overdue, sees callback_running == False, and runs immediately. That repeats within the same poll, so spin(0) can still remain in _poll_scheduler_and_get_next_deadline() and publish NodeStatus back-to-back.
I reproduced this without wall-clock timing using the repository's real Scheduler and a deterministic fake clock: period 1.0 seconds, callback duration 1.1 seconds, and a sentinel on the fifth call. One non-blocking poll invokes callbacks at [1.0, 2.1, 3.2, 4.3, 5.4] before reaching the sentinel on exact base 37f416fa54e61c9f4832ebe614c257fe66072225, exact head 7011c144d9eea60da1564c23a7f16d287c25bca4, and current GitHub merge 97e90c06f1d9c4748d94dbff4385a5c9ae522f73. The head therefore leaves the stated failure unchanged. Please make an overrun advance the queued deadline into the future (or otherwise skip missed ticks) and add a regression asserting that one scheduler poll returns after a slow periodic callback.
The full repository suite passes 58/58 on exact base, head, and current merge, but no existing test exercises periodic scheduler overruns. git diff --check also reports five trailing-whitespace errors in the submitted diff.
AI disclosure: I used OpenAI Codex to inspect the exact revisions, build and run the deterministic scheduler-overrun oracle, run the repository suites, and draft this review. I verified the commit IDs, fake-clock trace, source paths, test output, live merge parents, and conclusion.
3 participants
Summary
Technical Details