Add ota upload command #31
Conversation
|
Regarding the lzss dependency, check https://github.com/bcmi-labs/builder-api/pull/181. It seems like there are no widespread Go implementations that are compatible with the C implementation used on boards firmware. Imho in the future we should just make ota-builder public (after checking that this hasn't security implications). |
| return fmt.Errorf("%s: %w", "cannot create temporary folder", err) | ||
| } | ||
| otaFile := filepath.Join(otaDir, "temp.ota") | ||
| defer os.RemoveAll(otaDir) |
There was a problem hiding this comment.
Why don't we generate the file directly in the generic temporary directory? Also, I think it could be useful to not delete the binary in case we (or some sophisticated user) want to look at it for debugging purposes.
If I'm not wrong it should be regularly deleted by the OS anyway (I know this happens at least on Ubuntu). Or we could give the possibility to decide this behaviour with a --debug flag.
What do you think?
There was a problem hiding this comment.
Why don't we generate the file directly in the generic temporary directory?
What do you mean? It is generated in the temp directory
I think it could be useful to not delete the binary in case we (or some sophisticated user) want to look at it for debugging purposes.
here I see two reasons for not doing this:
- it's simpler if the user doesn't know at all that a
.otaexists, so leaving a compressed binary would be also dangerous - if we want to make the user capable of generating a
.otafile, we should add a specificota generatecommand - allowing the user to manage
.otafiles is risky. Each ota file is valid only for a specific board, if a user tries to upload it to a different board then the firmware will not be applied and the user is not notified of the causes. .otafiles are compressed.binfiles with the addition of an header, for debug purposes the.binfile should be analyzed
in conclusion I don't see any benefits in letting the user see or know about a .ota file
There was a problem hiding this comment.
What do you mean? It is generated in the temp directory
I mean replacing lines 37-41 with:
otaFile, err := ioutil.TempFile("", "temp.ota")
if err != nil {
return fmt.Errorf("%s: %w", "cannot create temporary file for ota binary", err)
}
defer os.Remove(otaFile.Name())So that we don't create a temporary directory but just a file.
here I see two reasons ...
Ok, you convinced me 😛 (we or the sophisticated user can always edit the source code to accomplish it)
There was a problem hiding this comment.
LGTM! Tested and fully working.
I was wondering if we want to implement in this PR an additional option to support the so-called "deferred OTA" (see section "Deferred/Asynchronous OTA" of the RFC doc) which basically maps the expire_in_mins parameter.
Or we can just implement that in another branch.
The Deferred OTA only requires a change to the parameters used to call iot-api, right? |
|
Now that the OTA code is spread across three different repos I am starting to feel concerned 😇 We should consider to pick one of them (the https://github.com/arduino/ota-builder from @silvanocerza is probably a good one) and make it single source of the OTA crap. I'm adding a todo to make it happen. |
Yep! |
Yeah, the firmware team moved first, and picked up this public domain C implementation that we (@rsora and me) weren't able to handle using any available Go implementation. So our unique option was embrace it and wrap in a Go package. |
Which are the others? Other than arduino/ota-builder obviously. |
The ota flow is the following: the device to be updated should be already provisioned and online. The user should have a sketch locally. This sketch must be compiled through arduino-cli or arduino-ide1/2. Then, this command should be used in order to start the upload: $./iot-cloud-cli ota upload --device-id <deviceID> --file <sketchName.ino.bin> * Import ota generator * Implement ota generation * Add ota upload command * Adapt to format-output changes * Fix typos * Improve ota upload help * Add deferred option * Improve deferred ota * Update readme
The ota flow is the following: the device to be updated should be already provisioned and online. The user should have a sketch locally. This sketch must be compiled through arduino-cli or arduino-ide1/2. Then, this command should be used in order to start the upload: $./iot-cloud-cli ota upload --device-id <deviceID> --file <sketchName.ino.bin> * Import ota generator * Implement ota generation * Add ota upload command * Adapt to format-output changes * Fix typos * Improve ota upload help * Add deferred option * Improve deferred ota * Update readme
Motivation
This PR introduces all the necessary changes to send an ota upload request to the file.
The flow is the following: the device to be updated should be already provisioned and online. The user should have a sketch locally. This sketch must be compiled through arduino-cli or arduino-ide1/2. Then, this command should be used in order to start the upload:
$./iot-cloud-cli ota upload --device-id <deviceID> --file sketch-file.ino.binNote that the ota command/subcommand is just a proposal, it can be easily changed.
Also, note that this internal project https://github.com/arduino/ota-builder (that used to live in builder-api) has been temporarily imported in an internal package.
We should now decide if it makes sense to maintain this library in a separated repo like ota-builder (but it must become public) or it's better to make it living here as an exportable package. Also a feedback I received: it would be better to change the lzss dependency with a actively maintained go library.
Change description
Additional Notes
Reviewer checklist
main.CONTRIBUTING.md) and are well formatted.