From 1cd3c4f5be825b43926c7849d9fd514f3dcec6f0 Mon Sep 17 00:00:00 2001 From: Wouter Geraedts Date: Mon, 11 Jul 2022 18:50:55 +0200 Subject: [PATCH] Reworked errors --- src/boot/mod.rs | 22 +++-- src/manager/mod.rs | 220 +++++++++++++++++++++++---------------------- src/state/mod.rs | 22 ++--- src/state/ram.rs | 113 ++++++++++------------- 4 files changed, 183 insertions(+), 194 deletions(-) diff --git a/src/boot/mod.rs b/src/boot/mod.rs index b770311..7481b1c 100644 --- a/src/boot/mod.rs +++ b/src/boot/mod.rs @@ -1,7 +1,7 @@ use crate::{ hardware::processor::Processor, hardware::{Bank, Config}, - state::{Exchange, ExchangeProgress, ExchangeStep, MemoryError, State, Update, UpdateError}, + state::{Exchange, ExchangeError, ExchangeProgress, ExchangeStep, State, Update, UpdateError}, }; use embedded_storage::Storage; @@ -58,13 +58,13 @@ impl< } /// Execute the update and boot logic of the bootloader - pub fn boot(&mut self) -> Result { + pub fn boot(&mut self) -> Result { // TODO: consider error handling log::info!("Booting with moonboot!"); self.processor.setup(&self.config); - let mut state = self.state.read(); + let mut state = self.state.read()?; log::info!("Old State: {:?}", state); @@ -73,7 +73,7 @@ impl< Update::None => self.handle_none(), Update::Request(bank) => self.handle_request(bank), Update::Revert(bank) => self.handle_revert(bank), - Update::Exchanging(progress) => self.handle_exchanging(progress), + Update::Exchanging(progress) => self.handle_exchanging(progress)?, Update::Error(err) => Update::Error(err), }; @@ -116,7 +116,7 @@ impl< // Handle a case of power interruption or similar, which lead to a exchange_banks being // interrupted. - fn handle_exchanging(&mut self, progress: ExchangeProgress) -> Update { + fn handle_exchanging(&mut self, progress: ExchangeProgress) -> Result { log::error!( "Firmware Update was interrupted! Trying to recover with exchange operation: {:?}", progress @@ -130,8 +130,8 @@ impl< progress, ); - if exchange_result.is_ok() { - let state = self.state.read().update; + Ok(if exchange_result.is_ok() { + let state = self.state.read()?.update; match state { Update::Exchanging(progress) => { if progress.recovering { @@ -148,7 +148,7 @@ impl< exchange_result ); Update::Error(UpdateError::ImageExchangeFailed) - } + }) } // Revert the bootable image with the image in index new_firmware. Returns Revert on success if @@ -193,7 +193,11 @@ impl< } } - fn exchange_banks(&mut self, a: Bank, b: Bank) -> Result<(), MemoryError> { + fn exchange_banks( + &mut self, + a: Bank, + b: Bank, + ) -> Result<(), ExchangeError> { self.exchange .exchange::( &mut self.storage, diff --git a/src/manager/mod.rs b/src/manager/mod.rs index ad995b8..4a7b193 100644 --- a/src/manager/mod.rs +++ b/src/manager/mod.rs @@ -3,55 +3,67 @@ use crate::{ state::{State, Update}, }; -use embedded_storage::{ReadStorage, Storage}; +use embedded_storage::Storage; use crate::log; /// Instantiate this in your application to enable mutation of the State specified in this and jump /// to the bootloader to apply any updates. pub struct MoonbootManager< - InternalMemory: Storage, - HardwareState: State, - CPU: Processor, + STORAGE: Storage, + STATE: State, + PROCESSOR: Processor, const INTERNAL_PAGE_SIZE: usize, > { config: Config, - internal_memory: InternalMemory, - state: HardwareState, - processor: CPU, + storage: STORAGE, + state: STATE, + processor: PROCESSOR, } -impl< - InternalMemory: Storage, - HardwareState: State, - CPU: Processor, - const INTERNAL_PAGE_SIZE: usize, - > MoonbootManager +pub struct InitError; + +pub enum MarkError { + UpdateQueuedButNotInstalled, + State(E), +} + +impl + MoonbootManager { pub fn new( config: Config, - internal_memory: InternalMemory, - state: HardwareState, - processor: CPU, - ) -> MoonbootManager { - Self { + storage: STORAGE, + state: STATE, + processor: PROCESSOR, + ) -> Result, InitError> { + if config.update_bank.size > config.boot_bank.size { + log::error!( + "Requested update bank {:?} is larger than boot bank {:?}", + bank, + self.config.boot_bank + ); + return Err(InitError); + } + + Ok(Self { config, - internal_memory, + storage, state, processor, - } + }) } /// Destroy this instance of the boot manager and return access to the hardware peripheral - pub fn destroy(self) -> (InternalMemory, HardwareState, CPU) { - (self.internal_memory, self.state, self.processor) + pub fn destroy(self) -> (STORAGE, STATE, PROCESSOR) { + (self.storage, self.state, self.processor) } /// Run this immediately after booting your new image successfully to mark the boot as /// succesful. If you do not do this, any reset will cause the bootloader to restore to the /// previous firmware image. - pub fn mark_boot_successful(&mut self) -> Result<(), ()> { - let mut current_state = self.state.read(); + pub fn mark_boot_successful(&mut self) -> Result<(), MarkError> { + let mut current_state = self.state.read().map_err(MarkError::State)?; log::info!( "Application running, marking boot as successful. Current state: {:?}", @@ -69,34 +81,24 @@ impl< } _ => { log::error!("There is an update queued, but it has not been installed yet. Did you skip the bootloader?"); - return Err(()); + return Err(MarkError::UpdateQueuedButNotInstalled); } }; log::trace!("New state: {:?}", current_state); - self.state.write(¤t_state) + self.state.write(¤t_state).map_err(MarkError::State) } // Upgrade firmware verifiying the given signature over the size of size. // Can only return an error or diverge (!, represented by Void while ! is not a type yet) - pub fn update(&mut self) -> Result { + pub fn update(&mut self) -> Result { // Apply the update stored in the update bank let bank = self.config.update_bank; - // TODO: Check size value! log::info!("Update requested on slot {:?}", bank); - if bank.size > self.config.boot_bank.size { - log::error!( - "Requested update bank {:?} is larger than boot bank {:?}", - bank, - self.config.boot_bank - ); - return Err(()); - } - - let mut current_state = self.state.read(); + let mut current_state = self.state.read()?; if current_state.update != Update::None { log::warn!( @@ -125,78 +127,78 @@ impl< } } -/// Easily get read access to the update bank -impl< - InternalMemory: Storage, - HardwareState: State, - CPU: Processor, - const INTERNAL_PAGE_SIZE: usize, - > core::convert::AsRef<[u8]> - for MoonbootManager -{ - #[inline] - fn as_ref(&self) -> &[u8] { - unsafe { - core::slice::from_raw_parts( - self.config.update_bank.location as *const u8, - self.config.update_bank.size as usize, - ) - } - } -} +// /// Easily get read access to the update bank +// impl< +// InternalMemory: Storage, +// HardwareState: State, +// PROCESSOR: Processor, +// const INTERNAL_PAGE_SIZE: usize, +// > core::convert::AsRef<[u8]> +// for MoonbootManager +// { +// #[inline] +// fn as_ref(&self) -> &[u8] { +// unsafe { +// core::slice::from_raw_parts( +// self.config.update_bank.location as *const u8, +// self.config.update_bank.size as usize, +// ) +// } +// } +// } -/// Read Access to the current update target slot -impl< - InternalMemory: Storage, - HardwareState: State, - CPU: Processor, - const INTERNAL_PAGE_SIZE: usize, - > ReadStorage for MoonbootManager -{ - type Error = (); // TODO +// /// Read Access to the current update target slot +// impl< +// InternalMemory: Storage, +// HardwareState: State, +// CPU: Processor, +// const INTERNAL_PAGE_SIZE: usize, +// > ReadStorage for MoonbootManager +// { +// type Error = (); // TODO - fn read(&mut self, offset: u32, bytes: &mut [u8]) -> Result<(), Self::Error> { - let bank = self.config.update_bank; // For now we always write updates to this bank. - if offset > bank.size || offset + bytes.len() as u32 > bank.size { - Err(()) // TODO: We want better error types! - } else { - // TODO! fix - let bank_start = bank.location; - log::info!("Writing at {:x}[{:x}]", bank_start, offset); - match bank.memory_unit { - crate::hardware::MemoryUnit::Internal => { - { self.internal_memory.read(bank_start + offset, bytes) }.map_err(|_| ()) - } - } - } - } +// fn read(&mut self, offset: u32, bytes: &mut [u8]) -> Result<(), Self::Error> { +// let bank = self.config.update_bank; // For now we always write updates to this bank. +// if offset > bank.size || offset + bytes.len() as u32 > bank.size { +// Err(()) // TODO: We want better error types! +// } else { +// // TODO! fix +// let bank_start = bank.location; +// log::info!("Writing at {:x}[{:x}]", bank_start, offset); +// match bank.memory_unit { +// crate::hardware::MemoryUnit::Internal => { +// { self.storage.read(bank_start + offset, bytes) }.map_err(|_| ()) +// } +// } +// } +// } - fn capacity(&self) -> usize { - self.config.update_bank.size as usize - } -} +// fn capacity(&self) -> usize { +// self.config.update_bank.size as usize +// } +// } -/// Write Access to the current update target slot -impl< - InternalMemory: Storage, - HardwareState: State, - CPU: Processor, - const INTERNAL_PAGE_SIZE: usize, - > Storage for MoonbootManager -{ - fn write(&mut self, offset: u32, bytes: &[u8]) -> Result<(), Self::Error> { - let bank = self.config.update_bank; // For now we always write updates to this bank. - if offset > bank.size || offset + bytes.len() as u32 > bank.size { - Err(()) // TODO: We want better error types! - } else { - // TODO! fix - let bank_start = bank.location; - log::info!("Writing at {:x}[{:x}]", bank_start, offset); - match bank.memory_unit { - crate::hardware::MemoryUnit::Internal => { - { self.internal_memory.write(bank_start + offset, bytes) }.map_err(|_| ()) - } - } - } - } -} +// /// Write Access to the current update target slot +// impl< +// InternalMemory: Storage, +// HardwareState: State, +// CPU: Processor, +// const INTERNAL_PAGE_SIZE: usize, +// > Storage for MoonbootManager +// { +// fn write(&mut self, offset: u32, bytes: &[u8]) -> Result<(), Self::Error> { +// let bank = self.config.update_bank; // For now we always write updates to this bank. +// if offset > bank.size || offset + bytes.len() as u32 > bank.size { +// Err(()) // TODO: We want better error types! +// } else { +// // TODO! fix +// let bank_start = bank.location; +// log::info!("Writing at {:x}[{:x}]", bank_start, offset); +// match bank.memory_unit { +// crate::hardware::MemoryUnit::Internal => { +// { self.internal_memory.write(bank_start + offset, bytes) }.map_err(|_| ()) +// } +// } +// } +// } +// } diff --git a/src/state/mod.rs b/src/state/mod.rs index 95f50a9..c7c9fb9 100644 --- a/src/state/mod.rs +++ b/src/state/mod.rs @@ -6,24 +6,22 @@ pub mod scratch; use embedded_storage::Storage; -/// Error occured during memory access -#[cfg_attr(feature = "use-defmt", derive(Format))] -#[derive(Debug)] -pub enum MemoryError { - BankSizeNotEqual, - BankSizeZero, - ReadFailure, - WriteFailure, +pub enum ExchangeError { + Storage(STORAGE), + State(STATE), + Other(OTHER), } /// Abstraction for the exchange operation of the current state. pub trait Exchange { + type OtherError; + fn exchange( &mut self, internal_memory: &mut STORAGE, state: &mut STATE, progress: ExchangeProgress, - ) -> Result<(), MemoryError>; + ) -> Result<(), ExchangeError>; } use crate::hardware::Bank; @@ -139,10 +137,12 @@ pub struct MoonbootState { /// RAM. As long as you don't want to perform update download, power cycle the device, and then /// apply the update, storing it in volatile memory is fine. pub trait State { + type Error; + /// Read the shared state - fn read(&mut self) -> MoonbootState; + fn read(&mut self) -> Result; /// Write the new state to the shared state - fn write(&mut self, data: &MoonbootState) -> Result<(), ()>; + fn write(&mut self, data: &MoonbootState) -> Result<(), Self::Error>; } /// Size of the serialized state diff --git a/src/state/ram.rs b/src/state/ram.rs index 1c11505..db45a71 100644 --- a/src/state/ram.rs +++ b/src/state/ram.rs @@ -15,7 +15,9 @@ extern "C" { } impl State for RamState { - fn read(&mut self) -> MoonbootState { + type Error = void::Void; + + fn read(&mut self) -> Result { let crc = unsafe { _moonboot_state_crc_start }; log::info!( @@ -28,17 +30,16 @@ impl State for RamState { if crc == checksum { let data = MoonbootState::deserialize_from(unsafe { &_moonboot_state_data_start }); log::trace!("CRC Match! {}: {:?}", crc, data); - return data; + Ok(data) } else { log::trace!("CRC Mismatch! {} vs {}", crc, checksum); - } - - MoonbootState { - update: Update::None, + Ok(MoonbootState { + update: Update::None, + }) } } - fn write(&mut self, data: &MoonbootState) -> Result<(), ()> { + fn write(&mut self, data: &MoonbootState) -> Result<(), Self::Error> { log::trace!("Writing data {:?}", data); unsafe { _moonboot_state_data_start = data.serialize() }; @@ -58,28 +59,21 @@ impl State for RamState { } impl Exchange for RamState { - fn exchange( + type OtherError = void::Void; + + fn exchange( &mut self, - internal_memory: &mut InternalMemory, - state: &mut HardwareState, - exchange: ExchangeProgress, - ) -> Result<(), MemoryError> { + storage: &mut STORAGE, + state: &mut STATE, + progress: ExchangeProgress, + ) -> Result<(), ExchangeError> { let ExchangeProgress { a, b, page_index, step, .. - } = exchange; - - // TODO: Sanity Check start_index - if a.size != b.size { - return Err(MemoryError::BankSizeNotEqual); - } - - if a.size == 0 || b.size == 0 { - return Err(MemoryError::BankSizeZero); - } + } = progress; let size = a.size; // Both are equal @@ -88,41 +82,36 @@ impl Exchange for RamState { let mut page_a_buf = [0_u8; INTERNAL_PAGE_SIZE]; let mut page_b_buf = [0_u8; INTERNAL_PAGE_SIZE]; - // can we reduce this to 1 buf and fancy operations? - // probably not with the read/write API. - // classic memory exchange problem :) - let mut last_state = state.read(); + let mut last_state = state.read().map_err(ExchangeError::State)?; // Set this in the exchanging part to know whether we are in a recovery process from a // failed update or on the initial update let recovering = matches!(last_state.update, Update::Revert(_)); - // TODO: Fix - let a_location = a.location; - let b_location = b.location; - for page_index in page_index..full_pages { let offset = page_index * INTERNAL_PAGE_SIZE as u32; + let a_location = a.location + offset; + let b_location = b.location + offset; log::trace!( "Exchange: Page {}, from a ({}) to b ({})", page_index, - a_location + offset, - b_location + offset + a_location, + b_location ); - internal_memory - .read(a_location + offset, &mut page_a_buf) - .map_err(|_| MemoryError::ReadFailure)?; - internal_memory - .read(b_location + offset, &mut page_b_buf) - .map_err(|_| MemoryError::ReadFailure)?; - internal_memory - .write(a_location + offset, &page_b_buf) - .map_err(|_| MemoryError::WriteFailure)?; - internal_memory - .write(b_location + offset, &page_a_buf) - .map_err(|_| MemoryError::WriteFailure)?; + storage + .read(a_location, &mut page_a_buf) + .map_err(ExchangeError::Storage)?; + storage + .read(b_location, &mut page_b_buf) + .map_err(ExchangeError::Storage)?; + storage + .write(a_location, &page_b_buf) + .map_err(ExchangeError::Storage)?; + storage + .write(b_location, &page_a_buf) + .map_err(ExchangeError::Storage)?; // Store the exchange progress @@ -134,32 +123,26 @@ impl Exchange for RamState { step, }); - state - .write(&last_state) - .map_err(|_| MemoryError::WriteFailure)?; + state.write(&last_state).map_err(ExchangeError::State)?; } // TODO: Fit this into the while loop if remaining_page_length > 0 { let offset = full_pages * INTERNAL_PAGE_SIZE as u32; + let a_location = a.location + offset; + let b_location = b.location + offset; - internal_memory - .read( - a.location + offset, - &mut page_a_buf[0..remaining_page_length], - ) - .map_err(|_| MemoryError::ReadFailure)?; - internal_memory - .read( - b.location + offset, - &mut page_b_buf[0..remaining_page_length], - ) - .map_err(|_| MemoryError::ReadFailure)?; - internal_memory - .write(a.location + offset, &page_a_buf[0..remaining_page_length]) - .map_err(|_| MemoryError::WriteFailure)?; - internal_memory - .write(b.location + offset, &page_b_buf[0..remaining_page_length]) - .map_err(|_| MemoryError::WriteFailure)?; + storage + .read(a_location, &mut page_a_buf[0..remaining_page_length]) + .map_err(ExchangeError::Storage)?; + storage + .read(b_location, &mut page_b_buf[0..remaining_page_length]) + .map_err(ExchangeError::Storage)?; + storage + .write(a_location, &page_a_buf[0..remaining_page_length]) + .map_err(ExchangeError::Storage)?; + storage + .write(b_location + offset, &page_b_buf[0..remaining_page_length]) + .map_err(ExchangeError::Storage)?; } Ok(())