[RSDK-6253] Update a device's image while retaining credentials data #204

mattjperez · 2024-05-13T20:51:32Z

tested with local app image on running device:
cargo r -p micro-rdk-installer -- update-app-image --binary-path target/xtensa-esp32-espidf/esp32-server.bin

The update-app-image command does the following in sequence:

read the partition table of a currently running device
download/read the new/selected image into memory
compare the partition tables between the old app image and new one
if they do not match, prompt user to re-build and flash device from scratch
overwrite the running device's app image with the new one

Modifications are restricted to just the factory partition, leaving pre-existing NVS data intact.

mattjperez · 2024-05-13T20:55:05Z

micro-rdk-installer/src/main.rs

+            // let mut flasher = connect(&args.connect_args, &config, false, false).unwrap();
+            flasher
+                .read_flash(nvs_offset, nvs_size, 0x1000, 64, nvs_path.clone())
+                .map_err(Error::EspFlashError)?;


stuck here for some reason. keep getting a (CorruptData(4096, 4039), where 4096 is the read block size, and 4039 is the size of the chunk that was read.

This step is for retrieving the actual NVS data and not just partition info.
Trying to read the full image at the start (before parsing the running partition table) fails with this error as well.

Note: related?
espressif/esptool#823 (comment)

is the stub properly loaded? https://docs.espressif.com/projects/esptool/en/latest/esp32/esptool/flasher-stub.html

have not checked the stub. new changes made this part unnecessary

micro-rdk-installer/src/main.rs

gvaradarajan

LGTM, waiting for resolution of the two comments in this review and for me to give it a quick test

gvaradarajan · 2024-05-15T21:58:57Z

micro-rdk-installer/src/main.rs

+    if old_ptable != new_ptable {
+        log::error!("partition tables do not match!");
+        log::error!("rebuild and flash micro-rdk from scratch");
+        return Err(Error::BinaryEditError(64));


why 64? I suspect this isn't an error regarding binary size specifically (although to be fair BinaryEditError is not a good name for that error). If so, could you make a new variant in the error enum for this (and rename BinaryEditError)?

you're right, that's residual and was the closest thing I could find at the time. nice catch!

gvaradarajan · 2024-05-15T22:05:56Z

micro-rdk-installer/src/main.rs

    log::info!("Connecting...");
    let mut flasher = connect(
-        &args.monitor_args.connect_args,
+        &monitor_args.clone().unwrap().connect_args,


I see we're assuming monitor_args is always some flash_args.monitor is true. Instead of assuming, could we check that we're in a valid state regarding these two settings (and return an error if we're not)?

actually, it might not be necessary anymore for MonitorArgs to be Option. checking nowa

At the time when I was still determining the fields for AppImageArgs, if two re-exported commands (FlashArgs, WriteArgs, MonitorArgs, etc) had duplicate fields due to nesting, a runtime panic would happen.
This was a workaround for that, but is no longer necessary.

npmenard · 2024-05-16T16:28:26Z

micro-rdk-installer/src/main.rs

+        .map_err(Error::FileError)?;
+
+    let file_len = app_file_new.metadata().map_err(Error::FileError)?.len();
+    if file_len < PARTITION_TABLE_SIZE.into() {


looks weird to me shouldn't it be PARTITION_TABLE_SIZE+PARTITION_TABLE_ADDR?

you're right, changed.

micro-rdk-installer/src/main.rs

gvaradarajan

LGTM

npmenard · 2024-05-17T13:40:08Z

micro-rdk-installer/src/main.rs

-fn flash(args: WriteFlashArgs, config: &Config, app_path: PathBuf) -> Result<(), Error> {
+fn flash(
+    flash_args: FlashArgs,
+    monitor_args: MonitorArgs,


Why do we pass MonitorArgs now? what's the upside? how does the new invocation looks like?

many of espflash's method's, such as Flasher::connect, take these structs as parameters. Sadly, because the structs are all annotated as non-exhaustive, they can't just be initialized directly within our functions (ex let m_args = MonitorArgs{..}).

This means that the only way we can create these input arguments is via clap.
cargo-espflash highlights this in that it also passes all there structs directly or with a thin wrapper.
In addition, we have to be mindful about what espflash Commands we wrap in our own because the flattened fields of all inner commands must be unique, which sadly isn't always the case. espflash as a library isn't very ergonomic.

The new invocation is cargo r -p micro-rdk-installer -- update-app-image --bootloader target/xtensa-esp32-espidf/esp32-server.bin, with the breaking change being the --bootloader flag to specify the binary. Bumping espflash to 3.0 was a breaking change so it seemed an opportune moment to also have breaking change for the installer.

feels counterintuitive to use bootloader to pass an app image via a bootloader flag we should add a bin or app-bin arg.

micro-rdk-installer/Cargo.toml

acmorrow · 2024-05-17T17:06:22Z

micro-rdk-installer/src/main.rs

+            0x1000,
+            64,


What is the origin of these values? Why are they correct? When, if ever, might they need to be updated? Let's add some comments or declare some named constants.

they're default values used in espflash. will add comments about it

micro-rdk-installer/src/main.rs

acmorrow · 2024-05-17T17:17:08Z

micro-rdk-installer/src/main.rs

+        .map_err(|e| Error::PartitionTableError(e.to_string()))?;
+
+    log::info!("Retrieving new image");
+    let tmp_new = tempfile::Builder::new()


Similar question re why not just tempfile::tempfile

similar to above, except it's how our download_micro_rdk_release uses paths and not just something that takes Read.

acmorrow · 2024-05-17T17:23:51Z

micro-rdk-installer/src/main.rs

+        .prefix("micro-rdk-bin")
+        .tempdir()
+        .map_err(Error::FileError)?;
+    let app_path_new = match &args.flash_args.bootloader {


Why bootloader here? Not saying that it is wrong, just a little surprising.

It's just how espflash's clap structs have things named. Not a huge fan of it

renamed it to binary_path

micro-rdk-installer/src/main.rs

acmorrow

Code is looking pretty good, though I think there is a related temp file API that would let you avoid needing to come up with names of your own but still gives you paths.

I did note that it appears that the partition table offset can be adjusted via sdkconfig. I'm not sure what we ought to do about that. I'd be fine with the answer being "nothing" or "ticket it".

acmorrow · 2024-05-17T19:49:45Z

micro-rdk-installer/src/main.rs

@@ -24,6 +26,14 @@ use secrecy::Secret;
 use serde::Deserialize;
 use tokio::runtime::Runtime;

+const PARTITION_TABLE_ADDR: u32 = 0x8000;


Apparently though, this can be adjusted at build time: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/kconfig.html#config-partition-table-offset.

An option would keep a list of possible values we use and iterate through it when trying to extract the partition table.

Currently, even espflash hardcodes it.

A perfect solution would involve fetching/initializing the sdkconfig, parsing it, using that info when extracting; which may not match if the sdkconfig changed this default address between the old and new app images.

Either way, it's an optimization at best and new feature at worst. Better left to another ticket if we want to look closer at this. I vote leaving it as-is for now and make a ticket when it becomes necessary. This will error at that time.

I'm fine with going forward with the hard-coded value give that espflash does the same.

micro-rdk-installer/src/main.rs

acmorrow

This LGTM though I share @npmenard's concerns about the use of the term bootloader.

acmorrow · 2024-05-20T17:21:03Z

micro-rdk-installer/src/main.rs

@@ -24,6 +26,14 @@ use secrecy::Secret;
 use serde::Deserialize;
 use tokio::runtime::Runtime;

+const PARTITION_TABLE_ADDR: u32 = 0x8000;


I'm fine with going forward with the hard-coded value give that espflash does the same.

npmenard · 2024-05-20T18:18:51Z

I can't invoke write-flash anymore seems like it's expecting --elf and --log-format or is another argment mandatory now? Seems like it's the case since c317551
also passing --monitor with update-app-image doesn't open a serial monitor after flashing is done

mattjperez commented May 13, 2024

View reviewed changes

npmenard reviewed May 14, 2024

View reviewed changes

micro-rdk-installer/src/main.rs Outdated Show resolved Hide resolved

mattjperez marked this pull request as ready for review May 15, 2024 16:28

mattjperez requested a review from a team as a code owner May 15, 2024 16:28

mattjperez requested review from acmorrow, gvaradarajan and npmenard May 15, 2024 16:30

gvaradarajan requested changes May 15, 2024

View reviewed changes

mattjperez requested a review from gvaradarajan May 16, 2024 15:15

npmenard reviewed May 16, 2024

View reviewed changes

micro-rdk-installer/src/main.rs Outdated Show resolved Hide resolved

mattjperez added 19 commits May 16, 2024 12:32

fix

3f4da4b

compiling

242dd3c

fix Cargo.toml

844b0c7

fix compilation issues

b4cc5a4

rm unwraps

1ecd580

change connect_args to monitor_args

a57db50

clippy, fmt

86cff3e

error_map,fmt

a37be20

suggestions

6359d5f

add WriteAppImage command

0ae0de8

wip, compiling, panicing

69d57aa

compiling

6f4111b

working

ff2484e

clean up

258e579

fmt, clippy

e12eb0b

rm hashing

8881a8d

wip

4cabf1a

fix build err

68a509c

verified working with local app image

efacaf9

mattjperez added 4 commits May 16, 2024 12:33

cast to usize

7463e5b

better error handling, comments

cabc364

gautham suggestions

0825340

rebase

737a524

mattjperez force-pushed the installer-write-appimage branch from 1807f90 to 737a524 Compare May 16, 2024 16:41

mattjperez added 2 commits May 16, 2024 13:13

add progress bar

19f70bb

fix size cmp

e6e5f34

mattjperez requested a review from npmenard May 16, 2024 17:16

gvaradarajan approved these changes May 16, 2024

View reviewed changes

npmenard reviewed May 17, 2024

View reviewed changes

mattjperez requested a review from npmenard May 17, 2024 14:06

acmorrow requested changes May 17, 2024

View reviewed changes

mattjperez added 2 commits May 17, 2024 14:38

acmorrow suggestions

1128fc0

comments

141cdb4

mattjperez requested a review from acmorrow May 17, 2024 18:40

acmorrow reviewed May 17, 2024

View reviewed changes

use NamedTempfile

1e4c6c7

mattjperez requested a review from acmorrow May 17, 2024 20:56

rename app-image argument

1956e4f

acmorrow approved these changes May 20, 2024

View reviewed changes

Merge branch 'main' into installer-write-appimage

77c4d76

mattjperez merged commit 012a41f into viamrobotics:main May 20, 2024
5 checks passed

mattjperez deleted the installer-write-appimage branch May 20, 2024 17:53

mattjperez mentioned this pull request May 20, 2024

hotfix: write-flash command #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-6253] Update a device's image while retaining credentials data #204

[RSDK-6253] Update a device's image while retaining credentials data #204

mattjperez commented May 13, 2024 •

edited

mattjperez May 13, 2024

mattjperez May 14, 2024

npmenard May 14, 2024

mattjperez May 14, 2024

gvaradarajan left a comment

gvaradarajan May 15, 2024 •

edited

mattjperez May 16, 2024

gvaradarajan May 15, 2024

mattjperez May 16, 2024 •

edited

npmenard May 16, 2024

mattjperez May 16, 2024

gvaradarajan left a comment

npmenard May 17, 2024

mattjperez May 17, 2024 •

edited

npmenard May 20, 2024

acmorrow May 17, 2024

mattjperez May 17, 2024

acmorrow May 17, 2024

mattjperez May 17, 2024

acmorrow May 17, 2024

mattjperez May 17, 2024

mattjperez May 20, 2024

acmorrow left a comment

acmorrow May 17, 2024

mattjperez May 17, 2024 •

edited

acmorrow May 20, 2024

acmorrow left a comment

acmorrow May 20, 2024

npmenard commented May 20, 2024

[RSDK-6253] Update a device's image while retaining credentials data #204

[RSDK-6253] Update a device's image while retaining credentials data #204

Conversation

mattjperez commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvaradarajan left a comment

Choose a reason for hiding this comment

gvaradarajan May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvaradarajan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npmenard commented May 20, 2024

mattjperez commented May 13, 2024 •

edited

gvaradarajan May 15, 2024 •

edited

mattjperez May 16, 2024 •

edited

mattjperez May 17, 2024 •

edited

mattjperez May 17, 2024 •

edited