gimlet_seq: separate error type for read_fpga_regs #1600
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Inspired by [this comment][1] from @cbiffle. Currently, the Gimlet Sequencer API has one error enum that's returned by all API methods. This enum contains a bunch of error variants that are specific to actually setting the sequencer state, and a `ReadRegsFailed` variant that's returned by the `read_fpga_regs` API method. This means that, currently, calls to set the sequencer state have to handle the `ReadRegsFailed` variant, even though it's never actually returned by the code that sets the sequencer state, and, conversely, code that reads the FPGA registers has to handle a bunch of error variants that aren't related to FPGA register reads. This commit separates the error types of the `read_fpga_regs` and `set_state` APIs into two separate enums that only contain the errors returned by that API. I didn't touch the other API methods, although (as far as I can tell), `get_state`, `fans_on`, and `fans_off` never actually return any errors and could be changed to `core::convert::Infallible` a la `send_hardware_nmi`. I didn't want to mess with these, though, based on the assumption that the `SeqError` error type was chosen to avoid having to change the API if these methods changed to return errors. Let me know if that's not the case --- I'm happy to change them as well. [1]: #1597 (comment)
cbiffle
reviewed
Mar 7, 2024
| /// Errors returned by `read_fpga_regs`. | ||
| #[derive(Copy, Clone, Debug, FromPrimitive, Eq, PartialEq, IdolError)] | ||
| pub enum ReadFpgaRegsError { | ||
| Failed = 1, |
There was a problem hiding this comment.
Maybe add a comment noting that this variant is probably illusory because of the SPI error API issue, with a TODO to revisit it.
cbiffle
approved these changes
Mar 7, 2024
hawkw
added a commit
that referenced
this pull request
Mar 25, 2024
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
added a commit
that referenced
this pull request
Mar 29, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by this comment from @cbiffle.
Currently, the Gimlet Sequencer API has one error enum that's returned
by all API methods. This enum contains a bunch of error variants that
are specific to actually setting the sequencer state, and a
ReadRegsFailedvariant that's returned by theread_fpga_regsAPImethod. This means that, currently, calls to set the sequencer state
have to handle the
ReadRegsFailedvariant, even though it's neveractually returned by the code that sets the sequencer state, and,
conversely, code that reads the FPGA registers has to handle a bunch of
error variants that aren't related to FPGA register reads.
This commit separates the error types of the
read_fpga_regsandset_stateAPIs into two separate enums that only contain the errorsreturned by that API. I didn't touch the other API methods, although (as
far as I can tell),
get_state,fans_on, andfans_offneveractually return any errors and could be changed to
core::convert::Infalliblea lasend_hardware_nmi. I didn't want tomess with these, though, based on the assumption that the
SeqErrorerror type was chosen to avoid having to change the API if these methods
changed to return errors. Let me know if that's not the case --- I'm
happy to change them as well.