Make sure the screen-off command reaches the display before exiting - #1082
Open
boscocp wants to merge 1 commit into
Open
Make sure the screen-off command reaches the display before exiting#1082boscocp wants to merge 1 commit into
boscocp wants to merge 1 commit into
Conversation
Exiting from the tray icon could leave the panel lit: the screen-off command was cut in the middle of being sent, or was followed by one last sensor frame. `is_queue_empty()` checked `update_queue.empty()`, which becomes true the moment `QueueHandler` takes a request out of the queue, not when the write to the serial port completes. `wait_for_empty_queue()` in `clean_stop()` returned too early and `os._exit(0)` interrupted the transfer. `periodic()` also ran `action()` one last time after `STOPPING` was set: the `if not STOPPING` only skipped the re-scheduling. Sensor threads therefore queued a frame behind the turn-off requests `clean_stop()` had just queued. - `is_queue_empty()` now uses the queue count of unfinished tasks, which only drops back to zero after `task_done()`, i.e. once the request has really been processed - `periodic()` no longer runs the action while the program is stopping - `QueueHandler` becomes its own loop with `get(timeout=)`: the scheduled job blocked in an untimed `get()` and never noticed `STOPPING`. An exception raised by a request is now logged instead of killing the only consumer of the queue - `clean_stop()` waits for the queue handler thread before closing the port: a late write would fail on the closed port and `WriteLine()` would reopen it - new `Display.close()`, since `os._exit(0)` skips the driver destructors - `LcdSimulated` no longer raises `AttributeError` on close when its web server did not start because the port was already in use, a path only reached now Reproducing the real `clean_stop()` order with a slow simulated write: before: the wait returns while the last command is still being sent (truncated) after: the turn-off commands are fully sent before exiting And with a real scheduled job, counting the extra frame queued after `STOPPING`: before: 1 after: 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Exiting the system monitor could cut the screen-off command mid-transfer, or let a sensor
frame reach the panel after it. Two separate causes:
is_queue_empty()returnedupdate_queue.empty(), which becomes true the momentQueueHandlertakes a request out of the queue — not when the write to the serial portcompletes. So
wait_for_empty_queue()inclean_stop()returned early andos._exit(0)cut the screen-off command mid-transfer.
periodic()ranaction()one last time afterSTOPPINGwas set (theif not STOPPINGonly skipped re-scheduling), so a sensor thread could queue a frame behind the turn-off
requests
clean_stop()had just queued.Changes
is_queue_empty()uses the queue's count of unfinished tasks, which only drops to zeroafter
task_done()— i.e. once the request has actually been processed.periodic()no longer runs the action while the program is stopping.QueueHandlerbecomes its own loop withget(timeout=). The scheduled job blocked in anuntimed
get()and never noticedSTOPPING. An exception raised by a request is now loggedinstead of killing the queue's only consumer.
clean_stop()waits for the queue handler thread before closing the port: a late writewould fail on the closed port, and
WriteLine()reopens the port when that happens.Display.close(), sinceos._exit(0)skips the driver destructors.LcdSimulatedno longer raisesAttributeErroron close when its web server did not start(port already in use) — a path only reached now that close actually runs.
Measurements
STOPPING(real scheduled job)How wide the truncation window is depends on what is last in the queue. On an idle exit on
rev A only
SCREEN_OFFis queued —SetBackplateLedColoris a no-op there (lcd_comm.py:234,overridden only by rev B and the simulated display) — so the window is very small; it is much
wider whenever the queue still holds image data.
Dropping
@schedule(timedelta(milliseconds=1))also removes a real cost.sched.enter(0.001)slept ~1 ms per queued chunk by construction, and a full-screen 320×480 RGB565 image is
120 chunks (
lcd_comm_rev_a.py:218,chunked(rgb565le, width * 8)).time.sleep(0.001)measures 1262 µs on macOS and 1505 µs on Windows, so that is 151 ms and 181 ms of pure
sleep per full-screen image. On a Turing 3.5" rev A the startup image flushes in about 2.5 s,
so the sleep alone was roughly 7% of the transfer time.
On hardware
Tested on a Turing 3.5" (rev A) on Windows, 5 runs per branch, exiting from the tray icon while
the startup image was still being flushed.
On
main, the run that exited with the most data still queued hit the full 5 s wait budget. Thesensor jobs started after
STOPPINGhad been set and queued a frame each behind the turn-offrequest, so the queue never drained and the exit truncated it:
The runs with this change that also exited with data still queued drained in 1.5-2.5 s and the
wait returned normally. One
mainrun that exited with the queue nearly drained behaved the sameas the equivalent run with this change (0.3 s vs 0.4 s), as expected: the failure needs data still
queued behind the turn-off request.
The panel itself went dark in all 10 runs, so the symptom reported in #907 did not reproduce here.
This PR fixes the mechanism above; it is not confirmed to fix that issue.
Notes
is_queue_empty()readsQueue.unfinished_tasks. Only its behaviour is documented (viaQueue.join()), not theattribute name, so this relies on a CPython implementation detail. The public alternative is
Queue.join(), which offers no timeout — both callers here need a bounded wait. Happy toswitch to a lock-protected counter if you prefer.
try/exceptinDisplay.close()is load-bearing, not defensive:LcdSimulated.closeSerial()raised
AttributeErrorwhen the web server never started. That is fixed at the source in thisPR, but the guard stays so closing can never block the exit.
LcdCommTuringUSBnever setslcd_serial, soDisplay.close()is a no-op forTUR_USB, andthat revision never used the update queue either. Neither half of this PR changes its behaviour.