Revert SDMChannelController speed TX calculation changes #4129
+11
−13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reverts the speed encoding changes from commits 6f166d2 and eb4b73b. Restores the original speed calculation logic: - payload[4] = (byte) speedM_s - payload[5] = (byte) ((speedM_s - (double)((int)speedM_s)) / (1.0/256.0)) - payload[6] = (byte) stride_count (without increment) Reverted in both TimerTask and TX event handler.
Owner
Author
Restore stride_count++ increment in both TimerTask and TX event handler. The stride count must increment according to ANT+ SDM protocol requirements.
Restore the fixed-point speed encoding with correct little-endian byte order: - speedFixed = speedM_s * 256.0 - payload[4] = LSB (low byte) - payload[5] = MSB (high byte) The ANT+ protocol requires little-endian byte order (LSB first) for multi-byte fields. The previous big-endian encoding was causing ~1.4% pace error on Garmin devices (e.g., 6:05/km instead of 6:00/km at 10 km/h). Maintains stride_count++ increment as required by ANT+ SDM protocol.
Reverts the little-endian fixed-point encoding back to the original format: - payload[4] = integer part of speedM_s - payload[5] = fractional part * 256 - payload[6] = stride_count++ (kept) The little-endian approach made the pace worse on Garmin. Need to investigate the actual ANT+ SDM data page format.
According to ANT+ SDM Page 1 specification, byte 4 is shared: - Upper 4 bits (nibble): Distance Fractional - Lower 4 bits (nibble): Speed Integer Changed: - payload[4] = (byte) speedM_s + payload[4] = (byte) ((int)speedM_s & 0x0F) This ensures speed integer only occupies the lower 4 bits as required by the protocol, preventing interference with distance fractional data in the upper nibble. Should fix the ~1.4% pace error on Garmin devices. Reference: https://github.com/Loghorn/ant-plus/blob/master/src/stride-speed-distance-sensors.ts
Changed speed encoding to use a unified fixed-point calculation: - speedFixed = round(speedM_s * 256.0) - payload[4] = upper 4 bits (speed integer part) - payload[5] = lower 8 bits (speed fractional part) This encodes speed as a 12-bit fixed-point value (4 integer + 8 fractional bits) in big-endian format, eliminating separate rounding errors and providing better precision. Should fix the ~1.4% pace discrepancy between QZ and Garmin devices.
Changed payload[5] calculation to round instead of truncate: - Before: (byte) ((speedM_s - (int)speedM_s) * 256.0) // truncates - After: (byte) Math.round((speedM_s - (int)speedM_s) * 256.0) // rounds This should improve precision and potentially fix the ~1.4% pace discrepancy between QZ (6:00/km) and Garmin (6:05/km) at 10 km/h.
Corrected payload[7] (Update Latency) calculation: - Before: deltaTime * 0.03125 (incorrect, assumes deltaTime in seconds) - After: deltaTime * 32.0 / 1000.0 (correct for deltaTime in milliseconds) Update Latency must be in units of 1/32 second. Since deltaTime is in milliseconds, the correct conversion is: (deltaTime / 1000) * 32 = deltaTime * 0.032 The previous value of 0.03125 (1/32) introduced a 2.4% error that may have been causing the Garmin to show incorrect pace calculations.
Changed payload[7] from deltaTime-based calculation to 0: - Before: (byte) (deltaTime * 32.0 / 1000.0) // ~8 for 250ms interval - After: (byte) 0 // no measurement delay Update Latency represents the delay between speed measurement and transmission, not the interval between transmissions. In a real-time system like ours, the speed is measured at transmission time, so the latency should be 0. The Garmin may have been using the incorrect latency value (250ms) to adjust its calculations, causing the 1.4% pace discrepancy.
Corrected payload[1] and payload[2] (time fields): - Before: ((lastTime % 256000) / 5) & 0xFF and ((lastTime % 256000) / 1000) - After: (lastTime % 1000) / 5 and (lastTime / 1000) % 256 According to ANT+ SDM specification: - Byte 1: Time fractional in 1/200 sec units (range 0-199) - Byte 2: Time integer in seconds (range 0-255) The previous calculation could overflow byte 1 and give incorrect values, potentially causing the Garmin to calculate speed incorrectly from time deltas. This should fix the persistent 1.4% pace discrepancy.
This reverts commit 95b764a.
This reverts commit d1b7fab.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts the speed encoding changes from commits 6f166d2 and eb4b73b.
Restores the original speed calculation logic:
Reverted in both TimerTask and TX event handler.