Consolidate brownfield chassis and BLE safety packets #1 + #8 - #722
Consolidate brownfield chassis and BLE safety packets #1 + #8#7229thLevelSoftware wants to merge 5 commits into
Conversation
Unknown and V-Form hardware now cap at 100 kg/cable; Trainer+ stays 110. Validators, packet factory, ASE send site, and weight sliders require PhoenixModel. forceMax is min(weight+10, chassisMax). Echo 0x4E is unchanged and still does not carry kg.
Thread PhoenixModel into rack calculate, set-ready weight, and weight-increase recommendations. Clamp finite-rep CONFIG progression to remaining chassis headroom. Wire HardwareDetection.getCapabilities to per-cable ChassisLimits. Expand host goldens and fail-close test helpers.
Extract BleAdvertisementFilter so startScanning matches scanAndConnect: connectable iff Vee_ or VIT. connect() re-checks identity before GATT and does not send CONFIG or RESET. Unnamed NUS/FEF3 may be listed, not auto-bound. Last-successful identifier remains an opt-in.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 755524bece
ℹ️ 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".
| val encodedProgression = ChassisLimits.finiteRepProgressionKg( | ||
| requestedKg = params.progressionRegressionKg, | ||
| weightPerCableKg = targetWeightPerCable, | ||
| reps = params.reps, |
There was a problem hiding this comment.
For finite workouts with positive progression and warmupReps > 0—the normal cable path forces three firmware warm-up reps—the packet serializes reps + warmupReps, and the firmware applies the increment to every rep, but this calculation caps against only params.reps. For example, 90 kg with 8 working reps and 3 warm-up reps encodes a 10/7 kg increment even though there are 10 transitions, producing an implied 104.3 kg trajectory that hits forceMax early instead of progressing to the ceiling on the final rep; pass the total serialized rep count to the cap calculation.
Useful? React with 👍 / 👎.
| BleAdvertisementFilter.mayConnectWithAdvertisementIdentity( | ||
| scannedName = device.name, | ||
| advertisedName = advertisedName, | ||
| identifier = device.address, | ||
| lastSuccessfulIdentifier = lastSuccessfulIdentifier, |
There was a problem hiding this comment.
When a stale ScannedDevice has a VIT... label but the stored advertisement for the same identifier now has a Vee_... label, this predicate accepts both independently. The connection is then published with HardwareDetection.detectModel(device.name) at line 686, so the stale label classifies the V-Form as Trainer+ and permits the new validators and packet builder to use the unsafe 110 kg ceiling. Derive the connected model from advertisedName, or reject identities whose chassis families disagree.
Useful? React with 👍 / 👎.
|
|
||
| hasPhoenixServiceData | ||
| } | ||
| .filter { advertisement -> shouldListAdvertisement(advertisement) } |
There was a problem hiding this comment.
If an identifier first advertises an accepted Vee_/VIT name and later advertises a rejected generic name such as Phoenix or Vitruvian, this filter discards the later observation before either discoveredAdvertisements or currentScannedDevices is updated. The stale trainer row therefore remains selectable, and connect() rechecks the previously cached accepted advertisement rather than the latest rejected identity, allowing GATT creation despite the intended fail-closed rule. Rejected observations for known identifiers need to remove or invalidate the cached entry.
Useful? React with 👍 / 👎.
| val preview by remember(currentMode, exercises) { | ||
| derivedStateOf { | ||
| currentMode?.let { mode -> applyBulkAdjust(exercises, mode) } | ||
| currentMode?.let { mode -> applyBulkAdjust(exercises, mode, hardwareModel) } |
There was a problem hiding this comment.
When the connection changes while this dialog is open, hardwareModel can switch between Unknown/V-Form and Trainer+, but it is absent from the remember keys, so the retained derivedStateOf continues calling applyBulkAdjust with the old model. Connecting a Trainer+ leaves the preview capped at 100 kg, while disconnecting or switching to V-Form can leave a preview up to 110 kg that is subsequently applied; include hardwareModel in the remembered inputs.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| name = scannedName, | ||
| identifier = identifier, | ||
| lastSuccessfulIdentifier = lastSuccessfulIdentifier, | ||
| ) && mayConnect( |
There was a problem hiding this comment.
🔥 The Roast: The mayConnect call on line 70 hands advertisedName = null into the opt-in branch. A nameless stored advertisement that was never a successful connect passes purely because its BLE address once matched lastSuccessfulIdentifier. That is not fail-closed — that is fail-warm with a free pass for whatever address your last Trainer had. The whole point of this function is that the stored advertisement is the authoritative identity; treating null as "it's an unnamed placeholder, OK" defeats it.
🩹 The Fix: Require the stored advertisement name to be connectable independently, OR at minimum disallow the lastSuccessfulIdentifier opt-in when advertisedName == null inside mayConnectWithAdvertisementIdentity. For example:
fun mayConnectWithAdvertisementIdentity(
scannedName: String?,
advertisedName: String?,
identifier: String?,
lastSuccessfulIdentifier: String? = null,
): Boolean {
val scannedOk = mayConnect(
name = scannedName,
identifier = identifier,
lastSuccessfulIdentifier = lastSuccessfulIdentifier,
)
// Stored advertisement is authoritative: must be connectable on its own.
// Opt-in is only for the scanned UI label path.
return isConnectableName(advertisedName) && scannedOk
}📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -178,7 +182,7 @@ fun BulkWeightAdjustDialog( | |||
| // Compute preview | |||
| val preview by remember(currentMode, exercises) { | |||
There was a problem hiding this comment.
🔥 The Roast: remember(currentMode, exercises) does not list hardwareModel as a key, even though the closure on line 185 calls applyBulkAdjust(exercises, mode, hardwareModel). If a user opens the dialog on a V-Form, swaps to a Trainer+ mid-dialog, and then changes nothing else, the preview silently keeps the V-Form 100 kg ceiling. Compose is forgiving until it isn't, and this is the "isn't" day.
🩹 The Fix: Add hardwareModel to the key list:
| val preview by remember(currentMode, exercises) { | |
| val preview by remember(currentMode, exercises, hardwareModel) { | |
| derivedStateOf { | |
| currentMode?.let { mode -> applyBulkAdjust(exercises, mode, hardwareModel) } | |
| } | |
| } |
📏 Severity: important
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| model: PhoenixModel, | ||
| unlimitedReps: Boolean, | ||
| ): Float { | ||
| if (!requestedKg.isFinite()) return 0f |
There was a problem hiding this comment.
🔥 The Roast: if (!requestedKg.isFinite()) return 0f silently turns NaN/Infinity into 0.0 and goes on to write that as the encoded progression byte. BlePacketFactory.createProgramParams calls this before validateProgramParams runs on the packet, so a NaN slipping in here never hits the validator and you get a 0 kg/rep progression encoded on the wire. The contract for "silent rewrite to 0" should be reserved for documented business logic, not "well, validator usually catches it."
🩹 The Fix: Either throw (let the validator fail the packet cleanly), or coerceIn to a finite range and add an explicit Logger.w so field debugging sees the clamp. Do not return a value that silently diverges from what the caller requested.
📏 Severity: important
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Trainer+ hardware ceiling — used by UI sliders to enforce absolute maximum | ||
| const val MAX_WEIGHT_PER_CABLE_KG = 110f | ||
| const val MAX_WEIGHT_KG = ChassisLimits.V_FORM_KG_PER_CABLE |
There was a problem hiding this comment.
🔥 The Roast: MAX_WEIGHT_KG = ChassisLimits.V_FORM_KG_PER_CABLE and MAX_WEIGHT_PER_CABLE_KG = ChassisLimits.TRAINER_PLUS_KG_PER_CABLE are now lying about themselves. MAX_WEIGHT_KG reads as "the system-wide max" but equals 100; MAX_WEIGHT_PER_CABLE_KG reads as "the per-cable max" but is hard-wired to Trainer+ 110. AssessmentEngine.kt:51 and MonitorDataProcessor.kt:307-310 (unchanged) still consume these as if they were authoritative, so a V-Form user with a 100 kg program is now correctly capped but the variable name has become a lie that future maintainers will trust. That is how you get a 220 kg regression in six months.
🩹 The Fix: Either delete these aliases and migrate callers to ChassisLimits.maxKgPerCable(model) (preferred — there is exactly one absolute-physical-ceiling concept, and it belongs in ChassisLimits), or rename them to V_FORM_FAIL_CLOSED_KG / TRAINER_PLUS_ABSOLUTE_CEILING_KG so the names match the values.
📏 Severity: minor
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * [maxResistanceKg] is the per-cable chassis ceiling from [ChassisLimits]. | ||
| */ | ||
| data class HardwareCapabilities(val supportsEccentricMode: Boolean, val supportsEchoMode: Boolean, val maxResistanceKg: Float) { | ||
| companion object { |
There was a problem hiding this comment.
🔥 The Roast: HardwareCapabilities.DEFAULT is the lonely walrus of this file — defined, fully populated, and never read by anything in the repo. getCapabilities constructs fresh instances per call. Dead config is dead config, regardless of how thoughtfully it was written.
🩹 The Fix: Delete lines 54-60 (the companion object { val DEFAULT = ... } block). If a default is ever needed, getCapabilities("") already returns the correct fail-closed shape via detectModel.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| object ChassisLimits { | ||
| const val V_FORM_KG_PER_CABLE = 100f | ||
| const val TRAINER_PLUS_KG_PER_CABLE = 110f | ||
| const val UNKNOWN_FAIL_CLOSED_KG_PER_CABLE = V_FORM_KG_PER_CABLE |
There was a problem hiding this comment.
🔥 The Roast: UNKNOWN_FAIL_CLOSED_KG_PER_CABLE = V_FORM_KG_PER_CABLE is a synonym with a hat on. There is one constant (V_FORM_KG_PER_CABLE = 100f), one alias of that constant, and exactly one call site (maxKgPerCable, line 30). The alias adds a hop in the reader's head and zero semantics. Naming the value "unknown fail-closed" when it is literally just "V-Form" is the kind of indirection that makes future maintainers think there is a policy where there isn't.
🩹 The Fix: Inline at the single call site and delete the alias:
| const val UNKNOWN_FAIL_CLOSED_KG_PER_CABLE = V_FORM_KG_PER_CABLE | |
| fun maxKgPerCable(model: PhoenixModel): Float = when (model) { | |
| PhoenixModel.TrainerPlus -> TRAINER_PLUS_KG_PER_CABLE | |
| PhoenixModel.VFormTrainer, | |
| PhoenixModel.Unknown, | |
| -> V_FORM_KG_PER_CABLE | |
| } |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| fun maxKgPerCable(model: PhoenixModel): Float = when (model) { | ||
| PhoenixModel.TrainerPlus -> TRAINER_PLUS_KG_PER_CABLE | ||
| PhoenixModel.VFormTrainer, |
There was a problem hiding this comment.
🔥 The Roast: Trailing comma followed by -> on its own line is a Kotlin formatter's nightmare. kotlinx.serialization and ktfmt will both normalize this on the next format run, producing a noisy diff churn for zero readability gain. If you wanted a comma-first list you wrote Kotlin; if you wanted ktlint to leave you alone, put it on one line.
🩹 The Fix:
| PhoenixModel.VFormTrainer, | |
| fun maxKgPerCable(model: PhoenixModel): Float = when (model) { | |
| PhoenixModel.TrainerPlus -> TRAINER_PLUS_KG_PER_CABLE | |
| PhoenixModel.VFormTrainer, PhoenixModel.Unknown -> V_FORM_KG_PER_CABLE | |
| } |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental commit actually closes the critical/important findings from the previous review. I need to sit down. I had my flamethrower warmed up and everything, and the author fixed the exact three things I asked them to fix. Fixes verified against
|
| Severity | Count |
|---|---|
| 🚨 critical | 1 |
| 2 | |
| 💡 suggestion | 0 |
| 🤏 nitpick | 3 |
Issue Details (click to expand)
| File | Line | Roast |
|---|---|---|
shared/.../BleAdvertisementFilter.kt |
70 | mayConnectWithAdvertisementIdentity lets a null-name stored ad pass via lastSuccessfulIdentifier opt-in — defeats the authoritative-identity guarantee. |
shared/.../BulkWeightAdjustDialog.kt |
183 | remember(currentMode, exercises) omits hardwareModel; preview goes stale on a mid-dialog chassis swap. |
shared/.../ChassisLimits.kt |
49 | !requestedKg.isFinite() → 0f silently rewrites NaN/Infinity to 0 before the validator can reject the packet. |
shared/.../Constants.kt |
18 | MAX_WEIGHT_KG/MAX_WEIGHT_PER_CABLE_KG now alias specific chassis ceilings — name lies about the value. |
shared/.../HardwareDetection.kt |
54 | HardwareCapabilities.DEFAULT is dead — no callers. Delete. |
shared/.../ChassisLimits.kt |
20 | UNKNOWN_FAIL_CLOSED_KG_PER_CABLE is a synonym for V_FORM_KG_PER_CABLE. Inline. |
shared/.../ChassisLimits.kt |
28 | Trailing-comma-then-->-on-newline is non-idiomatic Kotlin and will churn on the next ktfmt run. |
🏆 Best part: The fail-closed pattern (Unknown → V-Form 100 kg) is consistently threaded through WorkoutCommandValidator, BlePacketFactory.forceMaxKg, ApplyEquipmentRackLoadUseCase, recommendations, rack load, drop-set resolver, UI steppers, and ASE. The chassis abstraction lives in exactly one place (ChassisLimits) and every screen reads from it. Whoever designed the seam deserves a coffee.
💀 Worst part: BleAdvertisementFilter.mayConnectWithAdvertisementIdentity is the gate between user-tapped label and a CONFIG packet — and it lets advertisedName == null slide on a lastSuccessfulIdentifier match alone. That's the one finding here that can write bad CONFIG bytes to a cable.
📊 Overall: A consolidation this big almost always carries one subtle seam defect, and this PR's seam defect is in BLE identity. Fix line 70 of BleAdvertisementFilter.kt and the rest is gravy.
Ponytail Pass (mandatory)
shared/.../ChassisLimits.kt:20— ponytail-shrink:UNKNOWN_FAIL_CLOSED_KG_PER_CABLE = V_FORM_KG_PER_CABLEis a single-use alias. Replace withV_FORM_KG_PER_CABLEdirectly.shared/.../HardwareDetection.kt:54-60— ponytail-delete:HardwareCapabilities.DEFAULTis unreferenced. Delete the companion.shared/.../ChassisLimits.kt:28-30— ponytail-shrink: reformatwhento one line per Kotlin idiom; ktfmt will churn otherwise.shared/.../BulkWeightAdjustDialog.kt:183— already covered above as correctness.
No correctness-required validations, security checks, tests, or logging were touched by Ponytail.
Ponytail net: -8 lines (3 constants, 1 dead companion, 1 minor reformat).
Files Reviewed (44 files)
- BLE:
BleAdvertisementFilter.kt,KableBleConnectionManager.kt,BleConstants.kt,HardwareDetection.kt - Core limits:
ChassisLimits.kt,Constants.kt,WorkoutCommandValidator.kt,BlePacketFactory.kt,WeightRecommendation.kt - Use cases:
ApplyEquipmentRackLoadUseCase.kt,DropSetCandidateResolver.kt,DropSetEligibilityPolicy.kt,RecommendWeightAdjustmentUseCase.kt - Managers:
ActiveSessionEngine.kt,DefaultWorkoutSessionManager.kt,RoutineFlowManager.kt,ActiveSessionEngineChassisLimitHostTest.kt - UI:
BulkWeightAdjustDialog.kt,ExerciseConfigModal.kt,WeightStepper.kt,WorkoutTabPreviews.kt - Screens:
ExerciseEditBottomSheet.kt,JustLiftScreen.kt,ModeConfirmationScreen.kt,RestTimerCard.kt,RoutineEditorScreen.kt,RoutineOverviewScreen.kt,SetReadyScreen.kt,SingleExerciseScreen.kt,TrainingCyclesScreen.kt,WorkoutTab.kt - Tests: 9 test files (BLE, factory, validator, use-case, dialog, host)
Fix these issues in Kilo Cloud
Suggested Minimal Patch
BleAdvertisementFilter.kt:70— requireisConnectableName(advertisedName)independently of the opt-in, or remove the opt-in from themayConnect(advertisedName, …)call. The "stored advertisement is authoritative" contract is the whole reason this function exists.BulkWeightAdjustDialog.kt:183— addhardwareModeltoremember(currentMode, exercises, hardwareModel).ChassisLimits.kt:49— replaceif (!requestedKg.isFinite()) return 0fwiththrow IllegalArgumentException(...)or a loggedcoerceIn; do not silently rewrite to 0.ChassisLimits.kt:20,HardwareDetection.kt:54-60,ChassisLimits.kt:28-30— Ponytail cleanups, optional.
Final Merge Guidance
Do not merge until the BleAdvertisementFilter.mayConnectWithAdvertisementIdentity null-name path is closed. The other two important findings should be addressed in the same patch. Ponytail items are optional cleanup.
Reviewed by minimax-m3 · Input: 33.2K · Output: 8.2K · Cached: 874.1K
Review guidance: REVIEW.md from base branch main
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a450817207
ℹ️ 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".
| ) { | ||
| val weightSuffix = if (weightUnit == WeightUnit.LB) "lbs" else "kg" | ||
| val maxWeight = if (weightUnit == WeightUnit.LB) 242f else 110f | ||
| val maxWeight = ChassisLimits.maxDisplay(hardwareModel, weightUnit) |
There was a problem hiding this comment.
When a profile has a saved Trainer+ weight above 100 kg and this screen loads while disconnected or later connects to a V-Form, this only narrows the picker's range; weightPerCable remains, for example, 105 kg and the parameter-publishing effect is not keyed on hardwareModel. The picker renders its closest in-range value (100 kg), but ActiveSessionEngine subsequently rejects the retained 105 kg direct command, so starting fails despite the UI showing a valid weight until the user manually moves the picker. Clamp/synchronize the backing weight whenever the chassis maximum changes.
Useful? React with 👍 / 👎.
Summary
Consolidates brownfield packets #1 and #8 onto current
mainatf4695a90ce9467852308675c5b00ed0bbde12337.Applied in source-chain order:
bfbd18d7819f67fe01286dd0d7df82fe24189c1bb1486e8fab8ed613f965217841e5ffea661542a1a92214fe925e3156dc78c57ae0e4c1ca78a7f435The branch also contains a small post-cherry-pick review fix (
755524b) for stored-advertisement identity handling and the Vitruvian classifier edge case.Behavior
110 kg, V-Form/Unknown100 kg.0x04CONFIG validation.forceMaxtomin(selected weight + 10, chassis ceiling).Vee_/VITdevices, excludes namedPhoenix/Vitruvian/generic devices, and shows unnamed NUS/FEF3 candidates as visible-only.scanAndConnectexcludes unnamed candidates;connect()rechecks both the scanned label and stored advertisement before Peripheral/GATT creation.0x4Epacket behavior is unchanged.Preservation
WorkoutExecutionGuardsource and tests are unchanged.Verification
Fresh final-tree results:
218tests,0failures,0errors.324tests,0failures,0errors.:shared:testAndroidHostTest:3,757tests,0failures,0errors.:shared:compileKotlinIosArm64 :shared:compileTestKotlinIosArm64: passed.:androidApp:testDebugUnitTest: passed.:androidApp:assembleDebug: passed.:androidApp:lintDebug: passed.git diff --check: passed.Release boundary
Real V-Form and Trainer+ hardware validation remains a documented release boundary; no physical-device result is being claimed here.
Refs brownfield scan 8951e31b packets #1 and #8