Skip to content
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

e-h-bus/spi: Require infallible chipselect pins #574

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GrantM11235
Copy link
Contributor

@GrantM11235 GrantM11235 commented Jan 23, 2024

Fixes #573

  • Require infallible chipselect pins
  • Add an unwrapping adapter
    • Think of a good name for the adapter
  • Add documentation
  • Update changelog

@GrantM11235 GrantM11235 requested a review from a team as a code owner January 23, 2024 21:13
}

impl<Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for ExclusiveDevice<BUS, CS, D>
where
BUS: SpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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 previously approved these changes Feb 13, 2024
Copy link
Member

@MabezDev MabezDev left a 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.

Copy link
Member

@Dirbaio Dirbaio left a 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 flush fails 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 flush must 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.
  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The SPI sharing utilities are broken for fallible chipselect pins
4 participants