Repository: quartiq/stabilizer (language: rust)

Issue:
USB/flash `clear`/`save` behavior w.r.t. default changes
Once things have been saved to flash (also implicitly through e.g. `clear`), then changes in `Default` are not propagated. This makes the behavior with an empty flash (that has never seen `clear`) different from the one after `clear`. Usual expectation is that `clear` restores behavior to "what it was before/initially", arguably including transparent propagation/activation of changes to `Default` with app updates.

The proposed alternative is to use [`remove_item`](https://docs.rs/sequential-storage/latest/sequential_storage/map/fn.remove_item.html) in current `clear` and https://github.com/quartiq/stabilizer/issues/860

https://github.com/quartiq/stabilizer/pull/857#discussion_r1540132885


Discussion / hints:
@jordens Based on my understanding, if you change a `Default` in the application and re-run the `clear` command, you will receive the new `Default` value. Is this specifically what is not desired?

I'm not entirely seeing the difference between an uninitialized flash and one that is `clear`ed. Yes, it will have initial values in it that may or may not be `Default`, but the proposed change to use `remove_item` and the current implementation wouldn't have any different effect on the `clear` command or operation of the device.
I'd see the additional clear required as an work around.

The behavior is different. With the proposed change clear followed by app update also updates to new defaults (like sucessive app updates without ever touching USB).

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 729624e79..3abfe7f4d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 Booster
 * Panic information is now persisted after reboot and available via telemetry and the USB serial
 console.
+* Device operational settings can now be modified, stored and cleared from device flash using the
+  USB serial console. Run-time settings are unique to each application, but network settings are
+  unified for all applications (i.e. lockin, dual-iir, etc.)
 
 ### Changed
 * Broker and static IP/DHCP are no longer configured at compile time,
diff --git a/Cargo.lock b/Cargo.lock
index 70b48a220..7bb0d3fd4 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -201,6 +201,12 @@ dependencies = [
  "syn 2.0.66",
 ]
 
+[[package]]
+name = "embassy-futures"
+version = "0.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1f878075b9794c1e4ac788c95b728f26aa6366d32eeb10c7051389f898f7d067"
+
 [[package]]
 name = "embedded-dma"
 version = "0.2.0"
@@ -257,6 +263,15 @@ version = "0.3.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "a21dea9854beb860f3062d10228ce9b976da520a73474aed3171ec276bc0c032"
 
+[[package]]
+name = "embedded-storage-async"
+version = "0.4.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1763775e2323b7d5f0aa6090657f5e21cfa02ede71f5dc40eead06d64dcd15cc"
+dependencies = [
+ "embedded-storage",
+]
+
 [[package]]
 name = "embedded-time"
 version = "0.12.1"
@@ -1122,11 +1137,11 @@ checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3"
 
 [[package]]
 name = "sequential-storage"
-version = "0.6.2"
+version = "2.0.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "cc9294c2c7c725835fa69d5a144a29078159360bdc53593e5e977abf8d200f46"
+checksum = "8d25123e754473ed08bcc4e0ef61f0a733662872885e533ffdf03771119712fc"
 dependencies = [
- "embedded-storage",
+ "embedded-storage-async",
 ]
 
 [[package]]
@@ -1254,9 +1269,11 @@ dependencies = [
  "built",
  "cortex-m",
  "cortex-m-rt",
+ "embassy-futures",
  "embedded-hal 0.2.7",
  "embedded-io",
  "embedded-storage",
+ "embedded-storage-async",
  "enum-iterator",
  "fugit",
  "heapless 0.7.17",
diff --git a/Cargo.toml b/Cargo.toml
index d50b2562f..98d8785d7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -33,9 +33,10 @@ members = ["ad9959", "serial-settings"]
 
 [dependencies]
 panic-persist = { version = "0.3", features = ["utf8", "custom-panic-handler"] }
-sequential-storage = "0.6"
+sequential-storage = "2"
 embedded-io = "0.6"
 embedded-storage = "0.3"
+embedded-storage-async = "0.4"
 cortex-m = { version = "0.7.7", features = ["inline-asm", "critical-section-single-core"] }
 cortex-m-rt = { version = "0.7", features = ["device"] }
 log = { version = "0.4", features = ["max_level_trace", "release_max_level_info"] }
@@ -72,6 +73,7 @@ tca9539 = "0.2"
 smoltcp-nal = { version = "0.5", features = ["shared-stack"] }
 postcard = "1"
 bit_field = "0.10.2"
+embassy-futures = { version = "0.1", default-features = false }
 
 [build-dependencies]
 built = { version = "0.7", features = ["git2"], default-features = false }
diff --git a/serial-settings/src/lib.rs b/serial-settings/src/lib.rs
index 673ac4a67..13b583d1b 100644
--- a/serial-settings/src/lib.rs
+++ b/serial-settings/src/lib.rs
@@ -91,6 +91,17 @@ pub trait Platform<const Y: usize>: Sized {
     /// Execute a platform specific command.
     fn cmd(&mut self, cmd: &str);
 
+    /// Handle clearing a settings key.
+    ///
+    /// # Note
+    /// The run-time setting will have already been updated when this is called. This is intended
+    /// to remove settings from permanent storage if necessary.
+    ///
+    /// # Arguments
+    /// * `buffer` The element serialization buffer.
+    /// * `key` The name of the setting to be cleared. If `None`, all settings are cleared.
+    fn clear(&mut self, buffer: &mut [u8], key: Option<&str>);
+
     /// Return a mutable reference to the `Interface`.
     fn interface_mut(&mut self) -> &mut Self::Interface;
 }
@@ -225,15 +236,7 @@ impl<'a, P: Platform<Y>, const Y: usize> Interface<'a, P, Y> {
             writeln!(interface, "All settings cleared").unwrap();
         }
 
-        match interface.platform.save(interface.buffer, maybe_key, settings) {
-            Ok(_) => {
-                writeln!(interface, "Settings saved. You may need to reboot for the settings to be applied")
-            }
-            Err(e) => {
-                writeln!(interface, "Failed to clear settings: {e:?}")
-            }
-        }
-        .unwrap();
+        interface.platform.clear(interface.buffer, maybe_key);
     }
 
     fn handle_get(
diff --git a/src/hardware/flash.rs b/src/hardware/flash.rs
index e40639b70..b603caa8c 100644
--- a/src/hardware/flash.rs
+++ b/src/hardware/flash.rs
@@ -1,5 +1,8 @@
+use embedded_storage::nor_flash::{ErrorType, NorFlash, ReadNorFlash};
 use stm32h7xx_hal::flash::LockedFlashBank;
 
+use stm32h7xx_hal::flash::Error as FlashError;
+
 pub struct Flash(pub LockedFlashBank);
 
 impl Flash {
@@ -8,9 +11,8 @@ impl Flash {
     }
 }
 
-impl embedded_storage::nor_flash::ErrorType for Flash {
-    type Error =
-        <LockedFlashBank as embedded_storage::nor_flash::ErrorType>::Error;
+impl ErrorType for Flash {
+    type Error = FlashError;
 }
 
 impl embedded_storage::nor_flash::ReadNorFlash for Flash {
@@ -20,7 +22,23 @@ impl embedded_storage::nor_flash::ReadNorFlash for Flash {
         &mut self,
         offset: u32,
         bytes: &mut [u8],
-    ) -> Result<(), Self::Error> {
+    ) -> Result<(), FlashError> {
+        self.0.read(offset, bytes)
+    }
+
+    fn capacity(&self) -> usize {
+        self.0.capacity()
+    }
+}
+
+impl embedded_storage_async::nor_flash::ReadNorFlash for Flash {
+    const READ_SIZE: usize = LockedFlashBank::READ_SIZE;
+
+    async fn read(
+        &mut self,
+        offset: u32,
+        bytes: &mut [u8],
+    ) -> Result<(), FlashError> {
         self.0.read(offset, bytes)
     }
 
@@ -45,3 +63,24 @@ impl embedded_storage::nor_flash::NorFlash for Flash {
         bank.write(offset, bytes)
     }
 }
+
+impl embedded_storage_async::nor_flash::NorFlash for Flash {
+    const WRITE_SIZE: usize =
+        stm32h7xx_hal::flash::UnlockedFlashBank::WRITE_SIZE;
+    const ERASE_SIZE: usize =
+        stm32h7xx_hal::flash::UnlockedFlashBank::ERASE_SIZE;
+
+    async fn erase(&mut self, from: u32, to: u32) -> Result<(), FlashError> {
+        let mut bank = self.0.unlocked();
+        bank.erase(from, to)
+    }
+
+    async fn write(
+        &mut self,
+        offset: u32,
+        bytes: &[u8],
+    ) -> Result<(), FlashError> {
+        let mut bank = self.0.unlocked();
+        bank.write(offset, bytes)
+    }
+}
diff --git a/src/settings.rs b/src/settings.rs
index 0838a9ed2..51aa3ae48 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -26,7 +26,6 @@ use crate::hardware::{flash::Flash, metadata::ApplicationMetadata, platform};
 use core::fmt::Write;
 use heapless::{String, Vec};
 use miniconf::{JsonCoreSlash, Postcard, Tree};
-use sequential_storage::map;
 use serial_settings::{BestEffortInterface, Platform, Settings};
 use smoltcp_nal::smoltcp::wire::EthernetAddress;
 use stm32h7xx_hal::flash::LockedFlashBank;
@@ -50,7 +49,7 @@ pub struct NetSettings {
 
 impl NetSettings {
     pub fn new(mac: EthernetAddress) -> Self {
-        let mut id = heapless::String::new();
+        let mut id = String::new();
         write!(&mut id, "{mac}").unwrap();
 
         Self {
@@ -80,11 +79,14 @@ pub fn load_from_flash<T: for<'d> JsonCoreSlash<'d, Y>, const Y: usize>(
         let path = path.unwrap();
 
         // Try to fetch the setting from flash.
-        let item = match map::fetch_item::<SettingsItem, _>(
-            storage,
-            storage.range(),
-            &mut buffer,
-            path.clone(),
+        let item = match embassy_futures::block_on(
+            sequential_storage::map::fetch_item::<SettingsKey, SettingsItem, _>(
+                storage,
+                storage.range(),
+                &mut sequential_storage::cache::NoCache::new(),
+                &mut buffer,
+                SettingsKey(path.clone()),
+            ),
         ) {
             Err(e) => {
                 log::warn!("Failed to fetch `{path}` from flash: {e:?}");
@@ -94,9 +96,15 @@ pub fn load_from_flash<T: for<'d> JsonCoreSlash<'d, Y>, const Y: usize>(
             _ => continue,
         };
 
+        // An empty vector may be saved to flash to "erase" a setting, since the H7 doesn't support
+        // multi-write NOR flash. If we see an empty vector, ignore this entry.
+        if item.0.is_empty() {
+            continue;
+        }
+
         log::info!("Loading initial `{path}` from flash");
 
-        let flavor = postcard::de_flavors::Slice::new(&item.data);
+        let flavor = postcard::de_flavors::Slice::new(&item.0);
         if let Err(e) =
             structure.set_postcard_by_key(path.split('/').skip(1), flavor)
         {
@@ -105,27 +113,63 @@ pub fn load_from_flash<T: for<'d> JsonCoreSlash<'d, Y>, const Y: usize>(
     }
 }
 
-#[derive(Default, serde::Serialize, serde::Deserialize)]
-pub struct SettingsItem {
-    // We only make these owned vec/string to get around lifetime limitations.
-    pub path: String<64>,
-    pub data: Vec<u8, 256>,
-}
-
-impl map::StorageItem for SettingsItem {
-    type Key = String<64>;
-    type Error = postcard::Error;
+#[derive(
+    Default, serde::Serialize, serde::Deserialize, Clone, PartialEq, Eq,
+)]
+pub struct SettingsKey(String<64>);
+
+impl sequential_storage::map::Key for SettingsKey {
+    fn serialize_into(
+        &self,
+        buffer: &mut [u8],
+    ) -> Result<usize, sequential_storage::map::SerializationError> {
+        Ok(postcard::to_slice(self, buffer)
+            .map_err(|_| {
+                sequential_storage::map::SerializationError::BufferTooSmall
+            })?
+            .len())
+    }
 
-    fn serialize_into(&self, buffer: &mut [u8]) -> Result<usize, Self::Error> {
-        Ok(postcard::to_slice(self, buffer)?.len())
+    fn deserialize_from(
+        buffer: &[u8],
+    ) -> Result<(Self, usize), sequential_storage::map::SerializationError>
+    {
+        let original_length = buffer.len();
+        let (result, remainder) =
+            postcard::take_from_bytes(buffer).map_err(|_| {
+                sequential_storage::map::SerializationError::BufferTooSmall
+            })?;
+        Ok((result, original_length - remainder.len()))
     }
+}
+
+#[derive(
+    Default, serde::Serialize, serde::Deserialize, Clone, PartialEq, Eq,
+)]
+pub struct SettingsItem(Vec<u8, 256>);
+
+impl<'a> sequential_storage::map::Value<'a> for SettingsItem {
+    fn serialize_into(
+        &self,
+        buffer: &mut [u8],
+    ) -> Result<usize, sequential_storage::map::SerializationError> {
+        if buffer.len() < self.0.len() {
+            return Err(
+                sequential_storage::map::SerializationError::BufferTooSmall,
+            );
+        }
 
-    fn deserialize_from(buffer: &[u8]) -> Result<Self, Self::Error> {
-        postcard::from_bytes(buffer)
+        buffer[..self.0.len()].copy_from_slice(&self.0);
+        Ok(self.0.len())
     }
 
-    fn key(&self) -> Self::Key {
-        self.path.clone()
+    fn deserialize_from(
+        buffer: &'a [u8],
+    ) -> Result<Self, sequential_storage::map::SerializationError> {
+        let vec = Vec::from_slice(buffer).map_err(|_| {
+            sequential_storage::map::SerializationError::BufferTooSmall
+        })?;
+        Ok(Self(vec))
     }
 }
 
@@ -171,55 +215,61 @@ where
         settings: &Self::Settings,
     ) -> Result<(), Self::Error> {
         let mut save_setting = |path| -> Result<(), Self::Error> {
-            let mut item = SettingsItem {
-                path,
-                ..Default::default()
-            };
+            let path = SettingsKey(path);
 
-            item.data.resize(item.data.capacity(), 0).unwrap();
-
-            let flavor = postcard::ser_flavors::Slice::new(&mut item.data);
+            let mut data = Vec::new();
+            data.resize(data.capacity(), 0).unwrap();
+            let flavor = postcard::ser_flavors::Slice::new(&mut data);
 
             let len = match settings
-                .get_postcard_by_key(item.path.split('/').skip(1), flavor)
+                .get_postcard_by_key(path.0.split('/').skip(1), flavor)
             {
                 Err(e) => {
-                    log::warn!(
-                        "Failed to save `{}` to flash: {e:?}",
-                        item.path
-                    );
+                    log::warn!("Failed to save `{}` to flash: {e:?}", path.0);
                     return Ok(());
                 }
                 Ok(slice) => slice.len(),
             };
-            item.data.truncate(len);
+            data.truncate(len);
 
             let range = self.storage.range();
 
             // Check if the settings has changed from what's currently in flash (or if it doesn't
             // yet exist).
-            if map::fetch_item::<SettingsItem, _>(
+            if embassy_futures::block_on(sequential_storage::map::fetch_item::<
+                SettingsKey,
+                SettingsItem,
+                _,
+            >(
                 &mut self.storage,
                 range.clone(),
+                &mut sequential_storage::cache::NoCache::new(),
                 buf,
-                item.path.clone(),
-            )
+                path.clone(),
+            ))
             .unwrap()
-            .map(|old| old.data != item.data)
+            .map(|old| old.0 != data)
             .unwrap_or(true)
             {
-                log::info!("Storing `{}` to flash", item.path);
-                map::store_item(&mut self.storage, range, buf, item).unwrap();
+                log::info!("Storing `{}` to flash", path.0);
+                embassy_futures::block_on(sequential_storage::map::store_item(
+                    &mut self.storage,
+                    range,
+                    &mut sequential_storage::cache::NoCache::new(),
+                    buf,
+                    path,
+                    &SettingsItem(data),
+                ))
+                .unwrap();
             }
 
             Ok(())
         };
 
         if let Some(key) = key {
-            save_setting(heapless::String::from(key))?;
+            save_setting(String::from(key))?;
         } else {
-            for path in Self::Settings::iter_paths::<heapless::String<64>>("/")
-            {
+            for path in Self::Settings::iter_paths::<String<64>>("/") {
                 save_setting(path.unwrap())?;
             }
         }
@@ -278,4 +328,56 @@ where
     fn interface_mut(&mut self) -> &mut Self::Interface {
         &mut self.interface
     }
+
+    fn clear(&mut self, buf: &mut [u8], key: Option<&str>) {
+        let mut erase_setting = |path| -> Result<(), Self::Error> {
+            let path = SettingsKey(path);
+            let range = self.storage.range();
+
+            // Check if there's an entry for this item in our flash map. The item might be a
+            // sentinel value indicating "erased". Because we can't write flash memory twice, we
+            // instead append a sentry "erased" value to the map where the serialized value is
+            // empty.
+            let maybe_item = embassy_futures::block_on(
+                sequential_storage::map::fetch_item::<
+                    SettingsKey,
+                    SettingsItem,
+                    _,
+                >(
+                    &mut self.storage,
+                    range.clone(),
+                    &mut sequential_storage::cache::NoCache::new(),
+                    buf,
+                    path.clone(),
+                ),
+            )
+            .unwrap();
+
+            // An entry may exist in the map with no data as a sentinel that this path was
+            // previously erased. If we find this, there's no need to store a duplicate "item is
+            // erased" sentinel in flash. We only need to logically erase the path from the map if
+            // it existed there in the first place.
+            if matches!(maybe_item, Some(item) if !item.0.is_empty()) {
+                embassy_futures::block_on(sequential_storage::map::store_item(
+                    &mut self.storage,
+                    range,
+                    &mut sequential_storage::cache::NoCache::new(),
+                    buf,
+                    path,
+                    &SettingsItem(Vec::new()),
+                ))
+                .unwrap();
+            }
+
+            Ok(())
+        };
+
+        if let Some(key) = key {
+            erase_setting(key.into()).unwrap();
+        } else {
+            for path in Self::Settings::iter_paths::<String<64>>("/") {
+                erase_setting(path.unwrap()).unwrap();
+            }
+        }
+    }
 }


Test patch: