Repository navigation
fix(display): a WiFi notice preempts the current screen and Vegas yields to it - #712
Conversation
…lds to it Two behaviours #704's golden traces recorded (docs/RUN_LOOP_REDESIGN.md, "behaviour the traces pin down that may be wrong", items 1 and 2): 1. A WiFi notice was only checked at the top of a pass, i.e. between screens. A 5 s notice posted during a 20 s screen expired before the screen ended and was never shown. 2. Vegas's interrupt callback yielded for a notice, but run() then fell through to a rotation screen in the same pass, and the notice expired behind it. Fix: a new _wifi_notice_pending() (on-demand still outranks it; the file stat stays throttled to 1 Hz) is polled where run() already checks for preemption: both frame loops, the post-loop check (only when the screen was cut short, so a screen that ran its full time still advances), the make-up dwell, and _sleep_with_plugin_updates (edge-triggered, so the notice's own 0.5 s dwell does not cut itself short, and not while scheduled off). After Vegas yields with a notice pending, the pass ends so the next pass shows it. A screen the notice interrupts resumes in full afterwards rather than being rotated past. Goldens regenerated: wifi_notice and vegas only. The harness labels a screen followed by a notice with exit reason "wifi". wifi_notice: - [20, weather, 5, wifi]: the notice posted at 25.0 cuts weather short. - [25, <wifi>, 5]: shown for its full 5 s (posted 25, expires 30). - [30, weather, 20]: the interrupted mode comes back in full. - [50, clock, 20, on-demand-start]: the rotation is 10 s later; the on-demand request at 60 is for clock, the mode already showing, so the screen is not cut (unchanged behaviour for a same-mode request). - [70, clock, 10, on-demand-expired]: the on-demand screen, ended by expiry at 80 as before. - [95, clock ...], [115, weather ...], [135, clock ...]: rotation resumes at the index saved when on-demand started (clock, mid-screen at 60), where before clock's screen had ended at exactly 60 and advanced first. The second notice ("AP mode on", held back by on-demand) is unchanged. vegas: - [200.222, <wifi>, 3]: Vegas yields at 200.222 and the notice shows next instead of a 20 s clock screen. - [203.222, <vegas> ...], [233.23, <vegas> ...]: Vegas resumes once the notice ends; the later rows shift accordingly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWiFi notices now interrupt active screens and dwell periods. After a notice, the controller resumes the interrupted mode when applicable. On-demand sessions retain control, and Vegas resumes after yielding to a notice. ChangesWiFi notice preemption
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to WiFi notices now interrupt the current screen and then resume it. On-demand sessions keep priority, and Vegas yields to the notice. No merge-blocking risk is evident from the supplied evidence. Hardware testing was not done. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to WiFi activity gains a broader ability to interrupt ordinary display content, but on-demand and follower controls remain in place. No new privilege gain or security-control bypass was established. Deployment access settings and some expiry and recovery behavior remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 4 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
Conflicts with #712's WiFi-notice preemption, resolved to keep both: - _sleep_with_plugin_updates docstring names both early returns. - After an interrupted Vegas iteration the WiFi check comes first (it ends the pass so the next one shows the notice), then the live switch: the documented priority is follower, on-demand, WiFi, live, Vegas. - CHANGELOG keeps both Fixes entries; RUN_LOOP_REDESIGN.md drops items 1-4, leaving the two schedule items. Adds test/test_run_loop_wifi_and_live.py: a notice and a game arriving during the same screen (1 Hz, 125 Hz, Vegas) show the notice first, then the game, and neither while scheduled off. Goldens regenerated: all byte-identical to the merged files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Vegas (#713) A game that goes live takes over within about a second (_check_live_takeover in the frame loops and the dwell sleep, throttled to 1 s, never during on-demand, scheduled-off, live_in_ticker or an already-live screen); an interrupted Vegas iteration switches straight to the game; has_live_content() is asked once per plugin per scan. Goldens: live_priority, vegas. Merged with #712: after an interrupted Vegas iteration the WiFi-notice check runs before the live switch (WiFi outranks live). Adds test/test_run_loop_wifi_and_live.py, pinning that a notice and a game arriving during the same screen (1 Hz, 125 Hz, Vegas) show the notice first, then the game, and neither while scheduled off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CHANGELOG keeps all Fixes entries. RUN_LOOP_REDESIGN.md: with #712 and #713 in, removing items 5 and 6 empties the 'may be wrong' list, so the section now says all six are fixed and by which PR. Goldens regenerated: byte-identical to the merged files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…in off hours blanks at once (#714) Schedule and dim windows are half-open [start, end): on from the start time, off at exactly the end time, whatever second the check runs. An on-demand session that ends in scheduled-off hours (expiry or stop) clears the once-a-minute schedule gate, so the panel blanks within about a second. Golden: schedule; two test_display_pending_changes.py tests now say end_time 23:00. Merged with #712 and #713: with all three in, docs/RUN_LOOP_REDESIGN.md's 'may be wrong' list is empty, so that section now records that all six items are fixed and by which PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Post-merge rig check on hdpi: SKIPPED (not run) The run-loop checks were queued after the #702 soak, on origin/main 41db488, which contains #712, #713 and #714. They never ran. At 20:07 EDT another session on hdpi made these changes:
My guard stopped the batch at that point. Their config was left as they set it, and HEAD was put back to 4ddc3a3. Nothing about this PR was measured. For a re-run:
🤖 Generated with Claude Code |
…otice (#721) The schedule-off blank and the WiFi notice are drawn by the display controller, not dispatched to a plugin, so #716's handover never reached them. Drawn while the last scroll's state was still set, the blank went out with the ticker's lagging rows on a scan-compensated panel and stayed up for its 60 s dwell, and the notice's redraws (which #712 now shows over a running scroller or Vegas) were timed as 0.5-1 s freezes and logged as a mid-scroll Render stall. The controller now calls set_scrolling_state(False) before drawing either; a scroller that resumes sets the state again on its next frame. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ledpi check (Pi 4, 192x48), main 746dcfc: PASS. The notice was triggered by writing
Minor observations:
🤖 Generated with Claude Code |
Fixes two display-loop bugs that #704's golden traces recorded (items 1 and 2 of "Behaviour the traces pin down that may be wrong" in
docs/RUN_LOOP_REDESIGN.md).Root causes
run()checked for a WiFi notice only at the top of a pass (_show_wifi_notice), which means only between screens. The frame loops, the make-up dwell and_sleep_with_plugin_updatesnever looked at it. A 5 s notice posted during a 20 s screen expired before that screen ended._check_vegas_interruptreturned True for a pending notice, sorun_iteration()returned False. The interrupted-Vegas branch then fell through to step 9 (one rotation screen) in the same pass. The notice expired during that screen.Fix (
src/display_controller.py)_wifi_notice_pending(): True when_check_wifi_status_message()has an unexpired notice and no on-demand session is active. On-demand still outranks WiFi, and the priority order is unchanged: follower, on-demand, WiFi, live, Vegas, rotation. The filestatis still throttled to 1 Hz, so polling it at frame rate is just a clock compare. It also rejects a cached result that has expired inside the throttle window.run()already checks for preemption:not loop_completed). A screen that ran its full time still advances, and the notice shows before the next mode._sleep_with_plugin_updatesnow returns early when a notice becomes pending. This is edge-triggered, so the notice's own 0.5 s dwell does not cut itself short. It is skipped while the panel is scheduled off. After a dwell cut short, the pass ends without rotating.timestamp + durationfrom the status file. Because it now preempts within about a second, it is on the panel for (almost) all of that time.No restructuring toward the Arbiter. The diff only touches the WiFi paths.
Golden trace diffs
Only
wifi_noticeandvegaswere regenerated. The other 13 goldens are byte-identical. The harness (reduce_trace) now labels a screen followed by<wifi>with exit reasonwifi. No row in the other goldens is affected.wifi_notice.json[20, weather, 5.0, wifi]: bug 1 fixed. The notice posted at t=25.0 cuts weather short at the 1 Hz loop's next check.[25, <wifi>, 5.0]: the notice shows for its full 5 s (posted at 25, expires at 30). Before, it never appeared.[30, weather, 20.0]: the interrupted mode comes back in full.[50, clock, 20.0, on-demand-start]: the rotation now runs 10 s later. The on-demand request at t=60 is forclock, the mode already on screen, so that screen is not cut. This is unchanged behaviour for a same-mode request. The row's label comes from the request event at 60.[70, clock, 10.0, on-demand-expired]: the on-demand screen, ended by the session's expiry at t=80 as before.[80, <wifi>, 15.0]: unchanged. "AP mode on", posted at 65 during on-demand, waits for the session to end. This shows on-demand still outranking WiFi.[95, clock],[115, weather],[135, clock]: the rotation resumes at the index saved when on-demand started. That is nowclock, interrupted mid-screen at 60. Before, clock's screen had ended at exactly t=60 and advanced to weather before the request was read.vegas.json[200.222, <wifi>, 3.0]: bug 2 fixed. Vegas yields at 200.222 (its interrupt check cadence), and the 3 s notice is what shows next instead of a 20 sclockscreen.[203.222, <vegas>],[233.23, <vegas>]: Vegas resumes once the notice ends. The later rows shift to follow.Tests
test/test_wifi_notice_preemption.pyruns the realrun()on the harness's fake clock:enable_scrolling) screen within 1 sfirst_frame_onlyplugin), and that mode resumesFakeVegas): it yields for the notice,<wifi>shows next for the notice's full duration, then<vegas>resumes, with no rotation screen in between_wifi_notice_pending()ignores an expired cached result and returns False during on-demandMutation check: I removed each of the 8 changes one at a time: the Vegas
continue, each frame-loop check, the post-loop check, thenot loop_completedguard, the dwell break, the post-dwellcontinueand the expiry guard. Every mutation fails at least one test.Verification
pytest test/test_run_loop_golden.py test/test_wifi_notice_preemption.py: 23 passed.plugin-repos/pollution.scripts/check_types.py(mypy 1.8.0, run from a scratch venv): the same 5 errors as origin/main. They come from the venv lacking project dependencies, and none are in the files changed here.Hardware check needed (not done; no rig touched)
On a Pi with a mixed static/scrolling rotation:
The other run-loop fixes (
claude/fix-live-takeover,claude/fix-schedule-boundaries) touch nearby lines inrun()anddocs/RUN_LOOP_REDESIGN.md. Expect small textual conflicts in the frame-loop conditions and the renumbered "exposed behaviour" list.🤖 Generated with Claude Code
Summary by CodeRabbit