transceivers: fix I2C read bug #1787
Merged
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
hawkw
reviewed
May 20, 2024
mkeeter
reviewed
May 20, 2024
drv/transceivers-server/src/main.rs
Outdated
| // - SFF-8636 rev 2.10a, Section 6.2.4 | ||
| Ok(Celsius(out.temperature.get() as f32 / 256.0)) | ||
| } else { | ||
| Err(FpgaError::ImplError(status.as_bytes()[0])) |
There was a problem hiding this comment.
What is this doing? I think it's going to return the value of rdata_fifo_empty, because it's casting an PortI2CStatus to a &[u8] and taking the first byte (which happens to be rdata_fifo_empty: bool)
There was a problem hiding this comment.
Good catch! I actually don't care about the whole status here, I really just want to log the error observed. I'm going to make this line:
Suggested change
| Err(FpgaError::ImplError(status.as_bytes()[0])) | |
| Err(FpgaError::ImplError(status.error.as_bytes()[0])) |
And get rid of the AsBytes deriviation on the PortI2CStatus type.
There was a problem hiding this comment.
One more note: it looks like you can do status.error as u8 instead of status.error.as_bytes()[0], since it's already a single-byte enum type.
mkeeter
approved these changes
May 20, 2024
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.
#1768 did not properly account for the FIFO behavior of the FPGA's data buffers. The "check the status byte" portion of the loop happened outside the part where we read the buffer, and since the buffer was just memory-mapped registers it could be repeatedly without consequence. Since the data was now in a FIFO, I was inadvertently draining the FIFO before the transaction was done. This PR consolidates the "is I2C done yet" logic into the
get_i2c_status_and_read_bufferso calling code can just deal with the status register and the data buffer.Fixes #1786