Skip to content

SongList: render visible tracks directly - #2

Open
djdefi wants to merge 1 commit into
fix/mixscreen-single-deck-loadfrom
djdefi-direct-library-rendering
Open

SongList: render visible tracks directly#2
djdefi wants to merge 1 commit into
fix/mixscreen-single-deck-loadfrom
djdefi-direct-library-rendering

Conversation

@djdefi

@djdefi djdefi commented Aug 22, 2026

Copy link
Copy Markdown
Owner

SongList currently allocates a ScrollLayout/LinearLayout plus one
ListItem sprite/object per discovered track before returning. This keeps the
recursive synchronous browse contract from CircuitMess#42, but
stores paths in a bounded PSRAM-backed payload and draws only five visible rows
into the existing screen sprite.

Limits are explicit: 4,096 tracks, 255 bytes per path, and 128 KiB total path
payload. Hitting a content limit keeps the indexed prefix selectable and shows
SD card LIMIT; allocation failure clears partial state and shows
Memory error!. The buffers are compacted after scanning and freed on refresh
or destruction. No persistent index is added here.

Origin trail:

Validation: arduino-cli compile --fqbn cm:esp32:jayd with
JayD-Library#19 passes.
Against c6e611a, flash is 1,762,826 bytes vs. 1,764,870 (-2,044), and static
RAM is 47,152 bytes vs. 47,160 (-8). A Jay-D was detected, but runtime
heap/open-close measurements were not collected because its serial port was
already owned by another session's flashing process; no hardware heap claim is
made.

Follow-up, intentionally out of scope: persistent /.jayd-library.idx
creation/invalidation.

@djdefi
Render SongList rows directly
63bc837 
Replace per-track UI objects with a bounded PSRAM-backed path table and draw only the five visible rows into the existing screen sprite. Preserve recursive AAC discovery, navigation, loading, and SD recovery while making allocation limits explicit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@djdefi djdefi mentioned this pull request Aug 22, 2026
Open
djdefi added a commit that referenced this pull request Aug 22, 2026
@djdefi
fix(coach): boundary-arrival gating, bounded rollback, confirmed cros…
c863d79 
…sfade, manual-override guard, bounded fill

Addresses review issues #2, #3, #4, #5 on PR CircuitMess#44 (kept together: the
fixes share the same tickTransition()/tick() flow and DjAssistController
actuator, and were reviewed as a set).

Issue #2 (HIGH, boundary semantics): resolveBoundary() reports the NEXT
upcoming boundary every tick, so it is true almost always - WAIT_BOUNDARY
was advancing immediately instead of actually waiting. Add
DjAssistBoundaryHint::reached, set only by the new capture-once-then-
compare DjAssistController::resolveWaitBoundaryReached() (latches ONE
target frame the first tick the step is current, then just compares the
live playhead against it every tick after). DjAssistEngine::tick() now
gates WAIT_BOUNDARY strictly on `reached`.

Issue #3 (HIGH, cancel/failure rollback): a terminal cancel/failure left
plan-owned mix/sync/started-deck side effects active. Add a bounded
DjAssistController::tickRollback() state machine (MIX -> SYNC ->
STOP_DECK -> DONE, mirroring the forward plan, one actuator submit/poll
per tick, only undoing steps this plan itself submitted, waiting for
each command's real applied/failed/superseded result) plus the pure
DjAssistBridge::nextRollbackPhase()/evaluateCommandOutcome() helpers it's
built from. Never touches pre-existing user state, and deliberately never
restarts fromDeck once STOP_DECK has run.

Issue #4 (HIGH, crossfade confirmation + manual override): the crossfade
poll declared APPLIED from elapsed-time math alone, discarding the actual
setMix() result, and a manual mix override mid-ramp could be silently
overwritten by the next programmatic tick. The actuator now tracks the
exact final-value setMix() command id and only reports APPLIED once
DjSession confirms it applied (SUPERSEDED triggers exactly one resubmit).
Add DjAssistGuardSnapshot::manualMixOverride, computed by
DjAssistBridge::detectManualMixOverride() (a bounded scan of recent
command results for a non-system SET_MIX submitted after arming) and
checked unconditionally in guardOk() - not skipped once crossfadeSubmitted
like the plain mix-threshold check, closing exactly the gap the review
flagged.

Issue #5 (HIGH, hot-loop SD I/O): refreshCandidateTable()'s fill loop
reused DJ_ASSIST_DEFAULT_SCAN_BUDGET (256), a synchronous multi-record SD
read burst inside DjSession::loop(). Add a separate
DJ_ASSIST_FILL_RECORDS_PER_TICK (1) for the genuine-I/O fill path, leaving
the pure in-memory CPU scoring scan's 256 budget untouched.

Test coverage added: driveUntilCommandVisible()/
testTransitionWaitsForBoundaryStep() updated for `reached` semantics plus
a regression proving hasPhrase/hasDownbeat alone never advances the step;
a manual-mix-override-during-crossfade regression in
testTransitionManualOverride(); new dj_assist_bridge_selfcheck.cpp cases
for nextRollbackPhase() (full sequence + every skip-combination),
evaluateCommandOutcome() (exhaustive status mapping), and
detectManualMixOverride() (watermark boundary, origin/type gating,
rejected-command exclusion, null/empty).

All host self-checks (DjAssistSelfCheck, DjAssistBridgeSelfCheck, plus the
full ctest suite) pass under -Wall -Wextra -Werror + ASan/UBSan.
DjAssistController.cpp/DjSession.cpp remain validated only by the real
firmware build per existing project precedent.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
djdefi added a commit that referenced this pull request Aug 22, 2026
@djdefi
fix(coach): plan-owned rollback, durable command tracking, background…
b822c52 
… fill, bounded boundary tolerance

Fourth-round fixes from focused re-review of eca7930 (4 material issues,
3 HIGH + 1 MEDIUM):

1. HIGH - rollback ownership was inferred from which steps were
   *submitted*, so a target deck already playing/synced before arming
   would have rollback incorrectly stop/release state the transition
   never introduced, and a manual mix override mid-crossfade could be
   silently overwritten by the MIX rollback phase restoring armedMix.
   DjAssistTransitionPlan now captures armedToPlaying/armedToSynced from
   the guard snapshot at arm() time (DjAssistEngine::armTransition).
   DjAssistBridge::nextRollbackPhase() is resignatured to take
   manualMixOccurred/syncOwnedByPlan/startDeckOwnedByPlan instead of raw
   submitted flags: it skips MIX whenever a manual mix occurred (even if
   crossfade was submitted), and skips SYNC/STOP_DECK whenever the
   corresponding step was already true at arm time (an idempotent no-op,
   not a plan-owned mutation). DjAssistController::tickRollback() computes
   ownership fresh every tick from the plan baseline and the durable
   mix-generation counter (see #2).

2. HIGH - recentResults is a bounded/evictable presentation ring, not a
   durable control channel; a burst of results could evict the final
   crossfade command's outcome or a manual-override marker before the
   controller polled it, stranding the transition or missing an override.
   Added DjSession::assistTrackCommand()/assistTrackedStatus(): a single
   controller-owned tracked-command slot, retained until consumed,
   updated from the same command-finish path loop() already uses (mutex-
   protected). Added DjSession::assistNonSystemMixGeneration(): a
   monotonic counter incremented only on non-system SET_MIX apply(),
   captured at arm() time and compared every guard build - replacing the
   removed DjAssistBridge::detectManualMixOverride() ring scan entirely.
   DjAssistSessionActuator (poll/pollCrossfade) and the crossfade
   final-command wait now use the durable tracked status exclusively.

3. HIGH - refreshCandidateTable() synchronously read one metadata record
   per DjSession::loop() tick, including full string/provenance decode,
   directly on the audio/session loop; resolveBoundary()'s phrase lookup
   rescanned up to 128 records every tick whenever no future phrase
   existed (a permanently-terminal track). Candidate-table hydration now
   runs entirely off the session loop: DjAssistController owns a genuine
   background Task (fillTaskTrampoline/fillWorkerStep) that reads one
   record at a time with the real (possibly SD-backed) I/O performed
   outside candidateMutex_, taking the lock only for the brief
   bookkeeping/array-write; staleness is re-checked under the lock both
   before and after the read to survive a concurrent library-generation
   reset. Main-thread scan state (scanCursor_/suggestionCount_) is
   separate from background-owned fill state and is reset only when
   candidateTableReady() reports a new generation. Added
   DjAssistPhraseCacheState/phraseCacheNeedsRescan()/updatePhraseCache()
   to cache a found-or-terminal phrase result per deck, invalidated only
   by an identity change, a different deck, a backward seek, or a
   previously-found boundary being passed - never by forward playback
   alone while terminal, closing the up-to-128-rescans-per-tick gap.

4. MEDIUM - resolveWaitBoundaryReached() fired as soon as
   currentFrame >= target with no tolerance, so a boundary could release
   arbitrarily late relative to the actual audio position. Added
   DjAssistBridge::evaluateBoundaryArrival(), reusing the established
   DJ_QUANTIZE_TOLERANCE_FRAMES policy: REACHED only at/within tolerance
   past the target, MISSED beyond it (never releases late - the caller
   re-captures the next boundary via resolveBoundary() and keeps
   waiting), NOT_YET otherwise.

Also removes DJ_ASSIST_FILL_RECORDS_PER_TICK, a leftover constant from
the old per-tick synchronous fill model that issue #3's background task
made dead code.

Tests: new/updated coverage in dj_assist_selfcheck.cpp (arm() captures
toDeck playing/sync baseline both true and false) and
dj_assist_bridge_selfcheck.cpp (nextRollbackPhase ownership-vs-submitted
distinction and manual-override-skips-mix; evaluateBoundaryArrival
not-yet/reached-at-tolerance/missed-beyond-tolerance;
phraseCacheNeedsRescan/updatePhraseCache empty/found-not-yet-passed/
terminal-not-rescanned/identity-or-deck-change/backward-seek). All 8
host CMake self-check targets (including DjAssistSelfCheck,
DjAssistBridgeSelfCheck) pass under C++11 -Wall -Wextra -Werror +
ASan/UBSan, and tests/wireless_ui_self_test.mjs (43 checks) passes
unchanged.

Firmware validated with isolated cm:esp32@1.8.3 (CircuitOS 1.7.1)
against Library 78a9f3d7e52fd151dc50872812ae2ed1da308af1:
- default: 1,380,014 B flash (38%) / 47,160 B RAM (14%)
- wireless (-DJAYD_ENABLE_WIRELESS): 1,974,606 B flash (54%) / 52,856 B RAM (16%)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
djdefi added a commit that referenced this pull request Aug 23, 2026
@djdefi
fix(coach): relinquish rollback ownership on user divergence, guard s…
4292807 
…ource-deck sync, close candidate-generation race, add real controller integration tests

Round-5 review fixes against 8d387ce (4 issues: 1 HIGH, 3 MEDIUM):

1. guardOk() now relinquishes toDeckStartOwnedByPlan/toDeckSyncOwnedByPlan
   the instant it detects a user divergence on that exact property, before
   failing - a user re-asserting play/sync on the target deck after the
   plan's own step already applied no longer causes rollback to stop/
   release the user's own newer intent. guardOk() is now non-const to
   support this.

2. guardOk() also compares the SOURCE deck's (fromDeck) sync intent
   generation every tick (previously unguarded) - a non-system sync
   command on the outgoing deck is a genuine manual override and now
   aborts the transition instead of silently continuing past it. There is
   no rollback ownership for fromDeck sync (the plan never touches it), so
   this is a plain abort with no additional ownership bookkeeping.

3. Closed the remaining candidate-generation check-then-lock race:
   DjSession::libraryGeneration is now std::atomic<uint32_t> and is bumped
   BEFORE refreshLibraryMetadata()/invalidateLibraryMetadata() mutate the
   reader (still inside metadataMutex for sequencing, not required for the
   lock-free load itself). assistLibraryGeneration() is now a lock-free
   read, safe to call a second time from inside DjAssistController's own
   candidateMutex_ critical section with no lock-ordering/deadlock risk.
   assistTrackEntry() captures its revision under the SAME metadataMutex
   acquisition as the entry read (not a separate probe). fillWorkerStep()'s
   commit path re-checks both the entry's captured revision AND a fresh
   in-lock live-generation reload AND fillCursor_==cursor before writing,
   discarding (not committing) any record whose generation moved in any of
   these windows; candidateTableReady() re-loads the live generation inside
   its own lock immediately before deciding readiness.

4. Added DjAssistSessionPort (src/DjAssist/DjAssistSessionPort.h): the
   Arduino-free interface DjAssistController now depends on instead of a
   concrete DjSession - DjSession implements it unchanged in behavior.
   This lets the real, unmodified DjAssistController.cpp/DjAssistEngine.cpp
   be exercised by a new host test, DjAssistIntegrationSelfCheck
   (tests/dj_assist_integration_selfcheck.cpp), against a fake
   DjAssistSessionPort whose command admission reuses the exact real,
   shared admitAssistCommand()/DjCommandQueue/DjCommandResults/
   DjAssistTrackedCommand/DjAssistIntentGenerations primitives DjSession
   itself uses - not a parallel reimplementation. Seven scenarios, all
   passing under g++ -std=c++11 -Wall -Wextra -Werror -fsanitize=address,undefined:
   full default transition (arm->boundary->tempo-lock->sync->crossfade->
   stop->release->complete), cancel/rollback restoring only plan-owned
   state, user play-divergence relinquishing rollback ownership (issue #1),
   source-deck sync divergence aborting the transition (issue #2), manual
   mix mid-crossfade aborting with rollback skipping the mix phase,
   queue-wide supersede/removal through the real admission coordinator, and
   the candidate-fill generation race (issue #3) being caught and
   discarded. Host stubs (tests/host_stubs/{Arduino.h,Sync/Mutex.h,
   Util/Task.h,host_arduino_shim.cpp}) provide the minimal Arduino/
   CircuitOS surface DjAssistController.cpp needs (micros/delay/ps_malloc,
   Mutex, Task) without pulling in real ESP32 headers.

CMakeLists.txt: added the DjAssistIntegrationSelfCheck target (same
-Wall -Wextra -Werror + conditional ASan/UBSan treatment as the other
DjAssist self-checks), building the real DjAssistController.cpp/
DjAssistEngine.cpp/DjAssistScoring.cpp/DjAssistSessionBridge.cpp against
the new test and host stubs (BEFORE on the include path so the stubs win
over any real CircuitOS headers on a fully-configured machine).

Preserves all prior approved commits unamended (boundary tolerance,
4 KiB response bound, durable tracked-slot routing, admission-time intent
generations, queue-wide origin-priority scan, LOCK_TEMPO/ENABLE_SYNC
ownership unification, phrase cache metadata-revision keying).

Testing:
- DjAssistSelfCheck, DjAssistBridgeSelfCheck, DjSessionSelfCheck,
  DjAssistIntegrationSelfCheck, LibraryIndexSelfCheck, DjBeatEngineSelfCheck,
  MetadataIntegrationSelfCheck, WirelessApiSelfCheck: all pass via CMake +
  ctest under ASan/UBSan.
- Full isolated arduino-cli build against JayD-Library 78a9f3d
  (cm:esp32@1.8.3 / CircuitOS 1.7.1):
    default:  1,384,114 B flash / 47,160 B RAM (round-4: 1,381,390 / 47,160)
    wireless: 1,485,306 B flash / 52,768 B RAM (round-4: 1,481,442 / 52,768)

Auto DJ remains blocked/out of scope for this PR.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@djdefi