e-h-bus/spi: Require infallible chipselect pins #574
Conversation
| } | ||
|
|
||
| impl<Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for ExclusiveDevice<BUS, CS, D> | ||
| where | ||
| BUS: SpiBus<Word>, | ||
| CS: OutputPin, | ||
| CS: OutputPin<Error = Infallible>, |
There was a problem hiding this comment.
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.
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
|
The docs and changelog are done. I am open to suggestions for the name of 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. |
|
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #573