-
Notifications
You must be signed in to change notification settings - Fork 269
Conversation
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.
Is this change really needed for ExclusiveDevice? The scenario described in #573 doesn't apply here, as the bus isn't shared in this case.
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.
The issue is less severe with ExclusiveDevice, but there is still a problem. The SpiDevice trait says that all transactions must start by asserting the chipselect (a falling edge), which isn't possible if it is already asserted. This is important for devices that use chipselect for framing. In practice it is possible to work around the issue in this case (always de-assert cs again if you get any kind of DeviceError) but it is a pretty big footgun
GrantM11235
commented
Jan 30, 2024
The docs and changelog are done. I am open to suggestions for the name of UnwrappingAdapter if anyone has any better ideas, but I think it is okay as it is.
This PR was brought up at the meeting today, but no one had much to say about it. I assume that means there are no strong objections.
Dirbaio
commented
Feb 13, 2024
i'm personally 50/50 on this. The tradeoffs are subtle and i'm not sure which side of them is the best. So I won't merge this myself but I'm OK with merging if other people in the HAL team are in favor.
@MabezDev
MabezDev
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.
I did mean to leave a review a couple of weeks ago, but forgot, sorry!
Imo, given that this is not in ehal proper, and if required the users who need poisoning can write their own bus wrapper (and maybe PR it back here?), this LGTM.
ehal bus is not ehal and we can be a bit more selective on what level of support we want to provide out of the box :D.
@Dirbaio
Dirbaio
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.
After discussion in today's meeting I now believe we should not require infallible pins, we shold instead keep the current implementation, document how it handles errors and recommend the user use another implementation if it doesn't suit their requirements.
the TLDR is
- This doesn't fix the full problem: there's a similar case where
flushfails so the SPI keeps clocking out buffered bits, we deassert CS then unlock the bus, another driver asserts their CS and now the buffered bits go to the wrong device. Therefore we should require infallible SPI too, but that's not practical- @GrantM11235 proposed an alternative: specify
flushmust not return before SPI is stopped in ALL cases, even on failure, but I don't believe this is generally feasible and I don't think it's worth complicating the SpiBus contract to accomodate for SpiDevice implementation technicalities.
- @GrantM11235 proposed an alternative: specify
- Therefore we should apply the same "policy" to SPI and CS errors. Not doing so would be inconsistent. However most SPIs out there are fallible, so this would be quite burdensome for the user.
- the 3rd alternative (poisoning) is too complex, it requires wrapping the SPI bus into an "inner" struct for all users even if they're not affected by this problem. It's also not zero cost.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #573