-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
spi: A reSPIte from excessive error codes #1656
spi: A reSPIte from excessive error codes #1656
Conversation
7a37449
to
2cf1702
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't actually change the definition of I can also change the discriminants for the Hubris error enums back, if you like, but (AFAICT) it shouldn't make a difference? |
FWIW (maybe less than nothing), this is why we landed on "duplicate the types and implement |
Nope, it's all good! Leave it as is. As @jgallagher reminded me, we did this on purpose :D |
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.
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.
Currently, the SPI API has a single big error enum which it returns from
all IPCs. This isn't all that great: for example, a
NothingToRelease
error can be returned from the
Spi::read
IPC, which...doesn't make aton of sense. Additionally, many of these error variants are returned
only in the event of a programmer error in a client task, rather than
expected runtime errors, and probably should be
reply-fault
s ratherthan error codes.
Per @cbiffle on Matrix:
Therefore, I've refactored the
SpiError
type to remove theNothingToRelease
andBadDevice
variants, and changed theSpi
IPCinterface so that the
lock
andrelease
APIs only return runtimeerrors on server death --- the
SpiError
type is now only returned bySpi::read
,Spi::write
, andSpi::exchange
. In situations whereBadDevice
orNothingToRelease
errors would previously have beenreturned, the SPI server now sends a reply-fault instead.
The SPI API is a bit different from other driver APIs, as there's a
provision for running the SPI driver locally when a task has exclusive
ownership over a specific SPI bus, as well as running it as its own
driver task. In this case, the cases which would return reply-fault when
the driver is its own task now panic when the SPI interface's driver is
local to another task.
This reduces the total archive size for
gimlet/rev-f.toml
by 1%(according to
cargo xtask sizes
), which is, honestly, not that much.But, it still seems worthwhile.