Skip to content
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.

Sign up for GitHub

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

[RSDK-9593] move ota to config monitor, config monitor restart hook #369

Merged
merged 22 commits into from
Jan 7, 2025
Merged

[RSDK-9593] move ota to config monitor, config monitor restart hook #369

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f770c97
move ota to config_monitor, with retries, fix reset hook
mattjperez Dec 20, 2024
a9c61b6
rm reboot from ota::update
mattjperez Dec 20, 2024
26f1f6e
update comments
mattjperez Dec 20, 2024
986e3a3
fix reset hook
mattjperez Dec 20, 2024
d8e2ee6
fmt
mattjperez Dec 20, 2024
acb1e5e
add suggestions, rm retry loop, extra log
mattjperez Dec 20, 2024
adb66d4
clippy
mattjperez Dec 20, 2024
c2eebd8
add suggestions, refactor
mattjperez Dec 31, 2024
3600bff
add test flag to service config for ota loop
mattjperez Dec 31, 2024
b003612
Merge branch 'main' into rsdk-9593-config-monitor-ota
mattjperez Dec 31, 2024
9caf29c
add ticket comment
mattjperez Dec 31, 2024
42fceb5
rm ota test-loop flag
mattjperez Dec 31, 2024
e63c84b
fix loop bug
mattjperez Dec 31, 2024
c7b1b2e
wip
mattjperez Jan 3, 2025
532233a
Revert "wip"
mattjperez Jan 6, 2025
7444b59
rm async from ota helpers, chg firmware logging in viam.rs
mattjperez Jan 6, 2025
1a6bae6
add sugg
mattjperez Jan 6, 2025
a17c32c
propagate storage errors
mattjperez Jan 6, 2025
132a455
revert restart hook changes
mattjperez Jan 6, 2025
b23714c
shift increment to top of loop
mattjperez Jan 6, 2025
6f9685e
final changes
mattjperez Jan 7, 2025
2c233f4
fmt
mattjperez Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions micro-rdk/src/common/config_monitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ use crate::{
};
use async_io::Timer;
use futures_lite::{Future, FutureExt};
use std::fmt::Debug;
use std::pin::Pin;
use std::time::Duration;
use std::{fmt::Debug, pin::Pin, time::Duration};

#[cfg(feature = "ota")]
use crate::common::{exec::Executor, ota};

pub struct ConfigMonitor<'a, Storage> {
curr_config: Box<RobotConfig>, //config for robot gotten from last robot startup, aka inputted from entry
storage: Storage,
#[cfg(feature = "ota")]
executor: Executor,
restart_hook: Box<dyn Fn() + 'a>,
}

Expand All @@ -27,11 +30,14 @@ where
pub fn new(
curr_config: Box<RobotConfig>,
storage: Storage,
#[cfg(feature = "ota")] executor: Executor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inconsistent with how we usually do cfg things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt trips on it https://github.com/viamrobotics/micro-rdk/actions/runs/12655226567/job/35265116182?pr=369

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not ideal to have function parameters be dependent on features

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if either a) we decide to revert back to a separate ota task or b) if OTA becomes default and we remove the feagure-gate, we could remove this.

restart_hook: impl Fn() + 'a,
) -> Self {
Self {
curr_config,
storage,
#[cfg(feature = "ota")]
executor,
restart_hook: Box::new(restart_hook),
}
}
Expand Down Expand Up @@ -71,16 +77,47 @@ where
})
.await?;

if new_config
.config
.is_some_and(|cfg| cfg != *self.curr_config)
{
if let Err(e) = self.storage.reset_robot_configuration() {
log::warn!(
"Failed to reset robot config after new config detected: {}",
e
);
} else {
if let Some(config) = new_config.as_ref().config.as_ref() {
let mut reboot = false;

#[cfg(feature = "ota")]
{
if let Some(service) = config
.services
.iter()
.find(|&service| service.model == *ota::OTA_MODEL_TRIPLET)
{
match ota::OtaService::from_config(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We end up instantiating a new OtaService and doing a version check each time the ConfigMonitor triggers that's not super ideal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made a ticket, though may be a non-issue if we revert back to separate ota task after testing. RSDK-9676

service,
self.storage.clone(),
self.executor.clone(),
) {
Ok(mut ota) => match ota.update().await {
Ok(needs_reboot) => reboot = needs_reboot,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC at reboot the config retrieved offline overwrites the stored config, is there a particular reason for the ota to not reboot immediately on successful download?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not a likely scenario, but if app.viam becomes unreachable during the download period, at least it would be operating off of the new config at reboot.

we can iterate on it

Err(e) => log::error!("failed to complete ota update: {}", e),
},
Err(e) => log::error!(
"failed to create ota service from config:{} - {:?}",
e,
service,
),
}
}
}

if *config != *self.curr_config {
if let Err(e) = self.storage.reset_robot_configuration() {
log::warn!(
"Failed to reset robot config after new config detected: {}",
e
);
} else {
reboot = true;
}
}

if reboot {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks cleaner to use a flag instead of fiddling with feature gates

// TODO(RSDK-9464): flush logs to app.viam before restarting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ticket would be another reason to DRY the shutdown handling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in, have a global shutdown function that handles log-flushing?

self.restart();
}
}
Expand Down
60 changes: 17 additions & 43 deletions micro-rdk/src/common/conn/viam.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ use super::server::{IncomingConnectionManager, WebRtcConfiguration};
use crate::common::provisioning::server::AsNetwork;

#[cfg(feature = "ota")]
use crate::common::{credentials_storage::OtaMetadataStorage, ota};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this here for now instead of shifting it to config_monitor.rs. I like that it's part of ViamServerStorage, especially if we remove the flag entirely to make it a builtin

use crate::common::credentials_storage::OtaMetadataStorage;

pub struct RobotCloudConfig {
local_fqdn: String,
Expand Down Expand Up @@ -364,8 +364,6 @@ where
http2_server_port: self.http2_server_port,
wifi_manager: self.wifi_manager.into(),
app_client_tasks: self.app_client_tasks,
#[cfg(feature = "ota")]
ota_service_task: Default::default(),
max_concurrent_connections: self.max_concurrent_connections,
network: Some(network),
}
Expand Down Expand Up @@ -400,8 +398,6 @@ where
http2_server_port: self.http2_server_port,
wifi_manager: Rc::new(self.wifi_manager),
app_client_tasks: self.app_client_tasks,
#[cfg(feature = "ota")]
ota_service_task: None,
max_concurrent_connections: self.max_concurrent_connections,
network: None,
}
Expand All @@ -420,8 +416,6 @@ pub struct ViamServer<Storage, C, M> {
http2_server_port: u16,
wifi_manager: Rc<Option<Box<dyn WifiManager>>>,
app_client_tasks: Vec<Box<dyn PeriodicAppClientTask>>,
#[cfg(feature = "ota")]
ota_service_task: Option<Task<()>>,
max_concurrent_connections: usize,
network: Option<Box<dyn Network>>,
}
Expand Down Expand Up @@ -486,6 +480,14 @@ where
self.provision().await;
}

#[cfg(feature = "ota")]
{
if self.storage.has_ota_metadata() {
let metadata = self.storage.get_ota_metadata().unwrap_or_default();
log::info!("firmware version: {}", metadata.version);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will log the empty string if there is no OTA metadata in NVS. I'm not sure that's the best outcome. Can we be more explicit?

}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this here avoids complicating config monitor's ota stuff to get the right logging behavior.


// Since provisioning was run and completed, credentials are properly populated
// if wifi manager is configured loop forever until wifi is connected
if let Some(wifi) = self.wifi_manager.as_ref().as_ref() {
Expand Down Expand Up @@ -573,48 +575,20 @@ where
log::error!("couldn't store the robot configuration reason {:?}", err);
}

#[cfg(feature = "esp32")]
let hook = || crate::esp32::esp_idf_svc::hal::reset::restart();
#[cfg(not(feature = "esp32"))]
let hook = || std::process::exit(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Before we just called std::process::exit, but this goes back to an older style where on esp32 we do hal::reset::restart. What was the motivation for that change. I think I know, but I'd like to be sure.
  • If this is to effect the change I suspect, I think I'd want to see the same change in the RestartMonitor.
  • A block nearly identical to this exists in grpc::robot_shutdown. Can we DRY this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the motivation was to get rid of the coredump we see everytime on config change restarts since I'm already modifying the config_monitor's behavior.

I mention it in the description about partially addressing RSDK-9594, but just in config monitor.

I can look into grpc::robot_shutdown, but that depends if you want that done across the repo in RSDK-9594 or just here. Which would you prefer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per offline discussion, changing this back to just use std::process::exit. the right changes will be done in RSDK-9594


let config_monitor_task = Box::new(ConfigMonitor::new(
config.clone(),
self.storage.clone(),
|| std::process::exit(0),
#[cfg(feature = "ota")]
self.executor.clone(),
hook,
));
self.app_client_tasks.push(config_monitor_task);

#[cfg(feature = "ota")]
{
log::debug!("ota feature enabled");

if let Some(service) = config
.services
.iter()
.find(|&service| service.model == *ota::OTA_MODEL_TRIPLET)
{
match ota::OtaService::from_config(
service,
self.storage.clone(),
self.executor.clone(),
) {
Ok(mut ota) => {
self.ota_service_task
.replace(self.executor.spawn(async move {
if let Err(e) = ota.update().await {
log::error!("failed to complete ota update {}", e);
}
}));
}
Err(e) => {
log::error!("failed to build ota service: {}", e.to_string());
log::error!("ota service config: {:?}", service);
}
};
} else {
log::error!(
"ota enabled, but no service of type `{}` found in robot config",
ota::OTA_MODEL_TYPE
);
}
}

let mut robot = LocalRobot::from_cloud_config(
self.executor.clone(),
robot_creds.robot_id.clone(),
Expand Down
67 changes: 38 additions & 29 deletions micro-rdk/src/common/ota.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -156,9 +157,6 @@ impl OtaMetadata {
pub fn new(version: String) -> Self {
Self { version }
}
pub(crate) fn version(&self) -> &str {
&self.version
}
}

pub(crate) struct OtaService<S: OtaMetadataStorage> {
Expand All @@ -172,6 +170,17 @@ pub(crate) struct OtaService<S: OtaMetadataStorage> {
}

impl<S: OtaMetadataStorage> OtaService<S> {
pub(crate) async fn stored_metadata(&self) -> OtaMetadata {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function really async?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't, but shouldn't it be? The underlying calls are essentially I/O, getting data from flash storage. But this is maybe is more of a "should our storage APIs be async?" question.

I'll remove it here though.

if !self.storage.has_ota_metadata() {
log::info!("no ota metadata currently stored in NVS");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: OTA

}

self.storage
.get_ota_metadata()
.inspect_err(|e| log::warn!("failed to get ota metadata from nvs: {}", e))
.unwrap_or_default()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return a default value rather than an error?

}

pub(crate) fn from_config(
new_config: &ServiceConfig,
storage: S,
Expand Down Expand Up @@ -259,28 +268,17 @@ impl<S: OtaMetadataStorage> OtaService<S> {
})
}

pub(crate) async fn update(&mut self) -> Result<(), OtaError> {
let stored_metadata = if !self.storage.has_ota_metadata() {
log::info!("no ota metadata currently stored in NVS");
OtaMetadata::default()
} else {
self.storage
.get_ota_metadata()
.inspect_err(|e| log::warn!("failed to get ota metadata from nvs: {}", e))
.unwrap_or_default()
};
pub(crate) async fn needs_update(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function really async?

self.stored_metadata().await.version != self.pending_version
}

if self.pending_version == stored_metadata.version() {
log::info!("firmware is up-to-date: `{}`", stored_metadata.version);
return Ok(());
/// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly love the bool result. Maybe my unease is more to do with the name of the method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer a field in OtaService that the caller checks after calling update?

if !(self.needs_update().await) {
return Ok(false);
}

log::info!(
"firmware is out of date, proceeding with update from version `{}` to version `{}`",
stored_metadata.version,
self.pending_version
);

let mut uri = self
.url
.parse::<hyper::Uri>()
Expand All @@ -304,11 +302,18 @@ 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 {
if num_tries == NUM_RETRY_CONN {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a necessary part of this change? It seems independent of moving the driver of OTA checks to be the ConfigMonitor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't limit the number of connection attempts, it will block further config changes. So the change is to still have a limited number of short-lived retry attempts in case there's initial wonkiness with the connection.

If there is any general wonkiness with establishing the connection, there isn't any guarantee that returning immediately and depending on the next invocation of ConfigMonitor (in 10sec) will resolve it.

three one-second retries seemed like a decent middle ground that we can iterate on if anything.

return Err(OtaError::Other(
"failed to establish connection".to_string(),
));
}
match self.connector.connect_to(&uri) {
Ok(connection) => {
match connection.await {
Ok(io) => {
// TODO(RSDK-9617): add timeout for stalled download
match http2::Builder::new(self.exec.clone())
.max_frame_size(16_384) // lowest configurable
.timer(H2Timer)
Expand Down Expand Up @@ -337,6 +342,7 @@ impl<S: OtaMetadataStorage> OtaService<S> {
CONN_RETRY_SECS
);
Timer::after(Duration::from_secs(CONN_RETRY_SECS)).await;
num_tries += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Is there a way to frame this loop without an explicitly managed counter?
  • If you really need the counter, I'd recommend incrementing at or near the top so it can't somehow become skipped later.

Copy link
Member Author

@mattjperez mattjperez Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried exploring different paths for a day or two:

  • wrapping the connection initialization logic into its own function
    • getting the return type right (even with generics) was a rabbit hole of satisfying the compiler and enabling an unstable/nightly features that are used to define a lower-level trait (something with Allocator if I remember right)
  • declaring the variables outside the loop
    • same issue as above
  • using for i in 0..CONN_RETRY
    • can't use break in a for-loop, only in loop

I think I tried a couple other things, (like inspect,map_err, ok_and, etc) but it seemed like too much time spend on trying to avoid the messiness and have the correct behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move the increment to the top of the loop and set the range accordingly

};

let conn = Box::new(self.exec.spawn(async move {
Expand Down Expand Up @@ -520,15 +526,18 @@ impl<S: OtaMetadataStorage> OtaService<S> {
.map_err(|e| OtaError::Other(e.to_string()))?;

log::info!("firmware update complete");

// Test experimental ffi accesses here to be recoverable without flashing
// verifies nvs was stored correctly
let curr_metadata = self.stored_metadata().await;
log::info!(
"firmware update successful: version `{}`",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have this, do you still need the "firmware update complete" line above?

curr_metadata.version
);
// Note: test experimental ota ffi accesses here to be recoverable without flashing
#[cfg(feature = "esp32")]
{
log::info!("rebooting to load firmware from `{:#x}`", self.address);
// TODO(RSDK-9464): flush logs to app.viam before restarting
esp_idf_svc::hal::reset::restart();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shifts the responsibility of rebooting to whoever is calling ota::update()

log::info!("next reboot will load firmware from `{:#x}`", self.address);
}

Ok(())
Ok(true)
}
}
Loading