[RSDK-9195] ota async task #350

mattjperez · 2024-11-14T17:18:19Z

The following PR:

RSDK-9195: moves the ota logic to an async task attached to viam-server (this will enable retries/incremental-download/etc)
RSDK-9198: uses async hyper::http2 instead of the blocking EspHttpClient

The errors will get handled better as part of RSDK-9214

mattjperez · 2024-11-14T18:36:34Z

micro-rdk/Cargo.toml

+bincode = "2.0.0-rc.3"
+


I can make this an optional dependency for ota feature moving forward. That way it doesn't get into the regular build

will rm ws

moved to just being compiled when target isn't esp32 for now. the ticket for having a native representation for the app description might use a different crate eventually so defering until then.

mattjperez · 2024-11-14T18:37:52Z

micro-rdk/src/common/conn/viam.rs

@@ -622,6 +600,42 @@ where
            }
        }

+        #[cfg(feature = "ota")]


moved the whole ota section to after certs have been retrieved and stored

Why do we need certs? I thought the certs were for us serving

oh, I am using the same certs for the hyper client's https connection. should I not be?

Let's try the empirical answer: does it still work if you don't use them?

you were right. we just need the certificate authority, which the H2 connector is already using. removed the certificates completely.

mattjperez · 2024-11-14T18:39:47Z

micro-rdk/src/common/ota.rs

+struct EspAppDesc {
+    //TODO verify this is doing what I think it's doing, validate results
+    /// ESP_APP_DESC_MAGIC_WORD (0xABCD5432)
+    magic_word: u32,
+    secure_version: u32,
+    reserv1: [u32; 2],
+    /// application version
+    version: [u8; 32],
+    project_name: [u8; 32],
+    /// compile time
+    time: [u8; 16],
+    /// compile date
+    date: [u8; 16],
+    idf_ver: [u8; 32],
+    app_elf_sha256: [u8; 32],
+    /// minimal eFuse block revision supported by image, in format: major * 100 + minor
+    min_efuse_blk_rev_full: u16,
+    /// maximal eFuse block revision supported by image, in format: major * 100 + minor
+    max_efuse_blk_rev_full: u16,
+    reserv2: [u32; 19],
+}


I added this for testing in native, however we can maybe use it in place of esp_app_desc_t entirely.
Or if we want to just use the c ffi for esp_app_desc_t we can do an unsafe transmute on the first 256 bytes of an image.

This is fine for now, let's make a ticket to revisit. I'd prefer we ultimately use the C FFI one when possible, and declare our own for native. Ideally with some sanity checks on esp32 that the native one seems to line up with the FFI one.

Please link the ticket in here once you have filed it.

micro-rdk/src/common/ota.rs

mattjperez · 2024-11-14T18:45:55Z

micro-rdk/src/common/ota.rs

+                            EspAppDesc,
+                            bincode::config::Configuration,
+                        >(
+                            &data[..256], bincode::config::standard()


will change this to size_of::<EspAppDesc>

mattjperez · 2024-11-14T18:47:10Z

micro-rdk/src/common/ota.rs

+                    .await
+                    .map_err("failed to write data to partition".to_string())?;
+                // TODO change back to 'n' after impl async writer
+                nwritten += data.len();


will probably take the length before writing, but seems to still work

gvaradarajan

It mostly LGTM actually, just a few questions

gvaradarajan · 2024-11-19T21:04:48Z

micro-rdk/src/common/ota.rs

+                        let mut loader = EspFirmwareInfoLoader::new();
+                        loader.load(&data).map_err(OtaError::EspError)?;
+                        let new_fw = loader.get_info().map_err(OtaError::EspError)?;
+                        log::info!("current firmware: {:?}", running_fw_info);


log::debug instead (for both statements?

Overall though, I do feel like this method should probably have some well thought out user facing logs that provide context about what the OTA system is doing.

micro-rdk/src/common/ota.rs

gvaradarajan · 2024-11-19T21:07:54Z

micro-rdk/src/common/ota.rs

+        let (mut sender, conn) = {
+            http2::Builder::new(self.exec.clone())
+                .keep_alive_interval(Some(std::time::Duration::from_secs(120)))
+                .keep_alive_timeout(std::time::Duration::from_secs(10))


Is this the minimum amount of time we can use for this timeout? If a smaller timeout works, I think it would be preferred

is there a lower limit you have in mind?

no, I figured we could arrive at this by lowering the duration until it no longer works on a relatively unstable network (probably do something more like binary search)

Is there a reason that we know we need to adjust these from the defaults?

confirmed working without setting keep_alive_interval and using default timeout (20 seconds).

micro-rdk/src/common/ota.rs

gvaradarajan · 2024-11-19T21:13:26Z

micro-rdk/src/common/ota.rs

+
+#[cfg(feature = "esp32")]
+type OtaConnector = crate::esp32::tcp::Esp32H2Connector;
+#[cfg(not(feature = "esp32"))]


maybe I forgot from the previous PR, but why are we allowing cases where we're not on an esp32? Isn't OTA meaningless otherwise?

faster iteration when testing, specifically around hyper, config checks, and inspecting the binary to be downloaded (recommended by nico)

That's fair for development purposes maybe, but isn't this confusing for merged-in code? It took me a while to see that OTA compiles for native but makes http calls that essentially do nothing.

Not sure what you mean by nothing. The calls are for downloading the OTA binary and inspecting it. This would also be useful for verifying future work we talked about for editing the binary signatures directly.

Should I put it back and take it out between every PR for this feature? Or clean it up at the end if it doesn't prove useful beyond the planned work?

I guess to that point, what is the purpose of having a native build?

I mean the native build still tests the connection infrastructure abstraction and acts as a server. In native "OTA", there's no updating happening so it doesn't feel like a fair comparison. But we can leave it in and revisit at the end of the epic

micro-rdk/src/common/conn/viam.rs

acmorrow · 2024-11-21T19:01:22Z

micro-rdk/src/common/ota.rs

+
+// TODO(RSDK-9200): set according to active partition scheme
+const OTA_MAX_IMAGE_SIZE: usize = 1024 * 1024 * 4; // 4MB
+pub const OTA_MODEL_TYPE: &str = "ota_service";


Could this just be a slice of OTA_MODEL_TRIPLET so they can't get out of phase?

I tried doing that initially, but was having issues with using a const str to define another const str, will look into it again.

accomplished using once_cell::sync::Lazy, though not a fan of the deref, but it works.

acmorrow · 2024-11-21T19:04:06Z

micro-rdk/src/common/ota.rs

+struct EspAppDesc {
+    //TODO verify this is doing what I think it's doing, validate results
+    /// ESP_APP_DESC_MAGIC_WORD (0xABCD5432)
+    magic_word: u32,
+    secure_version: u32,
+    reserv1: [u32; 2],
+    /// application version
+    version: [u8; 32],
+    project_name: [u8; 32],
+    /// compile time
+    time: [u8; 16],
+    /// compile date
+    date: [u8; 16],
+    idf_ver: [u8; 32],
+    app_elf_sha256: [u8; 32],
+    /// minimal eFuse block revision supported by image, in format: major * 100 + minor
+    min_efuse_blk_rev_full: u16,
+    /// maximal eFuse block revision supported by image, in format: major * 100 + minor
+    max_efuse_blk_rev_full: u16,
+    reserv2: [u32; 19],
+}


This is fine for now, let's make a ticket to revisit. I'd prefer we ultimately use the C FFI one when possible, and declare our own for native. Ideally with some sanity checks on esp32 that the native one seems to line up with the FFI one.

acmorrow · 2024-11-21T19:05:32Z

micro-rdk/src/common/ota.rs

+    reserv2: [u32; 19],
+}
+
+// TODO(RSDK-9214)


When TODO(TICKET) please also include a little explanation of what is TODO. It can be brief, but that way the reader can get a sense without needing to jump over to the ticket.

acmorrow · 2024-11-21T19:06:22Z

micro-rdk/src/common/ota.rs

+        cert: TlsCertificate,
+        exec: Executor,
+    ) -> Result<Self, OtaError> {
+        // TODO(RSDK-9205)


Ditto more details on comment

acmorrow · 2024-11-21T19:12:39Z

micro-rdk/src/common/ota.rs

+        let mut total_downloaded: usize = 0;
+        let mut got_info = false;
+
+        while let Some(next) = response.frame().await {


Do we have any ability to exercise control over the max size of the frames we will get?

it appears the closest thing would be the range header, to request a specific range of bytes to be served (could be useful for resumable download). this would turn into mutiple requests though, one for each segment of our max size.

However, there's no guarantee that the server supports ranges and that would have to be verified and handled.

https://httpwg.org/specs/rfc7540.html#SETTINGS_MAX_FRAME_SIZE

https://docs.rs/h2/latest/h2/server/struct.Builder.html#method.max_frame_size

https://docs.rs/h2/latest/h2/client/struct.Builder.html#method.max_frame_size

acmorrow · 2024-11-21T19:14:19Z

micro-rdk/src/common/ota.rs

+                if total_downloaded < core::mem::size_of::<EspAppDesc>() {
+                    log::error!("initial frame too small to retrieve esp_app_desc_t");
+                } else {
+                    log::info!("data length {}", data.len());


This looks like a debug log, at best, or better removed

acmorrow · 2024-11-21T19:17:22Z

micro-rdk/src/common/ota.rs

+                        let mut loader = EspFirmwareInfoLoader::new();
+                        loader.load(&data).map_err(OtaError::EspError)?;
+                        let new_fw = loader.get_info().map_err(OtaError::EspError)?;
+                        log::info!("current firmware: {:?}", running_fw_info);


Overall though, I do feel like this method should probably have some well thought out user facing logs that provide context about what the OTA system is doing.

acmorrow · 2024-11-21T19:19:56Z

micro-rdk/src/common/ota.rs

+                        log::info!("new firmware: {:?}", new_fw);
+                        if let Some(ref running_fw) = running_fw_info {
+                            if running_fw.version == new_fw.version
+                                && running_fw.released == new_fw.released


What is released here and what does it mean?

this is the compiled timestamp we talked about before

acmorrow · 2024-11-21T19:20:56Z

micro-rdk/src/common/conn/viam.rs

@@ -622,6 +600,42 @@ where
            }
        }

+        #[cfg(feature = "ota")]


Why do we need certs? I thought the certs were for us serving

gvaradarajan

LGTM

gvaradarajan · 2024-11-22T21:17:03Z

micro-rdk/src/common/grpc_client.rs

@@ -219,7 +219,7 @@ impl Future for AsyncioSleep {
 }

 #[derive(Default, Clone, Debug)]
-struct H2Timer;
+pub struct H2Timer;


pub(crate) instead?

mattjperez requested review from acmorrow and gvaradarajan November 14, 2024 17:18

mattjperez self-assigned this Nov 14, 2024

mattjperez requested a review from a team as a code owner November 14, 2024 17:18

mattjperez commented Nov 14, 2024

View reviewed changes

micro-rdk/src/common/ota.rs Outdated Show resolved Hide resolved

mattjperez commented Nov 14, 2024

View reviewed changes

micro-rdk/src/common/ota.rs Outdated Show resolved Hide resolved

mattjperez commented Nov 14, 2024

View reviewed changes

gvaradarajan requested changes Nov 19, 2024

View reviewed changes

mattjperez requested a review from gvaradarajan November 19, 2024 21:36

acmorrow requested changes Nov 21, 2024

View reviewed changes

mattjperez added 11 commits November 22, 2024 15:45

compiling async task and hyper::http2 client

6a317e2

with hyper http1

bc8fe5d

switch

758096e

wip, testing on native

4a4ae4d

almost done, missing async wrapper around ota handle

91a3f0c

cleanup old files

200cca5

cleanup

bb53628

add suggestions

2262da0

rm certs from client

cf15da2

cleanup

2228f50

fix conflict

f3ccf5c

mattjperez force-pushed the rsdk-9195-ota-async-task branch from d569cde to f3ccf5c Compare November 22, 2024 20:49

mattjperez requested a review from acmorrow November 22, 2024 20:49

mattjperez added 2 commits November 22, 2024 15:52

magic number

d63da42

clippy

42a8367

gvaradarajan approved these changes Nov 22, 2024

View reviewed changes

		bincode = "2.0.0-rc.3"

[RSDK-9195] ota async task #350

Are you sure you want to change the base?

[RSDK-9195] ota async task #350

Conversation

mattjperez commented Nov 14, 2024 • edited by jira bot Loading

mattjperez Nov 14, 2024 • edited Loading

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

mattjperez Nov 14, 2024 • edited Loading

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

gvaradarajan left a 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

gvaradarajan Nov 19, 2024 • edited Loading

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

mattjperez Nov 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez Nov 21, 2024 • edited Loading

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

gvaradarajan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjperez commented Nov 14, 2024 •

edited by jira bot

Loading

mattjperez Nov 14, 2024 •

edited

Loading

mattjperez Nov 14, 2024 •

edited

Loading

gvaradarajan Nov 19, 2024 •

edited

Loading

mattjperez Nov 19, 2024 •

edited

Loading

mattjperez Nov 21, 2024 •

edited

Loading