[RSDK-8187] config cache changes #262
Conversation
| ServerError: From<<S as RobotConfigurationStorage>::Error>, | ||
| <S as WifiCredentialStorage>::Error: Sync + Send + 'static, | ||
| { | ||
| pub fn new(restart_hook: impl FnOnce() + 'a, curr_config: ConfigResponse, storage: S) -> Self { |
There was a problem hiding this comment.
Let's make the lambda come last.
| false => { | ||
| if let Err(e) = self | ||
| .storage | ||
| .store_robot_configuration(new_config.config.unwrap()) |
There was a problem hiding this comment.
I'm suspicious of unwrap. What should happen here if new_config.config is None?
There was a problem hiding this comment.
not sure, what happens in entry.rs when get_config is called and new_config is None or returns an AppClientError?
micro-rdk/micro-rdk/src/esp32/entry.rs
Lines 83 to 84 in f95d10b
There was a problem hiding this comment.
I'm guessing it panics?
There was a problem hiding this comment.
oh I see, I already new_config is already unwrapped
| S: RobotConfigurationStorage + WifiCredentialStorage + Clone + 'static, | ||
| <S as RobotConfigurationStorage>::Error: Debug, | ||
| ServerError: From<<S as RobotConfigurationStorage>::Error>, | ||
| <S as WifiCredentialStorage>::Error: Sync + Send + 'static, |
There was a problem hiding this comment.
This won't work unless feature = "provisioning" is enabled, but see #267 which offers a way out. Perhaps this change should wait on that one.
What about the following idea: when building from a cached config, we first fetch the config, and then we immediately erase it. Then we build the robot. Only if the robot builds OK do we write it back to the cache again. I don't love it because of the extra flash wear it implies, but that's already a problem we have. |
I discussed this idea with @npmenard and he feels like the flash wear is too much of a problem. Please move forward with committing this (once you have addressed the failure to build without provisioning), and file a new bug ticket about the situation that arises when a panic inducing config is cached. Then we can tackle that in a new PR. |
There was a problem hiding this comment.
This looks good for me (nice work on the generics in signatures), but definitely
- wait for new [RSDK-8219] allow micro-RDK to build w/out provisioning feature #267 to merge and retest compilation with and without provisioning
- make and reference a follow up ticket in TODO comment
| if let Some(config) = new_config.config { | ||
| if let Err(e) = self.storage.store_robot_configuration(config) { | ||
| log::warn!("Failed to store new robot configuration from app: {}", e); | ||
| } | ||
| } | ||
| self.restart(); |
There was a problem hiding this comment.
please add the crash loop bug as a TODO comment here once it's made.
| @@ -43,7 +69,12 @@ impl<'a> PeriodicAppClientTask for ConfigMonitor<'a> { | |||
| app_client.clone().get_config(None).await | |||
| { | |||
| if self.curr_config != *new_config { | |||
| self.restart() | |||
| if let Some(config) = new_config.config { | |||
| if let Err(e) = self.storage.store_robot_configuration(config) { | |||
There was a problem hiding this comment.
did we consider erasing the config then restarting?
There was a problem hiding this comment.
We could. That would leave open the possibility that if you can't connect when you come back online, you won't be able to build a robot until you talk to app again. That might be a risk worth taking.
There was a problem hiding this comment.
I think this could work, its either a empty config or an invalid config if you can't come back online which both wouldn't work. Should I implement this solution?
Added storage into the config_monitor in order to write newly detected configs to NVS before rebooting, preventing a reboot cycle where the config is out of date.
However, this introduced a new issue where some invalid configs would cause a reboot loop, as it would crash the robot before entry.rs could detect a robot error and reset_robot_configuration().
Some fixes could be: