-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add download and flash gRPC commands
#50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a7a403d to
9aaa1d0
Compare
download grpc commanddownload and flash gRPC commands
fc54f77 to
a05d1d9
Compare
9aaa1d0 to
6278ad8
Compare
6e387d9 to
7c5b688
Compare
| message TaskProgress { | ||
| // Description of the task. | ||
| string name = 1; | ||
| // Additional information about the task. | ||
| string message = 2; | ||
| // Whether the task is complete. | ||
| bool completed = 3; | ||
| // Amount in percent of the task completion (optional). | ||
| float percent = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a better object for the extract and flash command
|
|
||
| service Flasher { | ||
| rpc List(ListRequest) returns (ListResponse) {}; | ||
| rpc Download(DownloadRequest) returns (stream DownloadResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a Download grpc command? Didn't we agree on exposing only the flash one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we did, but it comes for free since it shares most of the logic with the Flash command. I would keep it, even if it is not needed right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required to implement the CLI download command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't implementing the CLI download command but the gRPC interface, so I don't want to include stuff that will not be used. Can we focus on the things we need?
| DownloadProgress download_progress = 1; | ||
| // Progress of the extraction. | ||
| TaskProgress extraction_progress = 2; | ||
| // Progress of qdl flashing. | ||
| TaskProgress flash_progress = 3; | ||
| // Download result. | ||
| Result result = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I would expect to have something like
| DownloadProgress download_progress = 1; | |
| // Progress of the extraction. | |
| TaskProgress extraction_progress = 2; | |
| // Progress of qdl flashing. | |
| TaskProgress flash_progress = 3; | |
| // Download result. | |
| Result result = 4; | |
| DownloadProgress download_progress = 1; | |
| ExtractProgress extraction_progress = 2; | |
| FlashProgress flash_progress = 3; | |
| Result result = 4; |
each of which has its own internal states
| // The path in which the image will be downloaded and extracted. | ||
| string temp_path = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // The path in which the image will be downloaded and extracted. | |
| string temp_path = 2; | |
| // The path in which the image will be downloaded and extracted. | |
| // If an image file is already present, it will not be downloaded again. | |
| string temp_path = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a bug because flashing fails if the file is already present. With the CLI command we override the files each time, so we should probably do the same gRPC side or preemptively delete any possible leftovers.
We could also go for a recovery or skip-download approach.
|
|
||
| message FlashResponse { | ||
| message Result { | ||
| // Empty message, reserved for future expansion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return the full path to the downloaded image file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not because at that point the image would have already been flashed onto the board and deleted locally. I wonder if it's actually needed, I don't see how it could be useful outside of providing a Flashing succeeded message.
| // Check if there is enough free disk space before downloading and extracting an image | ||
| d, err := disk.Usage(req.TempPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if d.Free/updater.GiB < updater.DownloadDiskSpace { | ||
| return fmt.Errorf("download and extraction requires up to %d GiB of free space", updater.DownloadDiskSpace) | ||
| } | ||
|
|
||
| client := updater.NewClient() | ||
| manifest, err := client.GetInfoManifest(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var rel *updater.Release | ||
| if req.Version == "latest" || req.Version == manifest.Latest.Version { | ||
| rel = &manifest.Latest | ||
| } else { | ||
| for _, r := range manifest.Releases { | ||
| if req.Version == r.Version { | ||
| rel = &r | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if rel == nil { | ||
| return fmt.Errorf("could not find Debian image %s", req.Version) | ||
| } | ||
|
|
||
| tmpZip := paths.New(req.GetTempPath(), "arduino-unoq-debian-image-"+rel.Version+".tar.zst") | ||
| defer func() { _ = tmpZip.RemoveAll() }() | ||
|
|
||
| if err := updater.DownloadFile(ctx, tmpZip, rel, downloadCB, downloader.Config{}); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated code with the Download service. Please, move the duplicated code in a private function that is called from both Download and Flash services.
Moreover, in the Download service, the check for free space is not performed. This will be solved automatically when the duplicated code is factored out.
Co-authored-by: Luca Rinaldi <lucarin@protonmail.com>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Co-authored-by: Cristian Maglie <c.maglie@bug.st>
Motivation
Add grpc support for
downloadandflashcommands.Change description
Add grpc support for
downloadandflashcommands. The flash progress coming fromqdlhas not been implemented yet. This is an example of the command's output:Additional Notes
Reviewer checklist
main.