add suggestions, refactor

micro-rdk/src/common/config_monitor.rs

@@ -92,26 +92,16 @@ where
                             self.storage.clone(),
                             self.executor.clone(),
                         ) {
-                            Ok(mut ota) => {
-                                if ota.needs_update().await {
-                                    log::info!(
-                                        "firmware update to `{}` detected",
-                                        ota.pending_version(),
-                                    );
-                                    if let Err(ota_err) = ota.update().await {
-                                        log::error!("failed to update firmware: {}", ota_err);
-                                    };
-                                    if ota.needs_reboot() {
-                                        reboot = true;
-                                    }
-                                }
-                            }
-
-                            Err(e) => {
-                                log::error!("failed to build ota service: {}", e.to_string());
-                                log::error!("ota service config: {:?}", service);
-                            }
-                        };
+                            Ok(mut ota) => match ota.update().await {
+                                Ok(needs_reboot) => reboot = needs_reboot,
+                                Err(e) => log::error!("failed to complete ota update: {}", e),
+                            },
+                            Err(e) => log::error!(
+                                "failed to create ota service from config:{} - {:?}",
+                                e,
+                                service,
+                            ),
+                        }
                     }
                 }
 

micro-rdk/src/common/ota.rs

@@ -59,7 +59,8 @@ use thiserror::Error;
 #[cfg(not(feature = "esp32"))]
 use {bincode::Decode, futures_lite::AsyncWriteExt};
 
-const CONN_RETRY_SECS: u64 = 60;
+const CONN_RETRY_SECS: u64 = 1;
+const NUM_RETRY_CONN: usize = 3;
 const SIZEOF_APPDESC: usize = 256;
 const MAX_VER_LEN: usize = 128;
 pub const OTA_MODEL_TYPE: &str = "ota_service";
@@ -166,7 +167,6 @@ pub(crate) struct OtaService<S: OtaMetadataStorage> {
     pending_version: String,
     max_size: usize,
     address: usize,
-    needs_reboot: bool,
 }
 
 impl<S: OtaMetadataStorage> OtaService<S> {
@@ -181,10 +181,6 @@ impl<S: OtaMetadataStorage> OtaService<S> {
             .unwrap_or_default()
     }
 
-    pub(crate) fn needs_reboot(&self) -> bool {
-        self.needs_reboot
-    }
-
     pub(crate) fn from_config(
         new_config: &ServiceConfig,
         storage: S,
@@ -269,20 +265,18 @@ impl<S: OtaMetadataStorage> OtaService<S> {
             pending_version,
             max_size,
             address,
-            needs_reboot: false,
         })
     }
 
     pub(crate) async fn needs_update(&self) -> bool {
         self.stored_metadata().await.version != self.pending_version
     }
 
-    pub(crate) fn pending_version(&self) -> &str {
-        &self.pending_version
-    }
-    pub(crate) async fn update(&mut self) -> Result<(), OtaError> {
+    /// Attempts to perform an OTA update.
+    /// On success, returns an `Ok(true)` or `Ok(false)` indicating if a reboot is necessary.
+    pub(crate) async fn update(&mut self) -> Result<bool, OtaError> {
         if !(self.needs_update().await) {
-            return Ok(());
+            return Ok(false);
         }
 
         let mut uri = self
@@ -308,7 +302,14 @@ impl<S: OtaMetadataStorage> OtaService<S> {
             uri = hyper::Uri::from_parts(parts).map_err(|e| OtaError::Other(e.to_string()))?;
         };
 
+        let mut num_tries = 0;
         let (mut sender, conn) = loop {
+            num_tries += 1;
+            if num_tries == NUM_RETRY_CONN {
+                return Err(OtaError::Other(
+                    "failed to establish connection".to_string(),
+                ));
+            }
             match self.connector.connect_to(&uri) {
                 Ok(connection) => {
                     match connection.await {
@@ -536,8 +537,6 @@ impl<S: OtaMetadataStorage> OtaService<S> {
             log::info!("next reboot will load firmware from `{:#x}`", self.address);
         }
 
-        self.needs_reboot = true;
-
-        Ok(())
+        Ok(true)
     }
 }