From 02798d90c43b740d213d069206c83120a8443b08 Mon Sep 17 00:00:00 2001 From: Djeeberjr Date: Mon, 3 Nov 2025 18:35:10 +0100 Subject: [PATCH] improved error handeling for sdcard functions --- src/init/sd_card.rs | 245 +++++++++++++++++++++--------------- src/main.rs | 1 + src/store/id_store.rs | 18 +-- src/store/mapping_loader.rs | 8 +- src/store/persistence.rs | 14 ++- src/webserver/api.rs | 50 ++++++-- 6 files changed, 206 insertions(+), 130 deletions(-) diff --git a/src/init/sd_card.rs b/src/init/sd_card.rs index e883360..dba3d5f 100644 --- a/src/init/sd_card.rs +++ b/src/init/sd_card.rs @@ -1,10 +1,12 @@ -use core::str::from_utf8; - use alloc::{string::ToString, vec::Vec}; +use core::str::from_utf8; use embassy_time::Delay; use embedded_hal_bus::spi::ExclusiveDevice; -use embedded_sdmmc::{SdCard, ShortFileName, TimeSource, Timestamp, VolumeIdx, VolumeManager}; +use embedded_sdmmc::{ + SdCard, SdCardError, ShortFileName, TimeSource, Timestamp, VolumeIdx, VolumeManager, +}; use esp_hal::{Blocking, gpio::Output, spi::master::Spi}; +use thiserror::Error; use crate::store::{ AttendanceDay, day::Day, mapping_loader::Name, persistence::Persistence, tally_id::TallyID, @@ -45,190 +47,229 @@ pub struct SDCardPersistence { impl SDCardPersistence { const MAPPING_DIRNAME: &'static str = "MAPPINGS"; - fn generate_filename_for_day(day: Day) -> ShortFileName { + fn generate_filename_for_day(day: Day) -> Result { let basename = day.to_string(); let mut filename: heapless::String<11> = heapless::String::new(); - filename.push_str(&basename).unwrap(); - filename.push_str(".JS").unwrap(); + filename + .push_str(&basename) + .map_err(|_| PersistenceError::DayFilename)?; + filename + .push_str(".JS") + .map_err(|_| PersistenceError::DayFilename)?; - ShortFileName::create_from_str(&filename).unwrap() + ShortFileName::create_from_str(&filename).map_err(|_| PersistenceError::DayFilename) } - fn generate_path_for_id(id: TallyID) -> (ShortFileName, ShortFileName) { + fn generate_path_for_id( + id: TallyID, + ) -> Result<(ShortFileName, ShortFileName), PersistenceError> { let basename: heapless::String<12> = id.into(); let (dir, file) = basename.split_at(6); let mut filename: heapless::String<11> = heapless::String::new(); - filename.push_str(file).unwrap(); - filename.push_str(".JS").unwrap(); + filename + .push_str(file) + .map_err(|_| PersistenceError::IDFilename)?; + filename + .push_str(".JS") + .map_err(|_| PersistenceError::IDFilename)?; let mut dirname: heapless::String<11> = heapless::String::new(); - dirname.push_str(dir).unwrap(); + dirname + .push_str(dir) + .map_err(|_| PersistenceError::IDFilename)?; - ( - ShortFileName::create_from_str(&dirname).unwrap(), - ShortFileName::create_from_str(&filename).unwrap(), - ) + Ok(( + ShortFileName::create_from_str(&dirname).map_err(|_| PersistenceError::IDFilename)?, + ShortFileName::create_from_str(&filename).map_err(|_| PersistenceError::IDFilename)?, + )) } - fn get_tallyid_from_path(dirname: &ShortFileName, filename: &ShortFileName) -> Option { + fn get_tallyid_from_path( + dirname: &ShortFileName, + filename: &ShortFileName, + ) -> Result { let mut id_str: heapless::String<12> = heapless::String::new(); - id_str.push_str(&dirname.to_string()).unwrap(); id_str - .push_str(from_utf8(filename.base_name()).unwrap()) - .unwrap(); + .push_str(&dirname.to_string()) + .map_err(|_| PersistenceError::IDFilename)?; + id_str + .push_str(from_utf8(filename.base_name()).map_err(|_| PersistenceError::IDFilename)?) + .map_err(|_| PersistenceError::IDFilename)?; - let id: TallyID = id_str.try_into().unwrap(); + let id: TallyID = id_str + .try_into() + .map_err(|_| PersistenceError::IDFilename)?; - Some(id) + Ok(id) + } +} + +#[derive(Error, Debug)] +pub enum PersistenceError { + #[error("Failed to interact with SD card")] + SdCard(embedded_sdmmc::Error), + + #[error("Failed to parse data")] + Parseing(#[from] serde_json::Error), + + #[error("Failed to parse Day and Filename")] + DayFilename, + + #[error("Failed to parse TallyID for file path")] + IDFilename, + + #[error("Item not found")] + NotFound, +} + +impl From> for PersistenceError { + fn from(err: embedded_sdmmc::Error) -> Self { + PersistenceError::SdCard(err) } } impl Persistence for SDCardPersistence { - async fn load_day(&mut self, day: Day) -> Option { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); + type Error = PersistenceError; - let filename = Self::generate_filename_for_day(day); + async fn load_day(&mut self, day: Day) -> Result { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; + + let filename = Self::generate_filename_for_day(day)?; let file = root_dir.open_file_in_dir(filename, embedded_sdmmc::Mode::ReadOnly); if file.is_err() { - return None; + return Err(PersistenceError::NotFound); } - let mut open_file = file.unwrap(); + let mut open_file = file?; let mut read_buffer: [u8; 1024] = [0; 1024]; - let read = open_file.read(&mut read_buffer).unwrap(); - open_file.close().unwrap(); + let read = open_file.read(&mut read_buffer)?; + open_file.close()?; - let day: AttendanceDay = serde_json::from_slice(&read_buffer[..read]).unwrap(); + let day: AttendanceDay = serde_json::from_slice(&read_buffer[..read])?; - Some(day) + Ok(day) } - async fn save_day(&mut self, day: Day, data: &AttendanceDay) { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); + async fn save_day(&mut self, day: Day, data: &AttendanceDay) -> Result<(), Self::Error> { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; - let filename = Self::generate_filename_for_day(day); + let filename = Self::generate_filename_for_day(day)?; - let mut file = root_dir - .open_file_in_dir(filename, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) - .unwrap(); + let mut file = + root_dir.open_file_in_dir(filename, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate)?; - file.write(&serde_json::to_vec(data).unwrap()).unwrap(); + file.write(&serde_json::to_vec(data)?)?; - file.flush().unwrap(); - file.close().unwrap(); + file.flush()?; + file.close()?; + + Ok(()) } - async fn list_days(&mut self) -> Vec { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); + async fn list_days(&mut self) -> Result, Self::Error> { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; - let mut days_dir = root_dir.open_dir(".").unwrap(); + let mut days_dir = root_dir.open_dir(".")?; let mut days: Vec = Vec::new(); - days_dir - .iterate_dir(|e| { - let filename = e.name.clone(); + days_dir.iterate_dir(|e| { + let filename = e.name.clone(); - if let Ok(day) = filename.try_into() { - days.push(day); - } - }) - .unwrap(); + if let Ok(day) = filename.try_into() { + days.push(day); + } + })?; - days + Ok(days) } async fn load_mapping_for_id( &mut self, id: crate::store::tally_id::TallyID, - ) -> Option { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); - let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME).unwrap(); + ) -> Result { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; + let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME)?; - let (dirname, filename) = Self::generate_path_for_id(id); + let (dirname, filename) = Self::generate_path_for_id(id)?; - let mut dir = mapping_dir.open_dir(dirname).unwrap(); - let mut file = dir - .open_file_in_dir(filename, embedded_sdmmc::Mode::ReadOnly) - .unwrap(); + let mut dir = mapping_dir.open_dir(dirname)?; + let mut file = dir.open_file_in_dir(filename, embedded_sdmmc::Mode::ReadOnly)?; let mut read_buffer: [u8; 1024] = [0; 1024]; - let read_bytes = file.read(&mut read_buffer).unwrap(); - file.close().unwrap(); + let read_bytes = file.read(&mut read_buffer)?; + file.close()?; - let mapping: Name = serde_json::from_slice(&read_buffer[..read_bytes]).unwrap(); + let mapping: Name = serde_json::from_slice(&read_buffer[..read_bytes])?; - Some(mapping) + Ok(mapping) } async fn save_mapping_for_id( &mut self, id: crate::store::tally_id::TallyID, name: crate::store::mapping_loader::Name, - ) { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); - let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME).unwrap(); + ) -> Result<(), Self::Error> { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; + let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME)?; - let (dirname, filename) = Self::generate_path_for_id(id); + let (dirname, filename) = Self::generate_path_for_id(id)?; let mut dir = if let Ok(dir) = mapping_dir.open_dir(&dirname) { dir } else { - mapping_dir.make_dir_in_dir(&dirname).unwrap(); - mapping_dir.open_dir(&dirname).unwrap() + mapping_dir.make_dir_in_dir(&dirname)?; + mapping_dir.open_dir(&dirname)? }; - let mut file = dir - .open_file_in_dir(filename, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) - .unwrap(); + let mut file = + dir.open_file_in_dir(filename, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate)?; - file.write(&serde_json::to_vec(&name).unwrap()).unwrap(); + file.write(&serde_json::to_vec(&name)?)?; + + Ok(()) } - async fn list_mappings(&mut self) -> Vec { - let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0)).unwrap(); - let mut root_dir = vol_0.open_root_dir().unwrap(); - let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME).unwrap(); + async fn list_mappings(&mut self) -> Result, Self::Error> { + let mut vol_0 = self.vol_mgr.open_volume(VolumeIdx(0))?; + let mut root_dir = vol_0.open_root_dir()?; + let mut mapping_dir = root_dir.open_dir(Self::MAPPING_DIRNAME)?; let mut ids: Vec = Vec::new(); let mut dir_names = Vec::new(); - mapping_dir - .iterate_dir(|entry| { - if entry.attributes.is_directory() - && entry.name.to_string() != "." - && entry.name.to_string() != ".." - { - dir_names.push(entry.name.clone()); - } - }) - .unwrap(); + mapping_dir.iterate_dir(|entry| { + if entry.attributes.is_directory() + && entry.name.to_string() != "." + && entry.name.to_string() != ".." + { + dir_names.push(entry.name.clone()); + } + })?; for dirname in dir_names { if let Ok(mut subdir) = mapping_dir.open_dir(&dirname) { let mut file_names = Vec::new(); - subdir - .iterate_dir(|file_entry| { - if !file_entry.attributes.is_directory() { - file_names.push(file_entry.name.clone()); - } - }) - .unwrap(); + subdir.iterate_dir(|file_entry| { + if !file_entry.attributes.is_directory() { + file_names.push(file_entry.name.clone()); + } + })?; for filename in file_names { - let id = Self::get_tallyid_from_path(&dirname, &filename); - ids.push(id.unwrap()); + let id = Self::get_tallyid_from_path(&dirname, &filename)?; + ids.push(id); } } } - ids + Ok(ids) } } diff --git a/src/main.rs b/src/main.rs index 5c7f638..9d7fa73 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,7 @@ #![no_main] #![feature(type_alias_impl_trait)] #![feature(impl_trait_in_assoc_type)] +#![warn(clippy::unwrap_used)] use alloc::rc::Rc; use embassy_executor::Spawner; diff --git a/src/store/id_store.rs b/src/store/id_store.rs index ce4433d..3a62029 100644 --- a/src/store/id_store.rs +++ b/src/store/id_store.rs @@ -58,7 +58,7 @@ impl IDStore { } } - async fn persist_day(&mut self) { + async fn persist_day(&mut self) -> Result<(), T::Error> { self.persistence_layer .lock() .await @@ -88,21 +88,25 @@ impl IDStore { } /// Load and return a AttendanceDay. Nothing more. Nothing less. - pub async fn load_day(&mut self, day: Day) -> Option { + pub async fn load_day(&mut self, day: Day) -> Result { if day == self.current_day.date { - return Some(self.current_day.clone()); + return Ok(self.current_day.clone()); } self.persistence_layer.lock().await.load_day(day).await } - pub async fn list_days_in_timespan(&mut self, from: Day, to: Day) -> Vec { - let all_days = self.persistence_layer.lock().await.list_days().await; + pub async fn list_days_in_timespan( + &mut self, + from: Day, + to: Day, + ) -> Result, T::Error> { + let all_days = self.persistence_layer.lock().await.list_days().await?; - all_days + Ok(all_days .into_iter() .filter(|e| *e >= from) .filter(|e| *e <= to) - .collect() + .collect()) } } diff --git a/src/store/mapping_loader.rs b/src/store/mapping_loader.rs index dc09441..3c29cce 100644 --- a/src/store/mapping_loader.rs +++ b/src/store/mapping_loader.rs @@ -17,15 +17,15 @@ impl MappingLoader { Self(persistence_layer) } - pub async fn get_mapping(&self, id: TallyID) -> Option { + pub async fn get_mapping(&self, id: TallyID) -> Result { self.0.lock().await.load_mapping_for_id(id).await } - pub async fn set_mapping(&self, id: TallyID, name: Name) { - self.0.lock().await.save_mapping_for_id(id, name).await; + pub async fn set_mapping(&self, id: TallyID, name: Name) -> Result<(), T::Error> { + self.0.lock().await.save_mapping_for_id(id, name).await } - pub async fn list_mappings(&self) -> Vec { + pub async fn list_mappings(&self) -> Result,T::Error> { self.0.lock().await.list_mappings().await } } diff --git a/src/store/persistence.rs b/src/store/persistence.rs index 3513eff..58409e9 100644 --- a/src/store/persistence.rs +++ b/src/store/persistence.rs @@ -3,11 +3,13 @@ use alloc::vec::Vec; use crate::store::{day::Day, id_store::AttendanceDay, mapping_loader::Name, tally_id::TallyID}; pub trait Persistence { - async fn load_day(&mut self, day: Day) -> Option; - async fn save_day(&mut self, day: Day, data: &AttendanceDay); - async fn list_days(&mut self) -> Vec; + type Error: core::error::Error; - async fn load_mapping_for_id(&mut self, id:TallyID ) -> Option; - async fn save_mapping_for_id(&mut self, id:TallyID, name: Name); - async fn list_mappings(&mut self) -> Vec; + async fn load_day(&mut self, day: Day) -> Result; + async fn save_day(&mut self, day: Day, data: &AttendanceDay) -> Result<(), Self::Error>; + async fn list_days(&mut self) -> Result, Self::Error>; + + async fn load_mapping_for_id(&mut self, id: TallyID) -> Result; + async fn save_mapping_for_id(&mut self, id: TallyID, name: Name) -> Result<(), Self::Error>; + async fn list_mappings(&mut self) -> Result, Self::Error>; } diff --git a/src/webserver/api.rs b/src/webserver/api.rs index 9ffcbae..5308d0d 100644 --- a/src/webserver/api.rs +++ b/src/webserver/api.rs @@ -34,19 +34,34 @@ pub struct QueryMapping { } // GET /api/mappings -pub async fn get_mappings(State(state): State) -> impl IntoResponse { +pub async fn get_mappings( + State(state): State, +) -> Result { let loader = state.mapping_loader.lock().await; - response::Json(loader.list_mappings().await) + + match loader.list_mappings().await { + Ok(ids) => Ok(response::Json(ids)), + Err(_) => Err(( + response::StatusCode::INTERNAL_SERVER_ERROR, + "INTERNAL_SERVER_ERROR", + )), + } } // GET /api/mapping pub async fn get_mapping( State(state): State, Query(QueryMapping { id }): Query, -) -> impl IntoResponse { +) -> Result { let loader = state.mapping_loader.lock().await; - let mapping = loader.get_mapping(id).await; - response::Json(mapping) + + match loader.get_mapping(id).await { + Ok(name) => Ok(response::Json(name)), + Err(_) => Err(( + response::StatusCode::INTERNAL_SERVER_ERROR, + "INTERNAL_SERVER_ERROR", + )), + } } // POST /api/mapping @@ -55,7 +70,13 @@ pub async fn add_mapping( Json(data): Json, ) -> impl IntoResponse { let loader = state.mapping_loader.lock().await; - loader.set_mapping(data.id, data.name).await; + match loader.set_mapping(data.id, data.name).await { + Ok(_) => (response::StatusCode::CREATED, ""), + Err(_) => ( + response::StatusCode::INTERNAL_SERVER_ERROR, + "INTERNAL_SERVER_ERROR", + ), + } } // SSE /api/idevent @@ -84,9 +105,13 @@ pub async fn get_days( let mut store = state.store.lock().await; - let days = store.list_days_in_timespan(from_day, to_day).await; - - response::Json(days) + match store.list_days_in_timespan(from_day, to_day).await { + Ok(days) => Ok(response::Json(days)), + Err(_) => Err(( + response::StatusCode::INTERNAL_SERVER_ERROR, + "Internal server error", + )), + } } // GET /api/day @@ -102,7 +127,10 @@ pub async fn get_day( let mut store = state.store.lock().await; match store.load_day(parsed_day).await { - Some(att_day) => Ok(response::Json(att_day)), - None => Err((response::StatusCode::NOT_FOUND, "Not found")), + Ok(att_day) => Ok(response::Json(att_day)), + Err(_) => Err(( + response::StatusCode::INTERNAL_SERVER_ERROR, + "Internal server error", + )), } }