From 00313248c1541c8c74b9339c2ff45701b1e566ea Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 24 May 2026 09:57:15 -0700 Subject: [PATCH 1/3] hiffy: only reset timer if it has elapsed Issue #2528 describes a bug in Hiffy introduced by the change to make Hiffy callable over the management network directly, rather than through `udprpc` (PR #2466). The bug results in Hiffy calls via a debug probe timing out while the system is connected to a management network. As I described in [this comment][1], @jamesmunns and I determined that this occurs due to a combination of two factors: First, the top of the `main()` loop in `task-hiffy` always begins by unconditionally advancing the deadline by `sleep_ms` and calling `sys_set_timer()` with the new deadline. Second, the check for whether `HIFFY_KICK` was set by the probe is *conditional on whether the timer notification has fired*. If hiffy was only notified by the timer, this would be fine; the previous timeout has elapsed so we should advance the timer. However, if Hiffy has been notified by the `net` task's `SOCKET` notification, we will set the timer forwards, but we will _not_ check `HIFFY_KICK`, meaning any kick delivered by the debug probe is missed. While debugging, we noticed that a large number of seemingly spurious notifications were being posted to hiffy by the `net` task. When the management network is connected, these spurious notifications are posted more frequently than the timer, meaning that the timer *never* completes, and `HIFFY_KICK` is never checked. This commit fixes the bug by changing `task-hiffy`'s main loop in two main ways. First, we _always_ check the value of `HIFFY_KICK`, regardless of which notification woke us. This change alone is sufficient to fix the bug, as I demonstrated in [this comment][2]. This probably also reduces the latency of the `net` Hiffy RPC, as it kicks execution by _also_ setting `HIFFY_KICK`, and then waiting for the timer to fire to start execution. This way, we no longer need to wait for the timer. In addition, I have also changed the `main()` loop so that the timer is only reset at the end of the loop, if it has actually fired. This way, no matter how often the `net` task notifies us, the timer will always fire before being reset. I believe that either of these changes would be sufficient to fix the bug, but fixing both of them makes the behavior closer to what we would expect. To demonstrate that it works, here's me flashing `mb-1-sp` (the system on which the bug was first encountered) with an image from this branch, and calling Hiffy both via the probe (using `humility net ip`, which uses Hiffy), and then over the network. Note that they both work fine now. Yay! ```console eliza@jeeves ~ $ export HUMILITY_ARCHIVE="$HOME/build-cosmo-b-dev-image-default.hiffy-kickme.zip" eliza@jeeves ~ $ pfexec /staff/eliza/humility -t mb-1-sp flash humility: WARNING: archive in environment variable overriding archive in environment file humility: attached to archive humility: attaching with chip set to "STM32H753ZITx" humility: attached to 1fc9:0143:ISS1EG0OBG2HT via CMSIS-DAP humility: flash/archive mismatch; reflashing humility: skipping measurement token handoff humility: auxiliary flash data is already loaded in slot 0; skipping programming humility: done with auxiliary flash humility: flashing done eliza@jeeves ~ $ pfexec /staff/eliza/humility -t mb-1-sp net ip humility: WARNING: archive in environment variable overriding archive in environment file humility: attached to 1fc9:0143:ISS1EG0OBG2HT via CMSIS-DAP MAC address: a8:40:25:04:11:c5 IPv6 address: fe80::aa40:25ff:fe04:11c5 eliza@jeeves ~ $ /staff/eliza/humility --ip fe80::aa40:25ff:fe04:11c5%e1000g0 hiffy -c Sequencer.get_state humility: connecting to fe80::aa40:25ff:fe04:11c5%2 Sequencer.get_state() => A0PlusHP eliza@jeeves ~ $ ``` This fixes #2528. Note that the spurious notifications from the `net` task are *not* fixed by this change, but Hiffy can now behave directly despite them. We need to determine why we're getting so many notifications while the recv queue is empty as well, but this commit fixes the bug. I'll open a new ticket for investigating the spurious notifications. [1]: https://github.com/oxidecomputer/hubris/issues/2528#issuecomment-4526508627 [2]: https://github.com/oxidecomputer/hubris/issues/2528#issuecomment-4526364868 The root cause of the spurious notifications hasn't been determined yet; we need to investigate this separately. However, in the meantime --- task/hiffy/src/main.rs | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index 175c7ee4d..ee3b91289 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -195,13 +195,13 @@ fn main() -> ! { #[cfg(feature = "net")] let mut net_state = net::State::new(); + // Set the initial timer deadline. + set_timer(sleep_ms); loop { - // Sleep until either the timer expires or we receive a notification - // from the `net` task indicating that it's ready for us. - let deadline = sys_get_timer().now.saturating_add(sleep_ms); HIFFY_READY.store(1, Ordering::Relaxed); - sys_set_timer(Some(deadline), notifications::TIMER_MASK); + // Sleep until either the timer expires or we receive a notification + // from the `net` task indicating that it's ready for us. #[cfg(feature = "net")] let bits = notifications::SOCKET_MASK | notifications::TIMER_MASK; #[cfg(not(feature = "net"))] @@ -215,20 +215,21 @@ fn main() -> ! { net_state.check_net(); } - if notif.has_timer_fired(notifications::TIMER_MASK) { - // Humility writes `1` to `HIFFY_KICK` - if HIFFY_KICK.load(Ordering::Acquire) == 0 { - sleeps += 1; + let timer_fired = notif.has_timer_fired(notifications::TIMER_MASK); - // Exponentially backoff our sleep value, but no more than 250ms - if sleeps == 10 { - sleep_ms = core::cmp::min(sleep_ms * 10, 250); - sleeps = 0; - } + // Humility writes `1` to `HIFFY_KICK` + if HIFFY_KICK.load(Ordering::Acquire) == 0 { + // If we were woken by the timer, rather than the net task, + // increment the number of times we have slept without being + // kicked. + sleeps += u32::from(timer_fired); - continue; + // Exponentially backoff our sleep value, but no more than 250ms + if sleeps == 10 { + sleep_ms = core::cmp::min(sleep_ms * 10, 250); + sleeps = 0; } - + } else { // // Whenever we have been kicked, we adjust our timeout down to 1ms, // from which we will exponentially backoff @@ -292,9 +293,20 @@ fn main() -> ! { } } } + + // If we were woken by the timer rather than the net task, reset the + // clock. + if timer_fired { + set_timer(sleep_ms); + } } } +fn set_timer(sleep_ms: u64) { + let deadline = sys_get_timer().now.saturating_add(sleep_ms); + sys_set_timer(Some(deadline), notifications::TIMER_MASK); +} + /// Converts an array pointer to a shared reference with a particular lifetime /// /// # Safety From d76e4f86e3d816444ad6322cfc9ca7cc5e28e82a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 24 May 2026 10:13:36 -0700 Subject: [PATCH 2/3] reset the timer if kicked from any source As @jamesmunns pointed out in https://github.com/oxidecomputer/hubris/pull/2530#discussion_r3295008059 --- task/hiffy/src/main.rs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index ee3b91289..5b6729ca1 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -215,14 +215,33 @@ fn main() -> ! { net_state.check_net(); } - let timer_fired = notif.has_timer_fired(notifications::TIMER_MASK); + // + // We shall reset the timer under either of the following conditions: + // + // 1. The previous deadline has elapsed (naturally), so that we + // can continue polling. This is the condition we check here. + // 2. We have been kicked and executed something, and the sleep + // duration has been reset. This is checked below. + // + // If the "net" feature is *not* enabled, this sounds suspiciously + // similar to just resetting the timer on every iteration of the loop, + // and in fact, it may as well be. However, if the "net" feature *is* + // enabled, we do not wish to reset the timer every time we wake up, + // as this means that notifications from `net` would reset the timer + // even if it has not yet completed. This way, we only reset the timer + // when we are woken by the timer *or* if we were kicked by a network + // RPC, rather than resetting it any time we receive a packet (or on + // spurious notifications from any source). + // + let mut should_reset_timer = + notif.has_timer_fired(notifications::TIMER_MASK); // Humility writes `1` to `HIFFY_KICK` if HIFFY_KICK.load(Ordering::Acquire) == 0 { // If we were woken by the timer, rather than the net task, // increment the number of times we have slept without being // kicked. - sleeps += u32::from(timer_fired); + sleeps += u32::from(should_reset_timer); // Exponentially backoff our sleep value, but no more than 250ms if sleeps == 10 { @@ -235,6 +254,7 @@ fn main() -> ! { // from which we will exponentially backoff // HIFFY_KICK.store(0, Ordering::Release); + should_reset_timer = true; sleep_ms = 1; sleeps = 0; @@ -294,9 +314,7 @@ fn main() -> ! { } } - // If we were woken by the timer rather than the net task, reset the - // clock. - if timer_fired { + if should_reset_timer { set_timer(sleep_ms); } } From 6fea81edcb320dc04bedc577315496dbf4da93ad Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 24 May 2026 11:05:44 -0700 Subject: [PATCH 3/3] hiffy: nicer ringbufs and counters --- Cargo.lock | 1 + lib/ringbuf/src/lib.rs | 17 ++++ task/hiffy/Cargo.toml | 5 +- task/hiffy/src/generic.rs | 5 -- task/hiffy/src/lpc55.rs | 19 ++--- task/hiffy/src/main.rs | 167 +++++++++++++++++++++++++++++++------- task/hiffy/src/stm32g0.rs | 5 -- task/hiffy/src/stm32h7.rs | 39 +-------- 8 files changed, 168 insertions(+), 90 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 19b984ceb..b149e1625 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5870,6 +5870,7 @@ dependencies = [ "byteorder", "cfg-if", "cortex-m", + "counters", "drv-hash-api", "drv-hf-api", "drv-i2c-api", diff --git a/lib/ringbuf/src/lib.rs b/lib/ringbuf/src/lib.rs index cb9c3d2c7..f31550e63 100644 --- a/lib/ringbuf/src/lib.rs +++ b/lib/ringbuf/src/lib.rs @@ -639,6 +639,23 @@ where } } +#[cfg(feature = "counters")] +impl CountedRingbuf +where + T: Count + Copy, +{ + /// Increment the counter for the provided variant of `T` in this + /// `CountedRingbuf`'s counters table, *without* isnerting it into the + /// ringbuffer itself. + /// + /// This is intended to be used in situations where it is desirable to count + /// some events without consuming ring buffer slots without creating a + /// separate counters table. + pub fn count(&self, entry: &T) { + entry.count(&self.counters); + } +} + impl RecordEntry for () where T: Copy + PartialEq, diff --git a/task/hiffy/Cargo.toml b/task/hiffy/Cargo.toml index 6de62de3d..4dce3bd98 100644 --- a/task/hiffy/Cargo.toml +++ b/task/hiffy/Cargo.toml @@ -15,7 +15,8 @@ drv-stm32xx-i2c = { path = "../../drv/stm32xx-i2c", optional = true } drv-stm32xx-sys-api = { path = "../../drv/stm32xx-sys-api", optional = true } task-net-api = { path = "../../task/net-api", optional = true } hubris-num-tasks = { path = "../../sys/num-tasks", features = ["task-enum"] } -ringbuf = { path = "../../lib/ringbuf" } +counters = { path = "../../lib/counters", features = ["derive"] } +ringbuf = { path = "../../lib/ringbuf" } static-cell = { path = "../../lib/static-cell" } userlib = { path = "../../sys/userlib" } test-api = { path = "../../test/test-api", optional = true} @@ -55,7 +56,7 @@ h743 = ["stm32h7", "drv-stm32xx-i2c/h743", "build-i2c/h743"] h753 = ["stm32h7", "drv-stm32xx-i2c/h753", "build-i2c/h753"] g031 = ["stm32g0", "drv-stm32xx-i2c/g031", "build-i2c/g031"] g030 = ["stm32g0", "drv-stm32xx-i2c/g030", "build-i2c/g030"] -micro = ["no-ipc-counters"] +micro = ["no-ipc-counters", "ringbuf/disabled"] # enables large memory buffer to make data transfers through hiffy (e.g. # `humility qspi` and such) not slow. this conflicts with the "micro" feature, # which makes the buffer smaller for memory constrained-targets, and is off by diff --git a/task/hiffy/src/generic.rs b/task/hiffy/src/generic.rs index a654168a8..5efd1045b 100644 --- a/task/hiffy/src/generic.rs +++ b/task/hiffy/src/generic.rs @@ -26,8 +26,3 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[ crate::common::send_lease_write, ]; -pub(crate) fn trace_execute(_offset: usize, _op: hif::Op) {} - -pub(crate) fn trace_success() {} - -pub(crate) fn trace_failure(_f: hif::Failure) {} diff --git a/task/hiffy/src/lpc55.rs b/task/hiffy/src/lpc55.rs index 8c08da9a4..fb2f1c7f1 100644 --- a/task/hiffy/src/lpc55.rs +++ b/task/hiffy/src/lpc55.rs @@ -2,30 +2,20 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use crate::Trace; #[cfg(feature = "spi")] use crate::common::{spi_read, spi_write}; use byteorder::ByteOrder; use drv_lpc55_gpio_api::*; use hif::*; use hubris_num_tasks::Task; -use ringbuf::*; +use ringbuf::ringbuf_entry_root; use userlib::*; #[cfg(feature = "gpio")] task_slot!(GPIO, gpio_driver); #[cfg(feature = "spctrl")] task_slot!(SP_CTRL, swd); - -#[derive(Copy, Clone, PartialEq)] -enum Trace { - None, - Execute((usize, hif::Op)), - Failure(Failure), - Success, -} - -ringbuf!(Trace, 64, Trace::None); - // TODO: this type is copy-pasted in several modules pub struct Buffer(#[allow(dead_code)] u8); @@ -246,6 +236,9 @@ fn gpio_configure( let task = GPIO.get_task_id(); let gpio = drv_lpc55_gpio_api::Pins::from(task); + ringbuf_entry_root!(Trace::GpioConfigure( + pin, alt, mode, slew, invert, digimode, opendrain + )); gpio.iocon_configure( pin, alt, mode, slew, invert, digimode, opendrain, None, ); @@ -310,6 +303,8 @@ fn gpio_input( let pin = gpio_args(stack)?; + ringbuf_entry_root!(Trace::GpioInput(pin)); + let input = gpio.read_val(pin); byteorder::LittleEndian::write_u16(rval, input as u16); diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index 5b6729ca1..e61146bad 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -17,6 +17,7 @@ use core::sync::atomic::{AtomicU32, Ordering}; use hif::*; +use ringbuf::{counted_ringbuf, ringbuf_entry}; use static_cell::*; use userlib::*; @@ -185,6 +186,83 @@ pub static HIFFY_VERSION_MINOR: AtomicU32 = AtomicU32::new(HIF_VERSION_MINOR); #[used] pub static HIFFY_VERSION_PATCH: AtomicU32 = AtomicU32::new(HIF_VERSION_PATCH); +#[derive(Copy, Clone, PartialEq, counters::Count)] +enum Trace { + #[count(skip)] + None, + // + // ==== Traces for executing HIF operations ==== + // + Execute(usize, Op), + // TODO(eliza): would like to be able to put `#[count(children)]` on this, + // but that requires `hif` to derive `Count` for `Failure`... + Failure(Failure), + Success, + // + // ==== Traces for GPIO operations (STM32H7 version) ==== + // + #[cfg(all(feature = "gpio", feature = "stm32h7"))] + GpioConfigure( + drv_stm32xx_sys_api::Port, + u16, + drv_stm32xx_sys_api::Mode, + drv_stm32xx_sys_api::OutputType, + drv_stm32xx_sys_api::Speed, + drv_stm32xx_sys_api::Pull, + drv_stm32xx_sys_api::Alternate, + ), + #[cfg(all(feature = "gpio", feature = "stm32h7"))] + GpioInput(drv_stm32xx_sys_api::Port), + // + // ==== Traces for GPIO operations (LPC55 version) ==== + // + #[cfg(all(feature = "gpio", feature = "lpc55"))] + GpioConfigure( + drv_lpc55_gpio_api::Pin, + drv_lpc55_gpio_api::AltFn, + drv_lpc55_gpio_api::Mode, + drv_lpc55_gpio_api::Slew, + drv_lpc55_gpio_api::Invert, + drv_lpc55_gpio_api::Digimode, + drv_lpc55_gpio_api::Opendrain, + ), + #[cfg(all(feature = "gpio", feature = "lpc55"))] + GpioInput(drv_lpc55_gpio_api::Pin), + // + // ==== Traces for net RPC ==== + // + #[cfg(feature = "net")] + NetRecvPacket(task_net_api::UdpMetadata), + #[cfg(feature = "net")] + NetRecvErr(#[count(children)] task_net_api::RecvError), + #[cfg(feature = "net")] + NetSendErr(#[count(children)] task_net_api::SendError), + #[cfg(feature = "net")] + NetRpcRequest(#[count(children)] net::RpcOp), + #[cfg(feature = "net")] + NetRpcReply(#[count(children)] net::RpcReply), + // + // ==== Traces for notification sources ==== + // + // Note that some of these variants are *not* recorded in the ring buffer, + // and are only used with the `CountedRingbuf::count` method, to record them + // in the counters table. This is because we would like to count events that + // occur frequently (such as being woken up by the timer) without pushing + // more interesting events out of the ring buffer. However, we would like to + // have a single counters table for all traces we record, whether or not + // they are actually put in the ringbuf. So, these variants are not going to + // appear in the actual ringbuf, but will appear in its counters. + // + Notified, + #[cfg(feature = "net")] + NotifiedSocket, + NotifiedTimer, + Kicked, + NotKicked, +} + +counted_ringbuf!(Trace, 64, Trace::None); + #[unsafe(export_name = "main")] fn main() -> ! { let mut sleep_ms = 250; @@ -209,9 +287,11 @@ fn main() -> ! { let notif = sys_recv_notification(bits); HIFFY_READY.store(0, Ordering::Relaxed); + __RINGBUF.count(&Trace::Notified); #[cfg(feature = "net")] if notif.check_notification_mask(notifications::SOCKET_MASK) { + __RINGBUF.count(&Trace::NotifiedSocket); net_state.check_net(); } @@ -233,11 +313,15 @@ fn main() -> ! { // RPC, rather than resetting it any time we receive a packet (or on // spurious notifications from any source). // - let mut should_reset_timer = - notif.has_timer_fired(notifications::TIMER_MASK); + let mut should_reset_timer = false; + if notif.has_timer_fired(notifications::TIMER_MASK) { + __RINGBUF.count(&Trace::NotifiedTimer); + should_reset_timer = true; + } // Humility writes `1` to `HIFFY_KICK` if HIFFY_KICK.load(Ordering::Acquire) == 0 { + __RINGBUF.count(&Trace::NotKicked); // If we were woken by the timer, rather than the net task, // increment the number of times we have slept without being // kicked. @@ -257,9 +341,10 @@ fn main() -> ! { should_reset_timer = true; sleep_ms = 1; sleeps = 0; + ringbuf_entry!(Trace::Kicked); let check = |offset: usize, op: &Op| -> Result<(), Failure> { - trace_execute(offset, *op); + ringbuf_entry!(Trace::Execute(offset, *op)); Ok(()) }; let rv = { @@ -296,7 +381,7 @@ fn main() -> ! { let prev = HIFFY_REQUESTS.load(Ordering::Relaxed); HIFFY_REQUESTS .store(prev.wrapping_add(1), Ordering::Release); - trace_success(); + ringbuf_entry!(Trace::Success); } Err(failure) => { // SAFETY: We are in single-threaded code and the debugger will @@ -308,7 +393,7 @@ fn main() -> ! { let prev = HIFFY_ERRORS.load(Ordering::Relaxed); HIFFY_ERRORS .store(prev.wrapping_add(1), Ordering::Release); - trace_failure(failure); + ringbuf_entry!(Trace::Failure(failure)); } } } @@ -358,12 +443,14 @@ unsafe fn bind_lifetime_mut<'a, const N: usize>( #[cfg(feature = "net")] mod net { use super::{ - HIFFY_DATA, HIFFY_KICK, HIFFY_TEXT, bind_lifetime_mut, notifications, + HIFFY_DATA, HIFFY_KICK, HIFFY_TEXT, Trace, bind_lifetime_mut, + notifications, }; use core::sync::atomic::Ordering; + use ringbuf::ringbuf_entry_root; use static_cell::ClaimOnceCell; use task_net_api::{ - LargePayloadBehavior, RecvError, SendError, SocketName, UdpMetadata, + LargePayloadBehavior, SendError, SocketName, UdpMetadata, }; use userlib::{FromPrimitive, UnwrapLite, sys_recv_notification}; use zerocopy::{FromBytes, IntoBytes, LittleEndian, U16, U32, U64}; @@ -390,17 +477,19 @@ mod net { } const CURRENT_VERSION: u16 = 1; - #[derive(Copy, Clone, Debug, FromPrimitive)] + #[derive( + Copy, Clone, Debug, FromPrimitive, Eq, PartialEq, counters::Count, + )] #[repr(u16)] - enum RpcOp { + pub(super) enum RpcOp { WriteHiffyText = 1, WriteHiffyData, HiffyKick, } - #[derive(Copy, Clone, Debug)] + #[derive(Copy, Clone, Debug, Eq, PartialEq, counters::Count)] #[repr(u8)] - enum RpcReply { + pub(super) enum RpcReply { Ok = 0u8, /// The RPC packet was too short to include the complete header TooShort, @@ -449,8 +538,12 @@ mod net { LargePayloadBehavior::Discard, self.rx_data_buf, ) { - Ok(meta) => self.handle_packet(meta), - Err(RecvError::QueueEmpty | RecvError::ServerRestarted) => { + Ok(meta) => { + ringbuf_entry_root!(Trace::NetRecvPacket(meta)); + self.handle_packet(meta); + } + Err(err) => { + ringbuf_entry_root!(Trace::NetRecvErr(err)); // Our incoming queue is empty or `net` restarted. Wait for // more packets in dispatch, back in the main loop. } @@ -462,6 +555,8 @@ mod net { // `handle_packet_inner` does not write to it! let tx_data_buf = core::mem::take(&mut self.tx_data_buf); let (r, data) = self.handle_packet_inner(meta); + ringbuf_entry_root!(Trace::NetRpcReply(r)); + tx_data_buf[0] = r as u8; tx_data_buf[1..][..data.len()].copy_from_slice(data); meta.size = (1 + data.len()) as u32; @@ -473,18 +568,25 @@ mod net { &self.tx_data_buf[..(meta.size as usize)], ) { Ok(()) => break, - // If `net` just restarted, immediately retry our send. - Err(SendError::ServerRestarted) => continue, - // If our tx queue is full, wait for space. This is the - // same notification we get for incoming packets, so we - // might spuriously wake up due to an incoming packet - // (which we can't service anyway because we are still - // waiting to respond to a previous request); once we - // finally succeed in sending we'll peel any queued - // packets off our recv queue at the top of our main - // loop. - Err(SendError::QueueFull) => { - sys_recv_notification(notifications::SOCKET_MASK); + Err(e) => { + ringbuf_entry_root!(Trace::NetSendErr(e)); + match e { + // If `net` just restarted, immediately retry our send. + SendError::ServerRestarted => continue, + // If our tx queue is full, wait for space. This is + // the same notification we get for incoming + // packets, so we might spuriously wake up due to an + // incoming packet (which we can't service anyway + // because we are still waiting to respond to a + // previous request); once we finally succeed in + // sending we'll peel any queued packets off our + // recv queue at the top of our main loop. + SendError::QueueFull => { + sys_recv_notification( + notifications::SOCKET_MASK, + ); + } + } } } } @@ -509,9 +611,15 @@ mod net { return (RpcReply::BadVersion, CURRENT_VERSION.as_bytes()); } + // Decode the RPC operation. + let Some(op) = RpcOp::from_u16(header.operation.get()) else { + return (RpcReply::InvalidOperation, &[]); + }; + ringbuf_entry_root!(Trace::NetRpcRequest(op)); + // Perform the actual operation - match RpcOp::from_u16(header.operation.get()) { - Some(RpcOp::WriteHiffyText) => { + match op { + RpcOp::WriteHiffyText => { // Dummy object to bind references to a non-static lifetime let lifetime = (); let offset = header.arg.get() as usize; @@ -534,7 +642,7 @@ mod net { (RpcReply::OutOfRange, &[]) } } - Some(RpcOp::WriteHiffyData) => { + RpcOp::WriteHiffyData => { // Dummy object to bind references to a non-static lifetime let lifetime = (); let offset = header.arg.get() as usize; @@ -557,11 +665,10 @@ mod net { (RpcReply::OutOfRange, &[]) } } - Some(RpcOp::HiffyKick) => { + RpcOp::HiffyKick => { HIFFY_KICK.fetch_add(1, Ordering::SeqCst); (RpcReply::Ok, &[]) } - None => (RpcReply::InvalidOperation, &[]), } } } diff --git a/task/hiffy/src/stm32g0.rs b/task/hiffy/src/stm32g0.rs index 9bbb77a91..64fb5a9b7 100644 --- a/task/hiffy/src/stm32g0.rs +++ b/task/hiffy/src/stm32g0.rs @@ -462,8 +462,3 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[ #[unsafe(no_mangle)] pub static HIFFY_FUNCTIONS: Option<&Functions> = None; -pub(crate) fn trace_execute(_offset: usize, _op: hif::Op) {} - -pub(crate) fn trace_success() {} - -pub(crate) fn trace_failure(_f: hif::Failure) {} diff --git a/task/hiffy/src/stm32h7.rs b/task/hiffy/src/stm32h7.rs index 22999e190..8d6d7ccaa 100644 --- a/task/hiffy/src/stm32h7.rs +++ b/task/hiffy/src/stm32h7.rs @@ -2,6 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use crate::Trace; #[cfg(feature = "hash")] use crate::common::{ hash_digest_sha256, hash_finalize_sha256, hash_init_sha256, hash_update, @@ -25,28 +26,6 @@ task_slot!(I2C, i2c_driver); #[cfg(feature = "gpio")] task_slot!(SYS, sys); -#[derive(Copy, Clone, PartialEq)] -enum Trace { - None, - Execute((usize, Op)), - Failure(Failure), - #[cfg(feature = "gpio")] - GpioConfigure( - drv_stm32xx_sys_api::Port, - u16, - drv_stm32xx_sys_api::Mode, - drv_stm32xx_sys_api::OutputType, - drv_stm32xx_sys_api::Speed, - drv_stm32xx_sys_api::Pull, - drv_stm32xx_sys_api::Alternate, - ), - #[cfg(feature = "gpio")] - GpioInput(drv_stm32xx_sys_api::Port), - Success, -} - -ringbuf!(Trace, 64, Trace::None); - // Field is only used in the debugger, appears dead to the compiler. pub struct Buffer(#[allow(dead_code)] u8); @@ -397,7 +376,7 @@ fn gpio_input( None => return Err(Failure::Fault(Fault::EmptyParameter(0))), }; - ringbuf_entry!(Trace::GpioInput(port)); + ringbuf_entry_root!(Trace::GpioInput(port)); let input = gpio.gpio_read_input(port); @@ -513,7 +492,7 @@ fn gpio_configure( let gpio = drv_stm32xx_sys_api::Sys::from(task); #[rustfmt::skip] - ringbuf_entry!( + ringbuf_entry_root!( Trace::GpioConfigure(port, mask, mode, output_type, speed, pull, af) ); @@ -584,15 +563,3 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[ // #[unsafe(no_mangle)] pub static HIFFY_FUNCTIONS: Option<&Functions> = None; - -pub(crate) fn trace_execute(offset: usize, op: hif::Op) { - ringbuf_entry!(Trace::Execute((offset, op))); -} - -pub(crate) fn trace_success() { - ringbuf_entry!(Trace::Success); -} - -pub(crate) fn trace_failure(f: hif::Failure) { - ringbuf_entry!(Trace::Failure(f)); -}