refactor(display): run loop stage 3 -- ScreenRunner, PREEMPTED, OnDemand/Live/Rotation Sources - #762
Merged
Merged
Conversation
…tage 3) The two frame loops, the make-up dwell and the dynamic-duration exit move out of DisplayController.run() into src/screen_runner.py. ScreenRunner paces with an injected FrameClock (production: this module's time, looked up per call so the golden harness's fake clock still drives it) and returns one Outcome whose ExitReason is DURATION, CYCLE_COMPLETE, EMPTY, ERROR, DISPLAY_FALSE, RELOAD or PREEMPTED. PREEMPTED replaces the re-checks that used to follow each frame loop and the make-up dwell (current_display_mode != active_mode, the schedule, a pending WiFi notice): the runner asks its host at named service points (FRAME, AFTER_LOOP, after_dwell, FINAL), and on PREEMPTED run() goes to the next pass without advancing the rotation, as each `continue` did. RELOAD is the one early end that still advances, as a reload always did. Each service point reads the WiFi notice file exactly when the loop did (NoticeRead), because the read is throttled and deletes expired files. The host answers still use the old checks; the following commits move them to the Arbiter. _screen_preempted is gone (folded into the FRAME check); _wait_frame_interval returns the preempting plan instead of a bool. The frame pacing (8 ms deadline, 1 ms minimum yield, 1 Hz wait with socket wake) is the same code, moved. Golden traces unchanged. A capture of every harness run (all 67, with every sleep, display() call, wifi read, live scan, publish and dwell logged) is identical to origin/main except for throttled WiFi reads that returned the cached answer (no side effect) after a notice preempted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Arbiter.decide() now answers for an active on-demand session itself (Source.ON_DEMAND) instead of returning LEGACY: - ArbiterState gains the session: its mode list, index, expiry and pin, plus current_mode, snapshotted from the controller's fields by _arbiter_state(). The controller's attributes stay the record that the web UI, the control socket and the cache read. - The OnDemand plan is the session's current mode (an index past the end of a shortened list starts again at 0), with what is left of a timed session at `now` as max_duration and the expiry as deadline. A session with no modes left is a plan with no mode; the controller ends it and shows the rotation's mode, as _resolve_active_mode did. - on_demand_bound() is _clamp_to_on_demand made pure. It is still applied after the first frame, with the clock read there. - ArbiterState.next_on_demand() is the step _advance_on_demand takes. run() asks decide() for the screen at the point it used to call _resolve_active_mode (after any Vegas iteration, so a session that started mid-iteration still shows next), and _take_plan() applies it. Golden traces and the 67-run capture identical to origin/main. Adds TestOnDemand and the bound table to test_display_arbiter.py; the stage-2 table's on-demand rows now name ON_DEMAND instead of LEGACY. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The live-priority step of run() (step 7) and the live checks around the Vegas iteration become the Arbiter's Live Source: - ArbiterInputs gains live_modes (the scan, None where run() made none), vegas_enabled, vegas_live_in_ticker and vegas_yielded. ArbiterState gains the rotation and its index, the live resume point and the "takeover not shown yet" flag. - Live picks the next live mode round-robin (live_pick, now also what _check_live_priority returns), not advancing past a mid-screen takeover that has not shown. It outranks Vegas unless the ticker keeps live content, in which case it has no say at all, as before. With nothing live, a plan below it carries ends_live and the interrupted rotation resumes. - ArbiterState.claim_live/release_live are _apply_live_priority's bookkeeping made pure; _apply_live_priority applies them. run() reads the inputs below the WiFi notice where it always did (_arbiter_inputs_below_wifi: the Vegas check, then the scan), asks decide() once more, and _take_plan applies the claim or the resume. The Vegas iteration moves to _run_vegas_iteration, which re-decides with vegas_yielded after a yield, so a game that stopped the ticker or an on-demand session that started mid-iteration still shows next. LEGACY now means Vegas or the rotation. One redundant call is gone: a Vegas pass scanned the live plugins twice at the same instant (step 7, then step 8's "is anything live?"); it scans once. Golden traces unchanged. The 67-run capture is identical to origin/main once that duplicate scan and _apply_live_priority(None) calls that changed nothing are left out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gas (run loop stage 3) decide() now names the screen for every pass: the Rotation Source (Source.ROTATION) answers with the rotation's current mode, after the resume when live priority just ended. LEGACY is left meaning only Vegas, whose iteration is still run()'s own code until stage 4. Once this pass's iteration has yielded (vegas_yielded), Vegas passes and the screen it fell through to is decided like any other. The rotation's mode is state.current_mode rather than rotation[rotation_index]: they agree except where something moved the panel off the list and the rotation carries on from there (a live mode no entry names, or None after a session ended with nothing to resume to), and run() always showed current_display_mode. ArbiterState.after(outcome) is _advance_after_screen's step: an on-demand session moves to its next mode; otherwise the rotation advances unless the mode just shown is still live. The Outcome carries the two facts only the controller can see at the end of the screen (on_demand_active, the live hold from _still_live). Ending a session with no modes left stays in the controller, because it is not pure. Golden traces unchanged; the 67-run capture is identical to origin/main with the same two exclusions as the previous commit. Adds the Vegas / Rotation table and TestAfter; the stage-2 rows that said LEGACY for "live, Vegas or rotation" now say ROTATION. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…points (run loop stage 3) The three mid-screen checks the frame loops made one after another -- _check_live_takeover, then _screen_preempted with _wifi_notice_pending in it -- become one call: Arbiter.decide(state, inputs, now, running=plan). It returns `running` itself while the screen holds, else the plan that ends it, from these rules in the order the loops checked them: 1. Live: a game went live while a non-live screen runs. First because it is the one preemption that changes the state (the rotation moves to the live mode and remembers where it was), and it is still claimed when a WiFi notice is pending too; the next pass shows the notice, then the game, as before. 2. The panel's mode moved under the screen (on-demand started, ended or changed; the rotation was rebuilt). 3. The schedule turned the panel off. 4. A WiFi notice arrived (on-demand outranks it; compared with expiry). 5. A plugin reload is waiting (between frames only). Each rule is gated by plan.preemptible_by: every screen may be preempted by the gate, OnDemand, Wifi, Live, Rotation and a reload, except that a live screen leaves Live out. A follower and Vegas never preempt mid-screen. The pure helper live_takeover() is the Live rule, shared with the dwell sleep's _check_live_takeover. The controller only gathers and applies. _screen_service applies pending changes and makes the live scan when one is due (_scan_for_takeover: the same throttle and gates as before); _screen_check reads the WiFi notice exactly where the loop did (the read is throttled and deletes an expired file, so an extra read would move both), calls decide() once, and claims a live takeover. _screen_preempted is gone; _check_live_takeover and _wifi_notice_pending remain for the dwell sleep and the Vegas yield path, built on the same rules. Golden traces unchanged. The 67-run capture is identical to origin/main (with the earlier two exclusions) except for one event: in the 125 Hz loop the live scan still runs before the frame's sleep, but the claim is now made by the service point after it, so the "live" state change is logged 8 ms later (test_live_game_cuts_a_scrolling_screen_short: 8.064 -> 8.072). The screen still ends at the same frame (8.072) and every frame, sleep and pass is unchanged. Adds the mid-screen table (24 rows), live_takeover's table, and test/test_screen_runner.py (the runner on a scripted host, plus the controller's service point: which reads it makes at which checkpoint). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs/RUN_LOOP_REDESIGN.md describes run() as it now is (two decide() calls per pass, the runner and its service points, the state snapshot and its transitions), records what stage 3 shipped and how it was checked, and adds one open "may be wrong" behaviour the mutation run surfaced: a Vegas iteration stopped for a sync follower falls through to a full rotation screen before the follower gets the panel (pinned by test_vegas_yielding_to_a_follower_shows_a_rotation_screen_first; passes on origin/main too). docs/IPC_CONTROL_SOCKET.md no longer names _screen_preempted. CHANGELOG entry under Unreleased. A mutation run broke 46 moved or new pieces once each (OnDemand, Live, Vegas/Rotation, after(), each mid-screen rule, the runner's pacing, exits and service points, the controller's gathering and claims). Three survived and get a test here: - the after-loop service point not reading the WiFi notice: the completed-loop checkpoint gets its own name, and a run-loop test has a notice pending when a later frame comes back empty; - the Vegas yield path not marking vegas_yielded: the follower test above; - _take_plan not writing back a reset on-demand index: a controller test with an index past a shortened list. All 46 now fail at least one test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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 configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe display loop now selects explicit Arbiter plans and uses ScreenRunner to pace screens and report outcomes. DisplayController connects plan selection, screen execution, preemption checks, and state advancement. ChangesDisplay loop
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant DisplayController
participant Arbiter
participant ScreenRunner
participant ScreenHost
DisplayController->>Arbiter: decide from controller state and inputs
Arbiter-->>DisplayController: return screen plan
DisplayController->>ScreenRunner: run plan and plugin
ScreenRunner->>ScreenHost: complete plan and run screen callbacks
ScreenHost->>Arbiter: check running plan for preemption
Arbiter-->>ScreenHost: return preempting plan or retain running plan
ScreenRunner-->>DisplayController: return outcome
DisplayController->>DisplayController: advance state from plan and outcome
Merge Risk: 🔵 Low · up to The display loop now runs screens through a new runner and chooses screens through the Arbiter, and no other behavior change is intended. No concrete defect was found. The author has asked that the PR wait for an on-hardware frame soak and for the full test-suite comparison to finish, so merge after those checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The examined access controls and state-ownership safeguards remain in place. A suspected reload regression was also present before this refactor. Risk remains low rather than minimal because deployment exposure and device-level validation are not fully established. 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 37.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 193 functions across 7 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
|
| Metric | Results |
|---|---|
| Complexity | 126 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
1 participant
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.
Stage 3 of
docs/RUN_LOOP_REDESIGN.md: a ScreenRunner runs each screen, and the Arbiter now decides every screen except Vegas. No behaviour change is intended. Do not merge yet: this moves the frame loops, so it needs the ledpi frame soak below first.Design
src/screen_runner.py(new, on the mypy ratchet).ScreenRunner(clock, host).run(plan, plugin) -> Outcome.plan.frame_policypicks, makes up the minimum duration, and handles the dynamic-duration exit. All of this moved out ofrun()unchanged, including the 8 ms deadline pacing, the 1 ms minimum yield, and the 1 Hz wait with socket wake.FrameClock:time,perf_counter,sleep). In production it is_ModuleClock, which looks upsrc.display_controller.timeon each call, so the harness's fake clock still drives it.ExitReasonis one ofDURATION,CYCLE_COMPLETE,EMPTY,ERROR,DISPLAY_FALSE,RELOADorPREEMPTED.PREEMPTEDreplaces the five "did the mode change under this screen" re-checks. OnPREEMPTED,run()goes to the next pass without advancing.RELOADis the one early end that still advances, as a pending plugin reload always did.ScreenHostthrough_ScreenHost, which is one-line forwards to its own methods.Sources.
src/display_arbiter.pygainsON_DEMAND,LIVEandROTATION.LEGACYnow means only Vegas.ArbiterStategains:ArbiterInputsgains:live_modes(None where no scan was made)vegas_enabled,vegas_live_in_tickerandvegas_yieldedreload_pending(mid-screen only)ScreenPlangains:mode,plugin,min_duration,max_duration,dynamic,frame_policyandpreemptible_bydeadline(the on-demand expiry) andends_liveThe plugin's answers (durations, dynamic, frame policy) are filled in after the first frame, where they were always read.
decide()is pure, so it can't ask a plugin.What each Source decides:
max_durationis what is left of the session atnow.on_demand_bound()is_clamp_to_on_demandmade pure, and is still applied after the first frame.live_pick, now also behind_check_live_priority). It outranks Vegas unless the ticker keeps live content. With nothing live, the plan below carriesends_liveand the rotation resumes.state.current_mode. That isavailable_modes[current_mode_index]except where something moved the panel off the list, andrun()always showedcurrent_display_mode.The state transitions are pure methods:
next_on_demand,showing,claim_live,release_liveandafter(outcome).afterreplaces_advance_after_screen's step. The controller's attributes stay the record, because the web UI, the socket and the cache read them._arbiter_state()snapshots them, and_adopt_state()writes a transition back.A pass asks
decide()twice:run()always read them. The scan asks every live-priority plugin, and_is_vegas_mode_activeapplies queued Vegas config, so reading them on a follower or notice pass would change behaviour.A Vegas iteration that yields asks once more, with
vegas_yielded.One
decide()call at the runner's service points.decide(state, inputs, now, running=plan)returnsrunningitself while the screen holds, else the plan that ends it. The rules come in the order the loops checked them:preemptible_bygates each rule. Every screen is preemptible by everything except a follower and Vegas, and a live screen leaves Live out.The service points are
FRAME,AFTER_LOOP/AFTER_COMPLETED_LOOP,after_dwellandFINAL. Each checkpoint says whether a reload counts there and when the WiFi file is read (NoticeRead). The read is throttled and deletes an expired file, so it happens exactly where it did._screen_preempted,_resolve_active_modeand_clamp_to_on_demandare gone._check_live_takeoverand_wifi_notice_pendingremain for the dwell sleep and the Vegas yield path, built on the same rules.Not converted, on purpose.
_sleep_with_plugin_updateskeeps its own break rules:decide()would change behaviour.Commits
Rebased onto #758 ("report a scrolling screen held by its plugin's update()"). Its
report_hold=Trueon the 125 Hz loop's frames is carried by_ScreenHost.draw, which passes it whenplan.frame_policy is HIGH_FPS; a test pins this._run_vegas_iteration.LEGACY= Vegas only) andArbiterState.after.decide()call at the service points.Golden traces
Byte-identical. Nothing was regenerated, and
LEDMATRIX_REGEN_GOLDEN=1produces no diff. That covers all 17 scenarios, including #748'son_demand_named_liveandon_demand_restore_failedand #713'slive_priorityandvegas, plustest_run_loop_wifi_and_live.py.A stricter check ran after every commit. Every
RunLoopHarnessrun in the suite was captured (67 runs: the goldens plus the live-takeover, WiFi+live, socket-wake, plugin-reload, schedule, tick and duration tests). The capture logs every fake-clock sleep,display()call, WiFi file read, live scan,has_live_content()call, publish, dwell and scroll-state call. It was diffed event by event againstorigin/main, and it is identical except:_apply_live_priority(None)calls that changed nothing are not made.test_live_game_cuts_a_scrolling_screen_shortlogsliveat 8.072 instead of 8.064. The screen ends at the same frame, 8.072, and every frame, sleep and pass is unchanged. Keeping the claim before the sleep would have needed a seconddecide()call per frame.Tests
test/test_display_arbiter.pygains:live_pick, the Live table and the claim/release transitionsafterlive_takeovertableThe stage-2 rows that said
LEGACYfor "on-demand" or "live/Vegas/rotation" now nameON_DEMANDandROTATION.test/test_screen_runner.py(new) tests the runner on a scripted host and fake clock:ExitReasonIt also tests the controller's service point: which checkpoint reads the notice, whether a live takeover is claimed before the notice, and that a reload counts only between frames.
Mutation check: 46 mutations, each breaking one moved or new piece once (OnDemand, Live, Vegas/Rotation,
after, each mid-screen rule, the runner's pacing, exits and service points, and the controller's gathering and claims). All 46 fail at least one test. The first run left three survivors, and each now has a test:vegas_yielded_take_plannot writing back a reset on-demand indexThe second survivor turned up a pre-existing oddity, recorded under "may be wrong" in the doc: Vegas stops for a sync follower, but a full rotation screen runs before the follower gets the panel. It is pinned by a test that passes on
maintoo, and is left for its own PR (stage 4 is the natural place).Full suite against an
origin/mainbaseline worktree (caec9f5), both run at the same time on this Windows host. The FAILED/ERROR ids are identical: main 35 failed + 6 errors (8550 passed), branch 35 failed + 6 errors (8686 passed; the extra passes are the new tests). These are the known Windows-only failures.test/test_install_lowmem.pywas excluded from both runs because its shell children hung under load from other sessions' suites; it touches nothing here. CI's Linux core suites (3.11, 3.13) and every other check pass.scripts/check_types.pywith mypy 1.20.2: clean (95 files,src/screen_runner.pyadded tomypy-clean.txt).Soak plan (ledpi, please run before merging)
ledpi is the Pi 4 with the Triple Bonnet and a 2x96x48 panel. Plain
mainis about 0.11-0.16 % late there, and every arm fails the default 0.1 % gate. Judge the branch against alternatedmainarms, not against 0.1 %.git statuson ledpi. Other sessions leavesrc/edits there; save them withgit diff > ~/other-session-edits-<ts>.patchbefore checking out. Re-checkgit log -1before each arm.mainat this PR's base, B = this branch), 20 min each, with the service running. Usepython3 scripts/frame_soak.py --previewvia~/ab/run_arm.sh <label> 1200, on the usual rotation: news ticker, then clock-simple, then football, then baseball, Vegas off.run_iteration(), but its entry, yield handling and the screen after a yield moved.ArbiterInputsand callsdecide(), about 1 µs on the desktop, so maybe 10-20 µs on a Pi 4 against an 8 ms frame. If B's late % is above A's, profile_screen_checkfirst.Follow-ups and overlaps
src/module. Right after this merges, ledmatrix-plugins'scripts/check_min_core_version.pyneedssrc.screen_runner: NoneinMODULE_FIRST_VERSION, or every plugins PR's safety job goes red. It cannot be added before this merges, because the guard checks that aNoneentry exists on coremain.## 3.8.1directly under## Unreleased. This PR's CHANGELOG section sits in the same place, so whichever merges second resolves a one-hunk conflict. Keep this section above## 3.8.1._get_display_durationand the imports ofsrc/display_controller.py, notrun()or the helpers here.git merge-treewith its head is clean; the new import is placed away from the line fix: a Vegas static pause survives a non-numeric display duration; aliased store installs ask for a restart #753 adds.🤖 Generated with Claude Code