Compute crank cadence RPM from the CSC sensor (issue #49) #108

Merged

robert merged 1 commit from area/csc-cadence into main 2026-09-05 11:50:27 +02:00

Conversation 0 Commits 1 Files changed 8 +373 -28

robert commented 2026-09-05 11:49:36 +02:00

Owner

Copy link

Closes #49.

Crank-field decoding on the CSC Measurement characteristic (0x2A5B) already landed in #18
(CscMeasurement.kt) but had no consumer — AvgCadenceAccumulator (#65) existed with no producer,
per that issue's own "honestly un-fed" note. This wires the missing conversion.

What changed

• companion/core/.../sensors/CrankCadenceTracker.kt (new): the crank-side mirror of
  WheelSpeedTracker — instantaneous CadenceSample (Measured(rpm) / NoNewRevolution) from
  successive CrankRevolutionData samples, reusing uint16Delta for both 16-bit crank fields
  (CscWraparound.kt already flagged this reuse for #49). Covered by CrankCadenceTrackerTest,
  including real uint16 rollover on the revolution counter, the event-time clock, and both at once.

• companion/sensors/.../AndroidCscClient.kt / CscSensorState.kt: wires a CrankCadenceTracker
  per connection, reset alongside the wheel tracker on disconnect. Found and fixed a real bug in
  the process: handleMeasurement previously did measurement.wheel ?: return, so a crank-only
  sensor's notifications (which never carry a wheel field) never updated state at all — cadence was
  unreachable for that sensor class. Wheel and crank are now updated independently, each keeping its
  last-known value when the other field is absent from a given notification.

  CscSensorState.Connected gained cadence: CadenceSample? — null means "this sensor has never
  reported crank data" (a wheel-only sensor, or no notification yet), kept distinct from
  CadenceSample.NoNewRevolution ("real cadence sensor, cranks currently stopped"). This is the
  "honestly absent, not zero" representation the issue asks for, matching issue #9's Effort page
  "auto-hides without sensors" spec.

• companion/pebble/.../DerivedMetricsWireEncoding.kt: new cadenceValue(CadenceSample?) for
  Proto.Key.CADENCE_RPM (id 21, already generated from #7 / shared/message_keys.json) — null ->
  UINT8 sentinel, NoNewRevolution -> a genuine 0 (the "clamped to zero when the cranks stop"
  acceptance criterion), Measured -> its rpm. Folded into the existing encode()/changedKeysOnly
  six-key ride-metrics group alongside the pre-existing AVG_CADENCE_RPM encoding.

• DerivedMetrics.kt's own KDoc (previously: AvgCadenceAccumulator "has no real input yet") is
  updated to describe the new input. A real integration test in DerivedMetricsTest.kt feeds a real
  CrankCadenceTracker's output into AvgCadenceAccumulator end to end, proving coasting/stationary
  spans are excluded from the moving average rather than averaged in as 0.

Answers to the questions this issue asks explicitly

• Crank-field decoding (cumulativeCrankRevolutions/lastCrankEventTime off 0x2A5B) already
  existed before this PR (#18, CscMeasurement.kt) — nothing needed adding there. Confirmed by
  reading the file and its existing CscMeasurementTest coverage before writing anything new.
• A wheel-only sensor (no crank support) is represented as CscSensorState.Connected.cadence == null
  through to DerivedMetricsWireEncoding.cadenceValue(null) == Proto.Sentinel.UINT8 on the wire — never
  a 0. A real cadence sensor with stationary cranks instead reports CadenceSample.NoNewRevolution,
  which does encode to a genuine 0. The two are different types, not two branches of the same
  nullable rpm value, so this distinction can't silently collapse.
• Real test runs: ./gradlew :companion:core:test (all green, including the new
  CrankCadenceTrackerTest and the DerivedMetricsTest integration test),
  ./gradlew :companion:pebble:testDebugUnitTest (all green, including updated
  DerivedMetricsWireEncodingTest), and ./gradlew :companion:assembleDebug (succeeds — this PR
  touches Android-SDK-bound code in :companion:sensors).

Explicitly out of scope for this PR (not touched, and said so in the commit message):
watchapp/src/c/view_ride.c (showing the field on the watch) and GPX TrackPointExtension recording
— there is no ride-recording pipeline in companion/ yet to hang either of those onto, and
view_ride.c is outside companion/'s ownership. Both remain real gaps for a future issue, not
silently declared done here.

https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

Closes #49. Crank-field decoding on the CSC Measurement characteristic (`0x2A5B`) already landed in #18 (`CscMeasurement.kt`) but had no consumer — `AvgCadenceAccumulator` (#65) existed with no producer, per that issue's own "honestly un-fed" note. This wires the missing conversion. **What changed** - `companion/core/.../sensors/CrankCadenceTracker.kt` (new): the crank-side mirror of `WheelSpeedTracker` — instantaneous `CadenceSample` (`Measured(rpm)` / `NoNewRevolution`) from successive `CrankRevolutionData` samples, reusing `uint16Delta` for both 16-bit crank fields (`CscWraparound.kt` already flagged this reuse for #49). Covered by `CrankCadenceTrackerTest`, including real `uint16` rollover on the revolution counter, the event-time clock, and both at once. - `companion/sensors/.../AndroidCscClient.kt` / `CscSensorState.kt`: wires a `CrankCadenceTracker` per connection, reset alongside the wheel tracker on disconnect. **Found and fixed a real bug** in the process: `handleMeasurement` previously did `measurement.wheel ?: return`, so a crank-only sensor's notifications (which never carry a wheel field) never updated state at all — cadence was unreachable for that sensor class. Wheel and crank are now updated independently, each keeping its last-known value when the other field is absent from a given notification. `CscSensorState.Connected` gained `cadence: CadenceSample?` — `null` means "this sensor has never reported crank data" (a wheel-only sensor, or no notification yet), kept distinct from `CadenceSample.NoNewRevolution` ("real cadence sensor, cranks currently stopped"). This is the "honestly absent, not zero" representation the issue asks for, matching issue #9's Effort page "auto-hides without sensors" spec. - `companion/pebble/.../DerivedMetricsWireEncoding.kt`: new `cadenceValue(CadenceSample?)` for `Proto.Key.CADENCE_RPM` (id 21, already generated from #7 / `shared/message_keys.json`) — `null` -> `UINT8` sentinel, `NoNewRevolution` -> a genuine `0` (the "clamped to zero when the cranks stop" acceptance criterion), `Measured` -> its rpm. Folded into the existing `encode()`/`changedKeysOnly` six-key ride-metrics group alongside the pre-existing `AVG_CADENCE_RPM` encoding. - `DerivedMetrics.kt`'s own KDoc (previously: `AvgCadenceAccumulator` "has no real input yet") is updated to describe the new input. A real integration test in `DerivedMetricsTest.kt` feeds a real `CrankCadenceTracker`'s output into `AvgCadenceAccumulator` end to end, proving coasting/stationary spans are excluded from the moving average rather than averaged in as `0`. **Answers to the questions this issue asks explicitly** - Crank-field decoding (`cumulativeCrankRevolutions`/`lastCrankEventTime` off `0x2A5B`) **already existed** before this PR (#18, `CscMeasurement.kt`) — nothing needed adding there. Confirmed by reading the file and its existing `CscMeasurementTest` coverage before writing anything new. - A wheel-only sensor (no crank support) is represented as `CscSensorState.Connected.cadence == null` through to `DerivedMetricsWireEncoding.cadenceValue(null) == Proto.Sentinel.UINT8` on the wire — never a `0`. A real cadence sensor with stationary cranks instead reports `CadenceSample.NoNewRevolution`, which *does* encode to a genuine `0`. The two are different types, not two branches of the same nullable rpm value, so this distinction can't silently collapse. - Real test runs: `./gradlew :companion:core:test` (all green, including the new `CrankCadenceTrackerTest` and the `DerivedMetricsTest` integration test), `./gradlew :companion:pebble:testDebugUnitTest` (all green, including updated `DerivedMetricsWireEncodingTest`), and `./gradlew :companion:assembleDebug` (succeeds — this PR touches Android-SDK-bound code in `:companion:sensors`). **Explicitly out of scope for this PR** (not touched, and said so in the commit message): `watchapp/src/c/view_ride.c` (showing the field on the watch) and GPX `TrackPointExtension` recording — there is no ride-recording pipeline in `companion/` yet to hang either of those onto, and `view_ride.c` is outside `companion/`'s ownership. Both remain real gaps for a future issue, not silently declared done here. https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

robert added 1 commit 2026-09-05 11:49:36 +02:00

Compute crank cadence RPM from the CSC sensor (issue #49) ... f8693cd501

Crank-field decoding on the CSC Measurement characteristic (0x2A5B) already
landed in #18 (CscMeasurement.kt) but was, per #65's own admission, "honestly
un-fed" -- AvgCadenceAccumulator existed with no producer. This closes that
gap:

- CrankCadenceTracker (companion/core, sensors package): the crank-side
  mirror of WheelSpeedTracker, turning successive CrankRevolutionData samples
  into an instantaneous CadenceSample (Measured(rpm) / NoNewRevolution for a
  stationary crank or the first sample). Reuses uint16Delta for both the
  16-bit crank-revolution counter and the 16-bit event-time clock, per
  CscWraparound.kt's own note that #49 could reuse it directly. Covered by
  CrankCadenceTrackerTest, including uint16 rollover on the revolution
  counter, the event-time clock, and both at once.

- AndroidCscClient / CscSensorState (companion/sensors): wires a
  CrankCadenceTracker per connection, reset alongside the wheel tracker on
  disconnect. Fixes a real bug found while doing this: handleMeasurement
  previously did `measurement.wheel ?: return`, which meant a crank-only
  sensor's notifications (no wheel field at all) never updated state --
  cadence was silently unreachable for that sensor class. Wheel and crank are
  now updated independently, each keeping its last-known value when the
  other field is absent from a given notification.

  CscSensorState.Connected gained `cadence: CadenceSample?`, `null` meaning
  "this sensor has never reported crank data" (wheel-only sensor, or no
  notification yet) -- kept distinct from CadenceSample.NoNewRevolution
  ("real cadence sensor, cranks currently stopped") so a wheel-only sensor's
  cadence can be honestly absent rather than a fabricated zero, matching #9's
  Effort page "auto-hides without sensors" spec.

- DerivedMetricsWireEncoding (companion/pebble): new cadenceValue(CadenceSample?)
  for Proto.Key.CADENCE_RPM (id 21, already generated from #7) -- null ->
  UINT8 sentinel, NoNewRevolution -> a genuine 0 (the "clamped to zero when
  the cranks stop" acceptance criterion), Measured -> its rpm. Folded into
  encode()/changedKeysOnly's existing six-key ride-metrics group alongside
  the pre-existing AVG_CADENCE_RPM encoding.

- DerivedMetrics.kt's own KDoc, which previously documented
  AvgCadenceAccumulator as having "no real input yet", is updated to
  describe the new CrankCadenceTracker input -- and a real integration test
  (DerivedMetricsTest) feeds a CrankCadenceTracker's real output into
  AvgCadenceAccumulator end to end, proving coasting/stationary spans are
  excluded from the moving average rather than averaged in as 0.

Out of scope for this PR (not touched): watchapp/src/c/view_ride.c (display
of the new field on the watch) and GPX TrackPointExtension recording --
no ride-recording pipeline exists yet in companion/ to hang either onto, and
view_ride.c is outside companion/'s ownership.

Verified: ./gradlew :companion:core:test, :companion:pebble:testDebugUnitTest
and :companion:assembleDebug all pass (JDK 21, real Android SDK).

Claude-Session: https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

robert merged commit fc3841660c into main 2026-09-05 11:50:27 +02:00

robert referenced this pull request from a commit 2026-09-05 11:50:28 +02:00

Merge pull request 'Compute crank cadence RPM from the CSC sensor (issue #49)' (#108) from area/csc-cadence into main

robert referenced this pull request 2026-09-05 11:50:45 +02:00

Cadence from the CSC sensor #49

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:companion

Android companion app

  

area:docs

Documentation

  

area:shared

Message contract shared by both sides

  

area:tooling

Build tooling, SDK, dev scripts

  

area:watchapp

Pebble C watchapp

  

blocker

Blocks a large part of the project until resolved

  

kind:chore

Setup, scaffolding, maintenance

  

kind:feature

New functionality

  

kind:spike

Time-boxed investigation to de-risk a decision

  

kind:test

Tests and fixtures

No labels

area:companion

area:docs

area:shared

area:tooling

area:watchapp

blocker

kind:chore

kind:feature

kind:spike

kind:test

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

robert/PedalPebble!108

No description provided.