gate GET_HELLO gen5 decode on profile - #69
Conversation
cmdGetHelloModern sends the same 0x91 opcode with a gen4 profile, but the response decoder ran every 0x91 reply through Gen5HelloInfo.parse unconditionally. a gen4 reply that happens to be >=104 bytes would get its fields read at gen5's byte offsets (wrong serial/fw/battery), and a shorter one would just get dropped. gate it on profile.isGen5 like every other shared-opcode branch already does.
There was a problem hiding this comment.
Sorry @abdulsaheel, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 6 days and 4 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesHello response decoding
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR fixes cross-generation decoding by ensuring the shared GET_HELLO opcode is passed to Gen5HelloInfo.parse only for Gen5 profiles, avoiding confidently incorrect Gen4 telemetry while retaining the existing Gen5 and status handling. Sequence diagram for profile-gated GET_HELLO decodingsequenceDiagram
participant Device
participant Parser as parseCommandResponse
participant Gen5 as Gen5HelloInfo.parse
Device->>Parser: GET_HELLO response
alt profile.isGen5 and status == 1
Parser->>Gen5: Gen5HelloInfo.parse(body)
Gen5-->>Parser: decoded Gen5 hello info
else Gen4 profile or non-success status
Parser-->>Device: no hello info emitted
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
cmdGetHelloModern builds the same 0x91 opcode gen5 uses for GET_HELLO, but with a gen4 profile (see commands.dart). the decode side in parseCommandResponse never checked profile before running the reply through Gen5HelloInfo.parse, unlike every other shared-opcode branch (battery, clock, data range, body location, battery pack info, alarm time, adv name) which all gate on profile.isGen5.
so a gen4 strap answering cmdGetHelloModern with a body >=104 bytes would get its fields read at gen5's fixed offsets — wrong serial, fw version, cpu hex, battery pct, all confidently reported. a shorter gen4 reply just silently drops (Gen5HelloInfo.parse returns null on length).
fix: gate the branch on profile.isGen5, same as its siblings. no known gen4 layout for this opcode yet, so gen4 replies emit nothing rather than guess.
not currently live in edge (grepped, no caller of cmdGetHelloModern there yet) but wrong regardless for the package's own correctness.
dart test: 631 passed, 4 skipped, 0 failed.Summary by Sourcery
Restrict GET_HELLO response decoding to gen5 profiles to prevent misreporting gen4 device information.
Bug Fixes:
Summary by CodeRabbit