Repository navigation
Conversation
A Woodway treadmill reporting heart rate sends a 21-byte Treadmill Data packet. The link was left at the default ATT MTU of 23, which carries only 20 bytes, so every one of those notifications arrived one byte short. Parsing ran straight off a ByteBuffer wrapping the packet, so the read of the last field threw BufferUnderflowException. Notifications are delivered on a binder callback thread, so the exception was swallowed and the whole sample was dropped, including the speed and distance that had already been parsed out of the front of the packet. Over two sessions on the machine, 591 of 602 notifications were discarded; the 11 that survived were the ones sent while no heart rate was present. The workout recorded zero distance and zero speed while the treadmill was reporting both correctly. Request an MTU on connect so packets arrive whole, and pad packets before parsing so a machine whose notifications still do not fit degrades to partial data instead of none. Log when a packet declares more than the link delivered, so this cannot fail silently again.
Author
|
Following up: I said I'd confirm this on hardware before you merged, so — confirmed. I've been running it on the Woodway for the past few weeks, and sessions now record speed, distance and incline correctly end to end, where previously the workout came out as zeros. One thing you may not be able to see from your side: CI has never run on this PR. Still merges cleanly against |
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
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.
Fixes #35.
Problem
A Treadmill Data packet that includes heart rate needs 21 bytes. The connection is left at the default ATT MTU of 23, which carries 20 bytes of payload, so every one of those notifications arrives a byte short.
parseTreadmillDatareads sequentially from aByteBufferwrapping the packet, so the read of the last field throwsBufferUnderflowException. Notifications arrive on a binder callback thread, so that exception is swallowed and logged as a warning, and the entire sample is dropped — including the speed and distance already parsed out of the front of the packet.On a Woodway treadmill this discarded 591 of 602 notifications. The workout recorded zero distance and zero speed while the machine reported both correctly, with nothing in the app indicating a problem.
Change
Negotiate the MTU on connect.
requestMtu()is issued when the link comes up, and service discovery moves intoonMtuChangedso it runs once the larger MTU is in force. A device that refuses the request still proceeds to discovery, and an outright rejection falls back to discovering immediately. The negotiated value is logged, so a link that stays at 23 is visible.Pad packets before parsing. Truncation only ever removes trailing fields, so parsing from a zero-padded copy lets the fields that arrived parse normally while missing ones read back as zero. A machine whose notifications still do not fit the link now degrades to partial data instead of none. This is applied to all four data parsers, since they share the same reading pattern and the same exposure.
Log when a packet declares more than it carried, so this cannot fail silently again.
Both halves are worth having: the MTU request addresses the cause, and the padding means the app still works against a device that will not negotiate.
Testing
The app's BLE session log captured every packet from two treadmill sessions. Replaying those 602 notifications through the parser:
The richest recovered sample reads 4.82 km/h, 264 m, 10.0% incline, 17 kcal, 106 bpm, 200 s — all of which the current code discards.
tools/lint.pypasses 4/4, and bothassembleDebugandassembleReleasesucceed.Confirmed against captured traffic rather than a live machine; I have a treadmill session planned and can confirm on hardware before you merge if you would prefer to wait for that.
Note on the requested MTU
I ask for 517, the maximum an LE link can negotiate, and use whatever the peer grants. If you would rather keep the request modest, anything above 24 solves the treadmill case; 247 is a common choice. Happy to change it.