gimlet_seq: Make the sequencer API less fallible

[hawkw]

Currently, the gimlet_seq IPC API has a bunch of IPCs that return errors that aren't really necessary. The SeqError enum is returned by all the sequencer's IPCs, but most of its variants are only ever relevant to the Sequencer::set_state IPC. The
Sequencer::read_fpga_regs only ever returns a
SeqError::ReadRegsFailed error if the SPI driver returns an error, and the other IPCs don't return errors at all.

This branch cleans this up a bit by removing all the error return values from the IPC operations that don't return errors. Now that PR #1656 has been merged, the Spi::read IPC now only errors if the SPI driver is restarted or if the transfer is too large. Since
Sequencer::read_fpga_regs only does small reads, we can now unwrap this and make that IPC infallible as well. And, since the Sequencer::read_fpga_regs and Sequencer::get_state just read a value and return it, I figured that we could make them idempotent, as well. This makes the IPC interface much simpler and allows several callers of it to avoid handling errors that aren't ever actually returned.

Closes #1600, which this obsoletes.

[hawkw]

surprisingly, this actually made gimlet_inspector a couple bytes bigger than it was before, i guess because the idol API will now do retries

[hawkw]

@cbiffle if the changes to mark some of these APIs as idempotent seems unsafe to you, I can back it out --- AFAICT it seems fine but would love a second pair of eyes.

[mkeeter]

After reading through the code, I agree that this should always be safe, but boy is it hard to check that.

I'd suggest adding some constants and static_assertion::const_assert!:

diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs
index 84f90a56b..ac3f5c7a5 100644
--- a/drv/gimlet-seq-server/src/main.rs
+++ b/drv/gimlet-seq-server/src/main.rs
@@ -1025,11 +1025,14 @@ impl<S: SpiServer> idl::InOrderSequencerImpl for ServerImpl<S> {
         _: &RecvMessage,
     ) -> Result<[u8; 64], RequestError<SeqError>> {
         let mut buf = [0; 64];
-        let size = 8;
+        const CHUNK_SIZE: usize = 8;
+        static_assertions::const_assert!(
+            CHUNK_SIZE <= seq_spi::MAX_SPI_CHUNK_SIZE
+        );
 
-        for i in (0..buf.len()).step_by(size) {
+        for i in (0..buf.len()).step_by(CHUNK_SIZE) {
             self.seq
-                .read_bytes(i as u16, &mut buf[i..i + size])
+                .read_bytes(i as u16, &mut buf[i..i + CHUNK_SIZE])
                 .map_err(|_| SeqError::ReadRegsFailed)?;
         }
 
diff --git a/drv/gimlet-seq-server/src/seq_spi.rs b/drv/gimlet-seq-server/src/seq_spi.rs
index dbe0fcadd..4136af806 100644
--- a/drv/gimlet-seq-server/src/seq_spi.rs
+++ b/drv/gimlet-seq-server/src/seq_spi.rs
@@ -24,6 +24,14 @@ include!(env!("GIMLET_FPGA_REGS"));
 
 pub const EXPECTED_IDENT: u16 = 0x1DE;
 
+/// Local buffer size for chunked reads and writes
+const RAW_SPI_BUFFER_SIZE: usize = 16;
+
+/// Available space in the chunked read/write buffer for user data
+#[allow(unused)] // only used in static assertions, which are compiled out
+pub const MAX_SPI_CHUNK_SIZE: usize =
+    RAW_SPI_BUFFER_SIZE - core::mem::size_of::<CmdHeader>();
+
 pub struct SequencerFpga<S: SpiServer> {
     spi: SpiDevice<S>,
 }
@@ -135,8 +143,8 @@ impl<S: SpiServer> SequencerFpga<S> {
         addr: u16,
         data_out: &mut [u8],
     ) -> Result<(), spi_api::SpiError> {
-        let mut data = [0u8; 16];
-        let mut rval = [0u8; 16];
+        let mut data = [0u8; RAW_SPI_BUFFER_SIZE];
+        let mut rval = [0u8; RAW_SPI_BUFFER_SIZE];
 
         let addr = U16::new(addr);
         let header = CmdHeader { cmd, addr };
@@ -165,8 +173,8 @@ impl<S: SpiServer> SequencerFpga<S> {
         addr: u16,
         data_in: &[u8],
     ) -> Result<(), spi_api::SpiError> {
-        let mut data = [0u8; 16];
-        let mut rval = [0u8; 16];
+        let mut data = [0u8; RAW_SPI_BUFFER_SIZE];
+        let mut rval = [0u8; RAW_SPI_BUFFER_SIZE];
 
         let addr = U16::new(addr);
         let header = CmdHeader { cmd, addr };

There's also a possible error if we pass the wrong device ID to spi.exchange(..) way down in the call stack, but that should never happen.

[hawkw]

@cbiffle and I had previously discussed either changing the SPI driver to expose a "max size" constant that client tasks can check before calling into the API, or have the SPI driver automatically chunk buffers that are too big, so the error no longer need exist. I'd be happy to go down one of those paths as well.

[hawkw]

ah, upon further investigation, it seems like the only times the SPI driver returns BadTransferSize are:

• size > u16::max
• size == 0

So...8 bytes should, in fact, never be too big for the SPI driver. But we should probably actually document this.

[mkeeter]

Right, but I think this call goes through SequencerFpga::read_bytes → SequencerFpga::raw_spi_read on the way to the SPI driver, so the limit is lower (determined by the internal buffers in raw_spi_read)?

[hawkw]

Oh, yup, I get it now --- thanks!

[hawkw]

@mkeeter I've applied your suggestions in 2ca7031.

It occurred to me, though, that we could also avoid the need to static_assert! here if we simply changed read_fpga_regs to use seq_spi's MAX_CHUNK_SIZE as the size of the chunk, rather than performing exactly 8-byte reads every time. I didn't make this change as it seems like it makes the code a bit more complex --- if we perform reads that are not exactly 8 bytes, the last chunk may be smaller than the rest, so we would need to handle that case. I'm not sure if this is worth the effort or not --- WDYT?

[mkeeter]

Yeah, I was considering that, but decided to err on the side of changing as few things as possible.