putbytes: check the install response cookie again - #398
Open
zunda-pixel wants to merge 3 commits into
Open
Conversation
The check was commented out with "TODO this fired?" — it fired because the watch answers an install with cookie 0, not because the cookie was wrong. In prv_do_install the firmware reads s_pb_state.token for the response, and the commit that always precedes an install has already reset that state, so zero is what a phone gets back. Every other step here keeps its cookie check; only the install had to give one up. Zero is tolerated rather than assumed, so the check returns for firmware that answers with the install's own token, while a cookie belonging to some other transfer is still caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Its own cookie, zero, and somebody else's. The middle one is what the firmware sends today and the reason the check was commented out; the last is what the check is still there to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Client side of coredevices/PebbleOS#1987. That PR fixes the firmware; this one restores the check that had to be given up because of it.
The TODO, answered
It fired because the watch answers an install with cookie 0, not because the cookie was ever wrong.
In
src/fw/services/put_bytes/put_bytes.c,prv_do_installanswers throughprv_cleanup_and_send_response, which readss_pb_state.token— and theCOMMITthat always precedes an install has already runprv_cleanup, which resets the transfer state. The firmware does validate the phone's token (prv_do_installlooks it up ins_ready_to_install[]and NACKs a mismatch); it just never echoed it.sendPutandsendCommitare unaffected, which is why only this one check had to go.What this changes
Zero is tolerated rather than assumed, so the check comes back for firmware that answers with the install's own token, while a cookie belonging to some other transfer is still caught. Nothing changes against firmware in the field.
Tolerating zero cannot mask a real mismatch, because zero is never a valid token:
and
prv_do_installrejectstoken == 0outright. Gating on firmware version instead would mean threading a version dependency intoPutBytesSession, which currently takes onlyputBytesService, for no gain over this.Tests
PutBytesSessionTestcovers the three answers an install can get: its own cookie, zero, and another transfer's. The middle one is what the firmware sends today; the last is what the check is still there to catch.Full suite on this branch: 209 tests, no failures. Reverting the
|| responseCookie == 0uhalf of the check failsanInstallAnsweredWithZeroIsAcceptedand nothing else, so the test pins what this PR changes.Correcting my own earlier note: an earlier version of this description said I could not compile or run Kotlin here. That was wrong — I had not tried properly. The tree does have a Gradle wrapper;
:libpebble3:compileKotlinJvmand:libpebble3:jvmTestboth work oncelocal.propertiesnames an Android SDK and a JDK 17 is on the toolchain path.The firmware side has two regression tests in coredevices/PebbleOS#1987, so the behaviour is covered from the other end.
Landing order
This wants to land with or after the firmware PR. On its own the
|| responseCookie == 0ureads oddly without the other half of the story.AI use
Written with Claude Code (Claude Opus 5), disclosed per CONTRIBUTING. The diff is nine lines; I have read it and the firmware path it describes, and can explain either.