From f1b18d08c7c687871c209ecbddb6ebacf73455ce Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Tue, 1 Sep 2026 11:53:50 -0700 Subject: [PATCH 1/2] firmware: remove std usage for thread::sleep and Instant Instead relying on using the embedded-hal traits and ESP-IDF version. This is yak shaving to remove std as much as possible and later using embassy where it makes sense. It also makes drivers more testable in the future (with a test timer). --- .../handheld/src/device/drivers/bq27427.rs | 32 ++++++---- firmware/handheld/src/device/drivers/dac.rs | 17 +++-- .../handheld/src/device/drivers/fpga/mod.rs | 27 +++++--- .../handheld/src/device/drivers/ili9488.rs | 36 ++++++----- .../handheld/src/device/drivers/ili9806e.rs | 40 +++++++----- firmware/handheld/src/device/drivers/mod.rs | 1 + firmware/handheld/src/device/drivers/timer.rs | 64 +++++++++++++++++++ firmware/handheld/src/device/mod.rs | 21 ++++-- 8 files changed, 170 insertions(+), 68 deletions(-) create mode 100644 firmware/handheld/src/device/drivers/timer.rs diff --git a/firmware/handheld/src/device/drivers/bq27427.rs b/firmware/handheld/src/device/drivers/bq27427.rs index 79a5890c..38f29d2c 100644 --- a/firmware/handheld/src/device/drivers/bq27427.rs +++ b/firmware/handheld/src/device/drivers/bq27427.rs @@ -1,7 +1,9 @@ #![allow(dead_code)] use embedded_hal::i2c::I2c; -use std::time::Duration; +use core::time::Duration; + +use super::timer::Timer; use thiserror::Error; const ADDRESS: u8 = 0x55; @@ -120,16 +122,18 @@ pub enum Error { InvalidArgument, } -pub struct BQ27427 { +pub struct BQ27427 { i2c: I2C, + timer: T, } -impl BQ27427 +impl BQ27427 where I2C: I2c, + T: Timer, { - pub fn new(i2c: I2C) -> Self { - BQ27427 { i2c } + pub fn new(i2c: I2C, timer: T) -> Self { + BQ27427 { i2c, timer } } /// Configure the fuel gauge (blocking, may take a while) @@ -191,7 +195,7 @@ where let retries = 10; let delay = Duration::from_millis(500); for _ in 0..retries { - std::thread::sleep(delay); + self.timer.sleep(delay); if (self.get_flags()? & flag) != 0 { return Ok(()); @@ -256,7 +260,7 @@ where self.i2c .write_read(ADDRESS, &[command.0], &mut data) .map_err(|_| Error::I2cError)?; - std::thread::sleep(WAIT_TIME); + self.timer.sleep(WAIT_TIME); Ok(u16::from_le_bytes(data)) } @@ -266,7 +270,7 @@ where self.i2c .write_read(ADDRESS, &[0x00], &mut data) .map_err(|_| Error::I2cError)?; - std::thread::sleep(WAIT_TIME); + self.timer.sleep(WAIT_TIME); Ok(u16::from_le_bytes(data)) } @@ -276,11 +280,11 @@ where self.i2c .write(ADDRESS, &[0x00, a0]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(WAIT_TIME); + self.timer.sleep(WAIT_TIME); self.i2c .write(ADDRESS, &[0x01, a1]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(WAIT_TIME); + self.timer.sleep(WAIT_TIME); Ok(()) } @@ -308,7 +312,7 @@ where .write(ADDRESS, &[CMD_DATA_BLOCK, offset / 32]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(BLOCK_DELAY); + self.timer.sleep(BLOCK_DELAY); // Write the bytes to the BlockData for (i, &x) in data.iter().enumerate() { @@ -324,7 +328,7 @@ where .write(ADDRESS, &[CMD_BLOCK_DATA_CHECKSUM, new_checksum]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(BLOCK_DELAY); + self.timer.sleep(BLOCK_DELAY); Ok(()) } @@ -358,7 +362,7 @@ where self.i2c .write(ADDRESS, &[CMD_DATA_BLOCK, offset / 32]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(BLOCK_DELAY); + self.timer.sleep(BLOCK_DELAY); // Read CC Gain let mut value = [0u8]; @@ -389,7 +393,7 @@ where .write(ADDRESS, &[CMD_BLOCK_DATA_CHECKSUM, new_checksum]) .map_err(|_| Error::I2cError)?; - std::thread::sleep(BLOCK_DELAY); + self.timer.sleep(BLOCK_DELAY); Ok(()) } } diff --git a/firmware/handheld/src/device/drivers/dac.rs b/firmware/handheld/src/device/drivers/dac.rs index 9c7ea039..6b388a79 100644 --- a/firmware/handheld/src/device/drivers/dac.rs +++ b/firmware/handheld/src/device/drivers/dac.rs @@ -1,6 +1,8 @@ #![allow(dead_code)] -use std::time::Duration; +use core::time::Duration; + +use super::timer::Timer; use embedded_hal::digital::OutputPin; use embedded_hal::i2c::I2c; @@ -32,24 +34,27 @@ pub struct InterruptStatus { pub right_dac_power: bool, } -pub struct TLV320DAC3101 { +pub struct TLV320DAC3101 { pin_reset: PinReset, i2c: I2C, + timer: T, page: u8, volume: u8, mute: bool, } -impl TLV320DAC3101 +impl TLV320DAC3101 where PinReset: OutputPin, I2C: I2c, + T: Timer, { - pub fn new(pin_reset: PinReset, i2c: I2C) -> Self { + pub fn new(pin_reset: PinReset, i2c: I2C, timer: T) -> Self { TLV320DAC3101 { pin_reset, i2c, + timer, page: 0, volume: 0, @@ -60,9 +65,9 @@ where /// Reset the device without configuring it. pub fn reset(&mut self) -> Result<(), Error> { self.pin_reset.set_low().map_err(|_| Error::PinError)?; - std::thread::sleep(Duration::from_micros(1)); + self.timer.sleep(Duration::from_micros(1)); self.pin_reset.set_high().map_err(|_| Error::PinError)?; - std::thread::sleep(Duration::from_millis(1)); + self.timer.sleep(Duration::from_millis(1)); Ok(()) } diff --git a/firmware/handheld/src/device/drivers/fpga/mod.rs b/firmware/handheld/src/device/drivers/fpga/mod.rs index 15cfce0e..3ef44215 100644 --- a/firmware/handheld/src/device/drivers/fpga/mod.rs +++ b/firmware/handheld/src/device/drivers/fpga/mod.rs @@ -2,7 +2,7 @@ use std::{ io::Read, - time::{Duration, Instant}, + time::Duration, }; use embedded_hal::{ @@ -14,6 +14,7 @@ use esp_idf_svc::hal::{ units::Hertz, }; use thiserror::Error; +use crate::device::drivers::timer::Timer; use crate::device::DisplayMode; @@ -77,6 +78,7 @@ pub struct Fpga< PinProgramB: OutputPin, PinInitB: InputPin, ProgramSpi: SpiDevice, + T: Timer, > { pin_done: PinDone, pub pin_program_b: PinProgramB, @@ -91,15 +93,18 @@ pub struct Fpga< /// Bitfield of enabled interrupts interrupts: u32, + + timer: T, } -impl<'a, PinDone, PinProgramB, PinInitB, ProgramSpi> - Fpga<'a, PinDone, PinProgramB, PinInitB, ProgramSpi> +impl<'a, PinDone, PinProgramB, PinInitB, ProgramSpi, T> + Fpga<'a, PinDone, PinProgramB, PinInitB, ProgramSpi, T> where PinDone: InputPin, PinProgramB: OutputPin, PinInitB: InputPin, ProgramSpi: SpiDevice, + T: Timer, { pub fn new( pin_done: PinDone, @@ -107,8 +112,10 @@ where pin_init_b: PinInitB, data_spi: Vec<(SpiDataDriver<'a>, Hertz)>, program_spi: ProgramSpi, + timer: T, ) -> Self { Fpga { + timer, pin_done, pin_program_b, pin_init_b, @@ -143,30 +150,30 @@ where // After power-on-reset, INIT_B will be low for 10ms to 35ms (T_POR), // configuration can only start after this. // Poll INIT_B until it goes high. - let start_time = Instant::now(); + let start_ms = self.timer.now_ms(); while self.pin_init_b.is_low().map_err(|_| Error::PinError)? { - if start_time.elapsed() > Duration::from_millis(35) { + if self.timer.now_ms().saturating_sub(start_ms) > 35 { return Err(Error::ProgramError); } - std::thread::sleep(Duration::from_millis(5)); + self.timer.sleep(Duration::from_millis(5)); } // Pull PROGRAM_B low, hold it for at least 250ns. self.pin_program_b.set_low().map_err(|_| Error::PinError)?; - std::thread::sleep(Duration::from_millis(1)); + self.timer.sleep(Duration::from_millis(1)); if self.pin_init_b.is_high().map_err(|_| Error::PinError)? { return Err(Error::ProgramError); } self.pin_program_b.set_high().map_err(|_| Error::PinError)?; // INIT_B will go high at most 5ms after PROGRAM_B release. - std::thread::sleep(Duration::from_millis(5)); + self.timer.sleep(Duration::from_millis(5)); if self.pin_init_b.is_low().map_err(|_| Error::PinError)? { return Err(Error::ProgramError); } log::info!("FPGA is in program mode"); - let start_time = Instant::now(); + let start_ms = self.timer.now_ms(); let mut num_read = 0; while num_read < header.length { @@ -185,7 +192,7 @@ where log::info!( "Programmed FPGA, done={}, time={}", self.pin_done.is_high().map_err(|_| Error::PinError)?, - start_time.elapsed().as_millis() as u32, + self.timer.now_ms().saturating_sub(start_ms), ); Ok(()) diff --git a/firmware/handheld/src/device/drivers/ili9488.rs b/firmware/handheld/src/device/drivers/ili9488.rs index 8b3a3429..3c701225 100644 --- a/firmware/handheld/src/device/drivers/ili9488.rs +++ b/firmware/handheld/src/device/drivers/ili9488.rs @@ -1,6 +1,8 @@ #![allow(unused)] -use std::time::{Duration, Instant}; +use core::time::Duration; + +use super::timer::Timer; use thiserror::Error; use embedded_hal::digital::OutputPin; @@ -25,35 +27,39 @@ pub enum RenderSource { Mcu, } -pub struct ILI9488 { +pub struct ILI9488 { pin_reset: PinReset, pin_dc: PinDc, spi: Spi, - last_sleep_change: Instant, + timer: T, + /// When the panel last changed sleep state, as a reading from `timer`. + last_sleep_change: u64, render_source: RenderSource, } -impl ILI9488 +impl ILI9488 where Spi: SpiDevice, PinReset: OutputPin, PinDc: OutputPin, + T: Timer, { - pub fn new(pin_reset: PinReset, pin_dc: PinDc, spi: Spi) -> Self { + pub fn new(pin_reset: PinReset, pin_dc: PinDc, spi: Spi, timer: T) -> Self { ILI9488 { + last_sleep_change: timer.now_ms(), pin_reset, pin_dc, spi, - last_sleep_change: Instant::now(), + timer, render_source: RenderSource::Mcu, } } pub fn init(&mut self) -> Result<(), Error> { self.pin_reset.set_low().map_err(|_| Error::ResetError)?; - std::thread::sleep(Duration::from_micros(100)); + self.timer.sleep(Duration::from_micros(100)); self.pin_reset.set_high().map_err(|_| Error::ResetError)?; - self.last_sleep_change = Instant::now(); + self.last_sleep_change = self.timer.now_ms(); // "Adjust Control 3": params have no specified meaning self.write_cmd(0xF7, &[0xA9, 0x51, 0x2C, 0x82])?; @@ -123,8 +129,8 @@ where } pub fn enter_sleep(&mut self) -> Result<(), Error> { - let wait_time = SLEEP_CHANGE_DELAY.saturating_sub(self.last_sleep_change.elapsed()); - std::thread::sleep(wait_time); + self.timer + .sleep_until(self.last_sleep_change, SLEEP_CHANGE_DELAY); // Display off self.write_cmd(0x28, &[])?; @@ -134,21 +140,21 @@ where // Sleep in self.write_cmd(0x10, &[])?; - self.last_sleep_change = Instant::now(); + self.last_sleep_change = self.timer.now_ms(); Ok(()) } pub fn exit_sleep(&mut self) -> Result<(), Error> { - let wait_time = SLEEP_CHANGE_DELAY.saturating_sub(self.last_sleep_change.elapsed()); - std::thread::sleep(wait_time); + self.timer + .sleep_until(self.last_sleep_change, SLEEP_CHANGE_DELAY); // Sleep out self.write_cmd(0x11, &[])?; - self.last_sleep_change = Instant::now(); + self.last_sleep_change = self.timer.now_ms(); // Must wait 5ms before sending commands after sleep out. - std::thread::sleep(Duration::from_millis(5)); + self.timer.sleep(Duration::from_millis(5)); // Re-enable use of DOTCLK if matches!(self.render_source, RenderSource::Fpga) { diff --git a/firmware/handheld/src/device/drivers/ili9806e.rs b/firmware/handheld/src/device/drivers/ili9806e.rs index 051ab8a2..bfce542e 100644 --- a/firmware/handheld/src/device/drivers/ili9806e.rs +++ b/firmware/handheld/src/device/drivers/ili9806e.rs @@ -1,4 +1,6 @@ -use std::time::{Duration, Instant}; +use core::time::Duration; + +use super::timer::Timer; use thiserror::Error; use embedded_hal::digital::OutputPin; @@ -15,34 +17,38 @@ pub enum Error { SpiError, } -pub struct ILI9806E { +pub struct ILI9806E { pin_reset: PinReset, spi: Spi, - last_sleep_change: Instant, + timer: T, + /// When the panel last changed sleep state, as a reading from `timer`. + last_sleep_change: u64, } -impl ILI9806E +impl ILI9806E where Spi: SpiDevice, PinReset: OutputPin, + T: Timer, { - pub fn new(pin_reset: PinReset, spi: Spi) -> Self { + pub fn new(pin_reset: PinReset, spi: Spi, timer: T) -> Self { Self { + last_sleep_change: timer.now_ms(), pin_reset, spi, - last_sleep_change: Instant::now(), + timer, } } pub fn init(&mut self) -> Result<(), Error> { // Reset the display. self.pin_reset.set_high().map_err(|_| Error::ResetError)?; - std::thread::sleep(Duration::from_millis(1)); + self.timer.sleep(Duration::from_millis(1)); self.pin_reset.set_low().map_err(|_| Error::ResetError)?; - std::thread::sleep(Duration::from_millis(5)); + self.timer.sleep(Duration::from_millis(5)); self.pin_reset.set_high().map_err(|_| Error::ResetError)?; - self.last_sleep_change = Instant::now(); - std::thread::sleep(Duration::from_millis(5)); + self.last_sleep_change = self.timer.now_ms(); + self.timer.sleep(Duration::from_millis(5)); // Change to page 1 self.write_cmd(0xFF, &[0xFF, 0x98, 0x06, 0x04, 0x01])?; @@ -196,29 +202,29 @@ where } pub fn enter_sleep(&mut self) -> Result<(), Error> { - let wait_time = SLEEP_CHANGE_DELAY.saturating_sub(self.last_sleep_change.elapsed()); - std::thread::sleep(wait_time); + self.timer + .sleep_until(self.last_sleep_change, SLEEP_CHANGE_DELAY); // Display off self.write_cmd(0x28, &[])?; // Sleep in self.write_cmd(0x10, &[])?; - self.last_sleep_change = Instant::now(); + self.last_sleep_change = self.timer.now_ms(); Ok(()) } pub fn exit_sleep(&mut self) -> Result<(), Error> { - let wait_time = SLEEP_CHANGE_DELAY.saturating_sub(self.last_sleep_change.elapsed()); - std::thread::sleep(wait_time); + self.timer + .sleep_until(self.last_sleep_change, SLEEP_CHANGE_DELAY); // Sleep out self.write_cmd(0x11, &[])?; - self.last_sleep_change = Instant::now(); + self.last_sleep_change = self.timer.now_ms(); // Must wait 5ms before sending commands after sleep out. - std::thread::sleep(Duration::from_millis(5)); + self.timer.sleep(Duration::from_millis(5)); // Display on self.write_cmd(0x29, &[])?; diff --git a/firmware/handheld/src/device/drivers/mod.rs b/firmware/handheld/src/device/drivers/mod.rs index c16f8118..7898d02e 100644 --- a/firmware/handheld/src/device/drivers/mod.rs +++ b/firmware/handheld/src/device/drivers/mod.rs @@ -17,4 +17,5 @@ pub mod rtc; pub mod sdcard; #[cfg(feature = "has_st7262")] pub mod st7262; +pub mod timer; pub mod usb; diff --git a/firmware/handheld/src/device/drivers/timer.rs b/firmware/handheld/src/device/drivers/timer.rs new file mode 100644 index 00000000..51c57a9d --- /dev/null +++ b/firmware/handheld/src/device/drivers/timer.rs @@ -0,0 +1,64 @@ +//! Blocking delays and a monotonic reading, for drivers that need to wait. +//! +//! `embedded_hal` already has [`DelayNs`] for waiting. It has no clock, so +//! that part is ours. Both live behind one trait so a driver carries one extra +//! parameter rather than two. + +use core::time::Duration; + +use embedded_hal::delay::DelayNs; + +/// A blocking delay plus a monotonic reading. +/// +/// Passed to the methods that need it rather than stored, so drivers keep the +/// concrete types they had and callers stay unchanged in shape. +pub trait Timer: DelayNs { + /// Milliseconds from an arbitrary fixed point. Only differences between + /// readings are meaningful. + fn now_ms(&self) -> u64; + + /// Wait out `duration`. + fn sleep(&mut self, duration: Duration) { + // Microseconds are enough for every wait here -- the shortest is 1us -- + // and keep the value inside a u32 for delays past four seconds. + self.delay_us(duration.as_micros().min(u32::MAX as u128) as u32); + } + + /// Wait until `duration` has passed since `since_ms`, if it hasn't already. + /// + /// Panels want a minimum interval between sleep-state changes. Waiting the + /// full interval every time would be simpler but would add that delay to + /// every display on/off, so keep track of when the last one was. + fn sleep_until(&mut self, since_ms: u64, duration: Duration) { + let elapsed = Duration::from_millis(self.now_ms().saturating_sub(since_ms)); + self.sleep(duration.saturating_sub(elapsed)); + } +} + +/// The system timer, backed by esp-idf's microsecond clock. +/// +/// A bare-metal port would implement [`Timer`] over a hardware timer instead; +/// nothing above this line knows the difference. +#[derive(Copy, Clone, Default)] +pub struct SystemTimer; + +impl DelayNs for SystemTimer { + fn delay_ns(&mut self, ns: u32) { + esp_idf_svc::hal::delay::Delay::new_default().delay_ns(ns); + } + + fn delay_us(&mut self, us: u32) { + esp_idf_svc::hal::delay::Delay::new_default().delay_us(us); + } + + fn delay_ms(&mut self, ms: u32) { + esp_idf_svc::hal::delay::Delay::new_default().delay_ms(ms); + } +} + +impl Timer for SystemTimer { + fn now_ms(&self) -> u64 { + // Monotonic since boot, in microseconds. + (unsafe { esp_idf_svc::sys::esp_timer_get_time() } as u64) / 1000 + } +} diff --git a/firmware/handheld/src/device/mod.rs b/firmware/handheld/src/device/mod.rs index a1e64314..e73894a8 100644 --- a/firmware/handheld/src/device/mod.rs +++ b/firmware/handheld/src/device/mod.rs @@ -55,6 +55,7 @@ pub struct Device<'a> { PinDriver<'a, AnyOutputPin, Output>, PinDriver<'a, AnyOutputPin, Output>, SpiSoftCsDeviceDriver<'a, SpiSharedDeviceDriver<'a, &'a SpiDriver<'a>>, &'a SpiDriver<'a>>, + drivers::timer::SystemTimer, >, #[cfg(feature = "has_st7262")] pub lcd: drivers::st7262::ST7262>, @@ -62,6 +63,7 @@ pub struct Device<'a> { pub lcd: drivers::ili9806e::ILI9806E< PinDriver<'a, AnyOutputPin, Output>, SpiSoftCsDeviceDriver<'a, SpiSharedDeviceDriver<'a, &'a SpiDriver<'a>>, &'a SpiDriver<'a>>, + drivers::timer::SystemTimer, >, /// Display mode (if initialized) @@ -71,6 +73,7 @@ pub struct Device<'a> { pub dac: drivers::dac::TLV320DAC3101< PinDriver<'a, AnyOutputPin, Output>, MutexI2C<'a, I2cDriver<'a>>, + drivers::timer::SystemTimer, >, /// FPGA driver @@ -80,6 +83,7 @@ pub struct Device<'a> { PinDriver<'a, AnyOutputPin, Output>, PinDriver<'a, AnyIOPin, Input>, SpiDeviceDriver<'a, &'a SpiDriver<'a>>, + drivers::timer::SystemTimer, >, /// RTC driver @@ -89,7 +93,7 @@ pub struct Device<'a> { #[cfg(feature = "has_max17048")] pub fuel_gauge: drivers::max17048::MAX17048>>, #[cfg(feature = "has_bq27427")] - pub fuel_gauge: drivers::bq27427::BQ27427>>, + pub fuel_gauge: drivers::bq27427::BQ27427>, drivers::timer::SystemTimer>, /// IMU driver pub imu: drivers::imu::LSM6DS3TRC>>, @@ -399,7 +403,7 @@ impl Device<'_> { )?; let lcd_reset = PinDriver::output(pin_lcd_reset)?; let lcd_dc = PinDriver::output(pin_lcd_dc)?; - let mut lcd = drivers::ili9488::ILI9488::new(lcd_reset, lcd_dc, lcd_spi); + let mut lcd = drivers::ili9488::ILI9488::new(lcd_reset, lcd_dc, lcd_spi, drivers::timer::SystemTimer); } else if #[cfg(feature = "has_st7262")] { let lcd_enable = PinDriver::output(pin_lcd_enable)?; let mut lcd = drivers::st7262::ST7262::new(lcd_enable); @@ -412,7 +416,7 @@ impl Device<'_> { gpio::Level::High, )?; let lcd_reset = PinDriver::output(pin_lcd_reset)?; - let mut lcd = drivers::ili9806e::ILI9806E::new(lcd_reset, lcd_spi); + let mut lcd = drivers::ili9806e::ILI9806E::new(lcd_reset, lcd_spi, drivers::timer::SystemTimer); } } lcd.init().context("LCD init")?; @@ -452,7 +456,7 @@ impl Device<'_> { let mut fuel_gauge = drivers::max17048::MAX17048::new(MutexI2C::new(&i2c)); let _ = fuel_gauge.set_alert_soc_change(true); // fuel gauge won't work without a battery } else if #[cfg(feature = "has_bq27427")] { - let fuel_gauge = drivers::bq27427::BQ27427::new(MutexI2C::new(&i2c)); + let fuel_gauge = drivers::bq27427::BQ27427::new(MutexI2C::new(&i2c), drivers::timer::SystemTimer); // Fuel gauge requires configuration, which could block for multiple seconds. Run it in another thread, // with a new instance of the driver. Scary, but fine, because the driver has no state @@ -463,7 +467,7 @@ impl Device<'_> { .name("battery_setup".to_string()) .stack_size(2 * 1024) .spawn(move || { - let mut fuel_gauge = drivers::bq27427::BQ27427::new(i2c_2); + let mut fuel_gauge = drivers::bq27427::BQ27427::new(i2c_2, drivers::timer::SystemTimer); // TODO: after software update, force reconfigure? let force_configure = false; if fuel_gauge.configure(force_configure).is_err() { @@ -490,7 +494,11 @@ impl Device<'_> { // Setup DAC (requires fpga_power on) log::info!("Initializing DAC"); let dac_reset = PinDriver::output(pin_dac_reset)?; - let mut dac = drivers::dac::TLV320DAC3101::new(dac_reset, MutexI2C::new(&i2c)); + let mut dac = drivers::dac::TLV320DAC3101::new( + dac_reset, + MutexI2C::new(&i2c), + drivers::timer::SystemTimer, + ); dac.init().context("DAC init")?; dac.configure_interrupts().context("DAC interrupts")?; dac.set_volume(kvs::keys::VOLUME.get().unwrap()) @@ -541,6 +549,7 @@ impl Device<'_> { fpga_init_b, fpga_data_spis, fpga_program_spi, + drivers::timer::SystemTimer, ); // Mount system_data to /system From bda6f8446a1a89ed184f02597c6b31fb1d7e40f6 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Tue, 1 Sep 2026 12:47:10 -0700 Subject: [PATCH 2/2] firmware: Use embedded-io for the FPGA bitstream std::io::Read was the last thing tying the bitstream mod to std. The heatshrink decoder already speaks embedded-io, so moving the FPGA driver and the header parser onto the same traits lets the decoder feed the driver directly and removes the ToStd adapter that sat between them. --- firmware/handheld/Cargo.toml | 3 ++ firmware/handheld/src/bitstream/mod.rs | 6 +-- .../handheld/src/device/drivers/bq27427.rs | 2 +- .../handheld/src/device/drivers/fpga/mod.rs | 13 +++-- .../src/device/drivers/fpga/xilinx.rs | 49 +++++++++++++++---- firmware/handheld/src/device/led.rs | 1 + firmware/handheld/src/device/mod.rs | 3 +- 7 files changed, 55 insertions(+), 22 deletions(-) diff --git a/firmware/handheld/Cargo.toml b/firmware/handheld/Cargo.toml index 2fb56293..032236b6 100644 --- a/firmware/handheld/Cargo.toml +++ b/firmware/handheld/Cargo.toml @@ -72,6 +72,9 @@ cfg-if = "1.0" base64 = "0.22.1" hex = "0.4.3" heatshrink-lib = { version = "1.0.0", features = ["embedded-io"] } +# Same family the heatshrink decoder already speaks; the adapters bridge the +# std side until the filesystem itself is abstracted. +embedded-io = "0.6" embedded-io-adapters = { version = "0.6.0", features = ["std"] } [build-dependencies] diff --git a/firmware/handheld/src/bitstream/mod.rs b/firmware/handheld/src/bitstream/mod.rs index 800ed368..9e06395c 100644 --- a/firmware/handheld/src/bitstream/mod.rs +++ b/firmware/handheld/src/bitstream/mod.rs @@ -1,5 +1,6 @@ use std::fs::File; -use std::io::Read; + +use embedded_io::Read; use std::sync::{Mutex, MutexGuard}; use std::time::Duration; @@ -91,8 +92,7 @@ fn heatshrink_decompress_stream(file: File) -> impl Read { // Heatshrink decoder parameters: W=9, L=6 (chosen empirically) type HeatshrinkDecoder = heatshrink::decoder::HeatshrinkDecoder<9, 6, 512, 512>; let reader = embedded_io_adapters::std::FromStd::new(file); - let decoder = heatshrink::io::DecoderReader::<_, HeatshrinkDecoder>::new(reader); - embedded_io_adapters::std::ToStd::new(decoder) + heatshrink::io::DecoderReader::<_, HeatshrinkDecoder>::new(reader) } pub enum CurrentBitstream { diff --git a/firmware/handheld/src/device/drivers/bq27427.rs b/firmware/handheld/src/device/drivers/bq27427.rs index 38f29d2c..5e1de3e6 100644 --- a/firmware/handheld/src/device/drivers/bq27427.rs +++ b/firmware/handheld/src/device/drivers/bq27427.rs @@ -1,7 +1,7 @@ #![allow(dead_code)] -use embedded_hal::i2c::I2c; use core::time::Duration; +use embedded_hal::i2c::I2c; use super::timer::Timer; use thiserror::Error; diff --git a/firmware/handheld/src/device/drivers/fpga/mod.rs b/firmware/handheld/src/device/drivers/fpga/mod.rs index 3ef44215..9bbbac0e 100644 --- a/firmware/handheld/src/device/drivers/fpga/mod.rs +++ b/firmware/handheld/src/device/drivers/fpga/mod.rs @@ -1,10 +1,10 @@ #![allow(dead_code)] -use std::{ - io::Read, - time::Duration, -}; +use core::time::Duration; + +use embedded_io::Read; +use crate::device::drivers::timer::Timer; use embedded_hal::{ digital::{InputPin, OutputPin}, spi::SpiDevice, @@ -14,7 +14,6 @@ use esp_idf_svc::hal::{ units::Hertz, }; use thiserror::Error; -use crate::device::drivers::timer::Timer; use crate::device::DisplayMode; @@ -127,9 +126,9 @@ where } /// Program the FPGA with a new bitstream. - pub fn program( + pub fn program( &mut self, - bitstream: &mut dyn Read, + bitstream: &mut R, scratch_buf: &mut [u8], ) -> Result<(), Error> { let header = diff --git a/firmware/handheld/src/device/drivers/fpga/xilinx.rs b/firmware/handheld/src/device/drivers/fpga/xilinx.rs index 07e10703..69c3c6cd 100644 --- a/firmware/handheld/src/device/drivers/fpga/xilinx.rs +++ b/firmware/handheld/src/device/drivers/fpga/xilinx.rs @@ -1,6 +1,4 @@ -use std::io::Read; - -use anyhow::bail; +use embedded_io::{Read, ReadExactError}; #[allow(unused)] mod consts { @@ -11,6 +9,8 @@ mod consts { pub const TAG_BITSTREAM: u8 = 0x65; } +const HEADER_LEN: usize = 9; + pub struct BitstreamMetadata { /// Bitstream payload length pub length: usize, @@ -18,38 +18,67 @@ pub struct BitstreamMetadata { pub user_id: Option, } +/// Why a header failed to parse. +/// +/// Concrete rather than `anyhow`, which would need the reader's error to be +/// `Send + Sync + 'static` -- a bound `embedded_io::Read` never promises, and +/// one that spreads to every caller until it reaches a reader too opaque to +/// name it on. +#[derive(Debug, thiserror::Error)] +pub enum HeaderError { + #[error("bitstream ended mid-header")] + UnexpectedEof, + #[error("could not read bitstream")] + Io, + #[error("header length field was not {HEADER_LEN}")] + BadHeaderLength, + #[error("header magic did not match")] + BadMagic, + #[error("version field was not 1")] + BadVersion, +} + +// Blanket over the reader's error type, so `?` needs no bound on it at all. +impl From> for HeaderError { + fn from(e: ReadExactError) -> Self { + match e { + ReadExactError::UnexpectedEof => Self::UnexpectedEof, + ReadExactError::Other(_) => Self::Io, + } + } +} + /// Parse the Xilinx .bit header from the file. /// /// On success, leaves the cursor at the start of the bitstream payload. -pub fn parse_bitstream_header(f: &mut dyn Read) -> anyhow::Result { - fn read_u16(f: &mut dyn Read) -> anyhow::Result { +pub fn parse_bitstream_header(f: &mut R) -> Result { + fn read_u16(f: &mut R) -> Result { let mut data = [0u8; 2]; f.read_exact(&mut data)?; Ok(u16::from_be_bytes(data)) } - fn read_u32(f: &mut dyn Read) -> anyhow::Result { + fn read_u32(f: &mut R) -> Result { let mut data = [0u8; 4]; f.read_exact(&mut data)?; Ok(u32::from_be_bytes(data)) } // Read initial header - const HEADER_LEN: usize = 9; let header_len = read_u16(f)?; if (header_len as usize) != HEADER_LEN { - bail!("Unexpected header length"); + return Err(HeaderError::BadHeaderLength); } let mut header = [0u8; HEADER_LEN]; f.read_exact(&mut header)?; if header != [0x0F, 0xF0, 0x0F, 0xF0, 0x0F, 0xF0, 0x0F, 0xF0, 0x00] { - bail!("Invalid header"); + return Err(HeaderError::BadMagic); } // Read the 2 bytes (0x0001)... a version perhaps? let unknown = read_u16(f)?; if unknown != 1 { - bail!("Invalid unknown value"); + return Err(HeaderError::BadVersion); } let mut metadata = BitstreamMetadata { diff --git a/firmware/handheld/src/device/led.rs b/firmware/handheld/src/device/led.rs index e69de29b..8b137891 100644 --- a/firmware/handheld/src/device/led.rs +++ b/firmware/handheld/src/device/led.rs @@ -0,0 +1 @@ + diff --git a/firmware/handheld/src/device/mod.rs b/firmware/handheld/src/device/mod.rs index e73894a8..1ddbd0ae 100644 --- a/firmware/handheld/src/device/mod.rs +++ b/firmware/handheld/src/device/mod.rs @@ -93,7 +93,8 @@ pub struct Device<'a> { #[cfg(feature = "has_max17048")] pub fuel_gauge: drivers::max17048::MAX17048>>, #[cfg(feature = "has_bq27427")] - pub fuel_gauge: drivers::bq27427::BQ27427>, drivers::timer::SystemTimer>, + pub fuel_gauge: + drivers::bq27427::BQ27427>, drivers::timer::SystemTimer>, /// IMU driver pub imu: drivers::imu::LSM6DS3TRC>>,