fix: keep the metronome off the clock pubsub - #638
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
15ed983 to
9f25525
Compare
ArthurGibert
left a comment
There was a problem hiding this comment.
Two questions before reviewing the fix itself.
Is this tied to an observed bug, or a code-reading finding? By our math, filling the shared 16-slot CLOCK_PUBSUB queue within one metronome flash (25ms, once per quarter note) needs ~1600 BPM of tick throughput — well above the 300 BPM internal clock ceiling (global_config.rs:127) and default external-clock debouncing. Not seeing how normal use reaches this, but might be missing something — got a repro?
Also, can 9f255253 (clock_ticker) split into its own PR? It only touches app.rs, unrelated to the pubsub/metronome fix.
|
Written by an AI coding agent on @kosmar's behalf. Good questions — the math is right for the steady state, it's the burst path that breaks it. On the repro. This came out of chasing real stalls on the playground WIP build, not from reading alone: with a crowded clock cable (dense layout, external clock) the device would go completely silent while USB and the config port stayed up. Honest caveat: some of those apps were themselves buggy at the time, so I can't pin every stall on this specific mechanism — it may have been the WIP apps, this, or both. And there is no minimal, deterministic repro on plain The 1600 BPM figure assumes ticks arrive evenly spaced. They don't on the external path. When an early pulse flushes unfired interpolated ticks, Second, the metronome isn't the only way the queue reaches full — it's just the one subscriber that is guaranteed present in every layout and provably stops draining. Any app subscriber that awaits a MAX write or a MIDI send inside its clock loop can push it over too. Once the queue is full, the gatekeeper's blocking On the split: yes, will do. |
9f25525 to
9ddb21f
Compare
The scene-LED metronome subscribes to
CLOCK_PUBSUBand thenawaits a 25 mstimer inside its message loop. While that timer runs it still holds its
subscriber slot but drains nothing, so the shared clock queue fills behind it.
Once it is full the gatekeeper’s
publish().awaiton the tick path blocks, andwith the gatekeeper stalled the whole device clock stops — every clocked app
goes silent at once, while USB and the config port stay alive, so the device
looks healthy.
The metronome only ever needed the tick number. This mirrors the gatekeeper’s
counter into a
TICK_COUNTERatomic and has the metronome poll it on a 1 mstimer, tracking its own LED-high countdown instead of sleeping in the event
loop. It no longer touches the pubsub at all, which also frees a subscriber
slot (
CLOCK_PUBSUB_SUBSCRIBERS17 → 16).Behaviour is unchanged: the LED still flashes on the first tick of each quarter
note, and a Start/Reset (counter set to
u64::MAX, then ticks from 0) stillretriggers it.
TICK_COUNTERispubbecause it is the natural read path for anything thatonly wants the current tick without owning a queue slot.
This branch is stacked on #636 and contains its commit, because the two changes
meet on the same line: #636 makes the tick path non-blocking, this removes the
subscriber that made it block. The tick line here keeps #636’s
publish_immediateand storesTICK_COUNTERalongside it, so the fix is notundone. Merge #636 first and this diff shrinks to just the metronome and
clock_tickerchanges. #631 touches the same region and may still need atrivial resolution.
Test checklist (hardware)
clock stall
Made with Cursor
The
clock_tickeraccessor that used to sit on top of this branch now lives in #644.