feat(#673): MachineStatusEvent flow and ROM-fraction stall detection - #697
feat(#673): MachineStatusEvent flow and ROM-fraction stall detection#6979thLevelSoftware wants to merge 8 commits into
Conversation
Refs #673 PR 2 of 3: Widen the narrow deloadOccurredEvents Flow<Unit> to a rich Flow<MachineStatusEvent> carrying the full SampleStatus + position + velocity. Add a geometric ROM-fraction stall signal as a secondary arm path for set-ending detection. Changes: - New MachineStatusEvent data class (timestamp, sampleStatus, position, velocity) - BleRepository: add machineStatusEvents: Flow<MachineStatusEvent> - MonitorDataProcessor: add onStatusEvent callback, fire after velocity smoothing with max(A,B) position and max(A,B) smoothed velocity - KableBleRepository: emit MachineStatusEvent from monitorProcessor callback, derive deloadOccurredEvents for backward compatibility - FakeBleRepository: add machineStatusEvents test double - WorkoutCoordinator: add ROM calibration state (romRangeTop, romRangeBottom, romFraction, stallArmedByRomFraction) - ActiveSessionEngine: add collector #5b consuming machineStatusEvents, tracking ROM range from position observations, computing ROM fraction, and arming stall timer when velocity is in dead band (2.5-10 mm/s) AND position is mid-ROM (30-80%). Rep events cancel position-armed countdown via existing resetStallTimer(). - Tests: MonitorDataProcessorStatusEventTest (4 tests), WorkoutCoordinatorAutoStopResetTest updated for ROM-fraction fields
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh look, the previous reviewer left a clean bill of health and the new commit kept the streak alive. One new 🏆 Best part: The new 💀 Worst part: Picking nits, 📊 Overall: Like the previous commit, this is a one-feature-one-test patch that doesn't drag in any new bugs. The deload-takes-precedence invariant is now actively guarded by a regression test instead of just being asserted in comments. Files Reviewed (3 files changed since previous SHA)
Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. Previous Review Summaries (5 snapshots, latest commit 09b926d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 09b926d)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental change is actually clean. The previous Codex finding ("timer not cancelling when status leaves qualifying window") is fixed with the smallest possible surface area: one new 🏆 Best part: The cancel branch ( 💀 Worst part: There's a theoretical race where the ROM collector's arm path and the DELOAD collector's arm path could both pass the 📊 Overall: Like finding a unicorn in production — a PR that adds state and logic without adding bugs. The fix is one logical commit with one logical test. Files Reviewed (3 files changed since previous SHA)
Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. VerdictApprove — incremental fix is minimal, correct, and properly tested. Previous review (commit 37a9429)Verdict: 0 Active Findings | Recommendation: Merge Overview
The previous review's warning (warmup gate blocking ROM calibration) and suggestion (missing arm/cancel/no-arm tests) are both resolved in commit
🏆 Best part: The fix is the minimum patch possible — one line moved, one comment updated, no new abstractions. The previous review's "hoist ROM tracking above the gate" suggestion is implemented literally. 💀 Worst part: The 📊 Overall: The parachute now has the fabric deployed where it matters. Ship it. Files Reviewed (1 file changed since previous SHA)
Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Correctness / Safety FindingsNo correctness or safety findings. Final Merge GuidanceCan merge as-is. Both previous findings are resolved with minimum-surface-area changes. Previous review (commit 18a4138)Verdict: 2 Active Findings | Recommendation: Address before merge Overview
Active Findings
🏆 Best part: The dead-state cleanup is actually dead-state cleanup this time — 💀 Worst part: The author moved the visible warmup gate but left 📊 Overall: Three of four wires are cut clean. The fourth still touches ground through a helper function nobody remembered to disconnect. Incremental progress — the parachute has more fabric, it just still won't deploy at altitude. Files Reviewed (5 changed files)
Fix these issues in Kilo Cloud Previous review (commit 333a1c4)Verdict: 1 Critical | Recommendation: Address before merge Overview
Current Findings (click to expand)
Resolved Since Last Review
Carry-Forward (Not Re-Commented)
🏆 Best part: Commit 2 actually addressed the test bitmask bug cleanly — single-purpose fix, no scope creep. I had to double-check the commit hash to believe it. 💀 Worst part: The incremental diff is six lines of test fixes, and the structural critical bug it was supposed to address time for is still byte-for-byte identical. Timeout for stall detection shipping: whenever the smoke detector decides to attend the actual fire. 📊 Overall: Like a parachute with the ripcord wired to the altimeter — all the parts are present and well-named, the parachute just won't deploy at the right altitude. (Same verdict as last review, because nothing material changed.) Files Reviewed (9 files)
VerdictRequest changes — critical gating bug (MonitorDataProcessor.kt:230) plus the newly-visible signed-velocity bug (MonitorDataProcessor.kt:236) still break the scenario this PR is supposed to detect. Test coverage for the new collector's arm logic is still missing. Correctness / Safety Findings
Ponytail Review
Ponytail net: -12 lines. Suggested Minimal Patch
Final Merge GuidanceDo not merge until the critical gating fix AND the signed-velocity fix land, and the new collector has at least one arm + one no-arm test. The dead-state cleanup is optional but cheap. Fix these issues in Kilo Cloud Previous review (commit 7cba1cd)Verdict: 1 Critical | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The plumbing widening — 💀 Worst part: The core stall-detection path can't actually fire in the scenario it's meant to detect. It's like installing a smoke detector that only triggers during a birthday party. 📊 Overall: Like a parachute with the ripcord wired to the altimeter — all the parts are present and well-named, the parachute just won't deploy at the right altitude. VerdictRequest changes — critical gating bug breaks the very scenario this PR is supposed to detect. Correctness / Safety Findings
Ponytail Review
Ponytail net: -12 lines. Suggested Minimal Patch
Final Merge GuidanceDo not merge until the critical gating fix lands and the new collector has at least one arm + one no-arm test. The dead-state cleanup is optional but cheap. Reviewed by minimax-m3 · Input: 36K · Output: 5.7K · Cached: 447K Review guidance: REVIEW.md from base branch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cba1cd74b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DELOAD_OCCURRED is bit 15 (0x8000), not bit 7 (0x80). Tests were using the wrong bitmask, causing isDeloadOccurred() to return false. Also increase timeProvider to 3000L so the 2-second deload debounce does not suppress the callback in tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 333a1c40a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1. Remove status!=0 gate: emit MachineStatusEvent for every processed packet so ROM-fraction stall detector gets continuous position/velocity data, not just edge events (status flags). 2. Use abs() for velocity magnitude: firmware velocities can be negative during eccentric cable movement; take absolute max so dead-band comparison works correctly. 3. Move warmup gate after ROM range tracking: learn the ROM range during warmup so the detector has a calibrated range when working reps begin. 4. Remove dead romFraction and stallArmedByRomFraction fields: neither is read by any control flow; only romRangeTop/Bottom are used. 5. Update tests to match new always-emit behavior and removed fields.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e09b2b2dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Kotlin/native test runner rejects method names containing parentheses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a9429324
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09b926dc21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
PR 2 of 3 for Issue #673. It widens the narrow
deloadOccurredEvents: Flow<Unit>toFlow<MachineStatusEvent>, carrying the fullSampleStatus, position, and velocity for every processed BLE monitor packet. It adds a geometric ROM-fraction signal as a secondary stall-arm path.Refs #673 — non-final phase; PR 3 owns the closing keyword.
Independent value
DELOAD_WARN.Changes
Machine-status flow
MachineStatusEvent(timestamp, sampleStatus, position, velocity)model.BleRepository.machineStatusEvents: Flow<MachineStatusEvent>.MonitorDataProcessoremits an event after smoothing for every processed packet, includingstatus = 0, with maximum position and velocity magnitude.KableBleRepositoryforwards the rich event while retaining the existing deriveddeloadOccurredEventsflow for compatibility.FakeBleRepositorysupports deterministic event injection for behavioral tests.ROM-fraction stall signal
WorkoutCoordinatorretains only ROM range extrema and clears them on auto-stop reset.ActiveSessionEnginecalibrates ROM extrema from warmup samples, then permits arming only after normal auto-stop eligibility is open.Tests
MonitorDataProcessorStatusEventTest: status event emission for non-zero and zero status packets, position selection, and legacy deload callback preservation.DWSMWorkoutLifecycleTest: warmup calibration enables later mid-ROM arming; out-of-band ROM/velocity does not arm; sustained in-window cable progress resets the countdown; DELOAD takeover survives ROM-window exit; and a completed working rep cancels a ROM-fraction timer../gradlew -Pskip.supabase.check=true :shared:testAndroidHostTest./gradlew -Pskip.supabase.check=true :shared:compileKotlinIosArm64 :shared:compileTestKotlinIosArm64Sequential phase requirement
PR #686 (Phase 1) was merged to
working_branch, notmain. It must be promoted intomainbefore this Phase-2 PR is merged, preserving the signed-off three-phase sequence.UI visual evidence
Not required for this non-UI phase. PR 3 is UI-involved and requires final in-app visual evidence.