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 7 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
73 changes: 60 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,57 @@ 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) => {
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);
}
};
}
}

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
61 changes: 35 additions & 26 deletions micro-rdk/src/common/ota.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,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 @@ -169,9 +166,25 @@ pub(crate) struct OtaService<S: OtaMetadataStorage> {
pending_version: String,
max_size: usize,
address: usize,
needs_reboot: bool,
}

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 needs_reboot(&self) -> bool {
self.needs_reboot
}

pub(crate) fn from_config(
new_config: &ServiceConfig,
storage: S,
Expand Down Expand Up @@ -256,31 +269,22 @@ impl<S: OtaMetadataStorage> OtaService<S> {
pending_version,
max_size,
address,
needs_reboot: false,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to have update return a boolean as to whether an update was actually performed rather than store state?

})
}

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);
pub(crate) fn pending_version(&self) -> &str {
&self.pending_version
}
pub(crate) async fn update(&mut self) -> Result<(), OtaError> {
if !(self.needs_update().await) {
return Ok(());
}

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 Down Expand Up @@ -520,15 +524,20 @@ 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);
}

self.needs_reboot = true;

Ok(())
}
}
Loading