Skip to content

Commit

Permalink
gimlet-seq: Name SetState ringbuf entry fields
Browse files Browse the repository at this point in the history
Currently, the `Trace::SetState` ringbuf entry in the sequencer is a
tuple-like enum variant. This entry includes two `PowerState` fields,
one recording the previous power state and the other recording the new
power state that has been set. IMHO, using a tuple-like variant to
represent this is a bit unfortunate, as in Humility, we'll see two
values of the same type and it's not immediately obvious which is the
previous state and which is the new state. This must be determined based
on the order of the fields in the ringbuf entry, which requires
referencing the Hubris code to determine.

I felt like it was nicer to just use a struct-like variant with named
fields for this. That way, the semantic meaning of the two `PowerState`s
is actually encoded in the debug info, and Humility can just indicate
which is the previous state and which is the new state when displaying
the ring buffer. I also think it's a bit nicer to name the timestamp
field --- otherwise, it just looks like some arbitrary integer, and you
need to look at the code to determine that it's the timestamp of the
power state change.

If this is controversial for some reason, I'm happy to land it in a
separate PR, but I figured it was nice to do while I was messing with
the sequencer ringbuf.
  • Loading branch information
hawkw committed Dec 14, 2024
1 parent 436f5dd commit 7b43a72
Showing 1 changed file with 14 additions and 8 deletions.
22 changes: 14 additions & 8 deletions drv/gimlet-seq-server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,13 @@ enum Trace {
RailsOn,
UartEnabled,
A0(u16),
SetState(
PowerState,
PowerState,
#[count(children)] StateChangeReason,
u64,
),
SetState {
prev: PowerState,
next: PowerState,
#[count(children)]
why: StateChangeReason,
now: u64,
},
UpdateState(#[count(children)] PowerState),
ClockConfigWrite,
ClockConfigSuccess,
Expand Down Expand Up @@ -674,12 +675,17 @@ impl<S: SpiServer> ServerImpl<S> {
fn set_state_internal(
&mut self,
state: PowerState,
reason: StateChangeReason,
why: StateChangeReason,
) -> Result<(), SeqError> {
let sys = sys_api::Sys::from(SYS.get_task_id());

let now = sys_get_timer().now;
ringbuf_entry!(Trace::SetState(self.state, state, reason, now));
ringbuf_entry!(Trace::SetState {
prev: self.state,
next: state,
why,
now
});

ringbuf_entry_v3p3_sys_a0_vout();

Expand Down

0 comments on commit 7b43a72

Please sign in to comment.