feat: add --reset flag to tkey-runapp - #31
Conversation
1451cc9 to
6429af0
Compare
…ands Add support for sending a reset command to a running TKey application before loading a new app, without requiring a manual replug. - --reset TYPE: specifies the firmware reset type (default, flash1, client) - --bv ACTION: optionally specifies the boot verifier action after reset (app1, cmdmode); defaults to app1 if not specified When --reset is given without a file argument, the tool exits after sending the reset. When combined with a file argument, it waits 3 seconds for the device to re-enumerate before loading the app. Firmware version and UDI are only queried when not sending a reset, since the device may be running an app rather than firmware in that case.
dehanj
left a comment
dehanj
left a comment
There was a problem hiding this comment.
Overall it looks OK and supports what I expect, and in line with what soon will be available in tkeyclient (and which tkey-runapp should start using in the future).
I have a few comments:
- CI is failing. Do you want to fix it or should assist?
- It is possible to use the flags with invalid combinations, such as using
--reset default/flash1and specifying an app to load. It is only when specifying--reset clientthat loading an app makes sense. I would say it shouldn't be allowed. - I think it should describe in the usage what it supports regarding reset, and maybe clarify it a bit. For example if one uses
defaultwe assume it is out boot verifier. - In usage, maybe include that
--bvis only relevant if one specify--reset default
| if fileName == "" { | ||
| exit(0) | ||
| } | ||
| le.Printf("Waiting for CH552 to re-enumerate\n") |
There was a problem hiding this comment.
I think we shouldn't mention the CH552.
At a later stage we say "Loading app from ../tkey-device-signer/signer/app.bin onto device"
So changing it to "device" will keep it consistent, or maybe "TKey".
| if pflag.NArg() > 1 { | ||
| le.Printf("Unexpected argument: %s\n\n", strings.Join(pflag.Args()[1:], " ")) | ||
| pflag.Usage() | ||
| os.Exit(2) |
There was a problem hiding this comment.
Why changing the exit code for some of the input error handling?
My interpretation is that if input flags to tkey-runapp cannot be satisfied, like wrong combination, it exited with 2 - other errors like not opening files, TKey communication was exit code 1.
| le.Printf("Firmware name0:'%s' name1:'%s' version:%d\n", | ||
| nameVer.Name0, nameVer.Name1, nameVer.Version) | ||
| if fwResetStr == "" { | ||
| nameVer, err := tk.GetNameVersion() |
There was a problem hiding this comment.
I think the user should have to think less about what state the TKey is in, and let tkey-runapp try to figure it out by itself. It has all the means to it.
So I purpose that we simplify the input a user has to give.
If I use (as before) tkey-runapp app.bin I think it should figure out if it is firmware or not, if it is not firmware it should try a reset, and then load the app.
Potentially that the reset request is guarded by the version in the UDI (we know bellatrix cannot perform a reset), or an opt-out flag --no-reset to make it both backwards compatible and possible to have the old behavior.
Then we can of course extend this to only send a reset command, but only then I think the --reset flag should be used - and that is targeting to communicate with apps, not firmware.
I see it that it handles to purposes:
- Load an app
- Just do a reset
3 participants
Description
Add support for sending a reset command to a running TKey application before loading a new app, without requiring a manual replug.
When --reset is given without a file argument, the tool exits after sending the reset. When combined with a file argument, it waits 3 seconds for the device to re-enumerate before loading the app.
Firmware version and UDI are only queried when not sending a reset, since the device may be running an app rather than firmware in that case.
Type of change
Please tick any that are relevant to this PR and remove any that aren't.
Submission checklist