Skip to content

fix: keep the metronome off the clock pubsub - #638

Open
kosmar wants to merge 2 commits into
ATOVproject:mainfrom
kosmar:feat/clock-ticker
Open

fix: keep the metronome off the clock pubsub#638
kosmar wants to merge 2 commits into
ATOVproject:mainfrom
kosmar:feat/clock-ticker

Conversation

@kosmar

@kosmar kosmar commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

The scene-LED metronome subscribes to CLOCK_PUBSUB and then awaits a 25 ms
timer 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().await on the tick path blocks, and
with 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_COUNTER atomic and has the metronome poll it on a 1 ms
timer, 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_SUBSCRIBERS 17 → 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) still
retriggers it.

TICK_COUNTER is pub because it is the natural read path for anything that
only 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_immediate and stores TICK_COUNTER alongside it, so the fix is not
undone. Merge #636 first and this diff shrinks to just the metronome and
clock_ticker changes. #631 touches the same region and may still need a
trivial resolution.

Test checklist (hardware)

  • Scene LED still flashes on the beat with the internal clock running
  • LED retriggers correctly after Start and after Reset
  • LED stops flashing shortly after the clock stops
  • External clock (Atom/Meteor/Cube and DIN) still drives the flash
  • Load a dense layout (12+ clocked apps) and run several minutes: no silent
    clock stall

Made with Cursor

The clock_ticker accessor that used to sit on top of this branch now lives in #644.

kosmar and others added 2 commits August 10, 2026 11:08
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@ArthurGibert ArthurGibert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kosmar

kosmar commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

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 main: the WIP branch has several other clock subscribers that make it far easier to hit.

The 1600 BPM figure assumes ticks arrive evenly spaced. They don't on the external path. When an early pulse flushes unfired interpolated ticks, clock.rs re-times the whole batch at CATCHUP_SPACING = 500 µs (up to 47 entries at 1 PPQN). That's 2000 ticks/s, so the 16-slot queue fills in ~8 ms — comfortably inside the metronome's 25 ms flash. The comment on CATCHUP_SPACING already says the spacing exists precisely so the burst “cannot overflow the pubsub backlog of slow subscribers”, which only holds if no subscriber sleeps for 25 ms mid-loop. The metronome does. Low-PPQN Eurorack clocks with a multiplier are the normal case, not an exotic one.

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 publish().await stalls, and with the gatekeeper stalled the whole device clock stops. So this PR and #636 address two halves of the same failure: #636 makes the tick path unable to block, this removes the subscriber that made it block. Neither alone is the full fix.

On the split: yes, will do. clock_ticker is purely additive, dead_code on main until the WIP apps land, and doesn't belong in a clock fix. I'll pull 9f255253 into its own PR and rebase this one so it only touches the metronome and the gatekeeper.

@kosmar

kosmar commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Done: 9f255253 is now its own PR (#644) and this branch is rebased down to the two clock commits — the gatekeeper fix (#636) and the metronome change. Nothing here touches app.rs any more.

Split and pushed by an AI coding agent on @kosmar's behalf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants