-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update Spi component to return a status on device access #4452
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
base: devel
Are you sure you want to change the base?
Conversation
thomas-bc
left a comment
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.
Looks good! Let's address the TODOs and we should be good to go
|
Ah also you will need to format your files. This can be done with this one-liner: |
thomas-bc
left a comment
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.
Looks good to me, but I'd like a second set of eyes to run through the comment below.
cc @LeStarch
| Fw::Buffer& readBuffer) { | ||
| FW_ASSERT(portNum >= 0, static_cast<FwAssertArgType>(portNum)); | ||
| FW_ASSERT(writeBuffer.isValid()); | ||
| FW_ASSERT(readBuffer.isValid()); |
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.
I believe this should be ok; but also Googling around seems to suggest there might be use cases for unidirectional SPI, so I wonder if we really should assert here? I'd like to get someone else's eyes on this to confirm, since we weren't asserting before.
Change Description
SpiStatusto include values for each error already noted inEvents.fppiDrv/Interfaces/Spi.fpp)Rationale
This would bring it in line with other similar drivers like I2C.
Testing/Review Recommendations
Unit Test is currently broken. Should be fixed to do one or several of:
Future Work
Issue #4470 captures more work to go on this component, including
AI Usage (see policy)
N/A