The SPI sharing utilities are broken for fallible chipselect pins
Nobody has claimed this yet.
Assessment
- Difficulty
- 5/5
- Estimated time
- Over a week
- Newbie friendliness
- 25/100
- Issue type
- Bug
- Clarity
- Needs clarification
- Activity status
- Stale
- Tech stack
- rust
- Domain
- embedded-iot
Research direction
Start by locating the SPI sharing utilities and reviewing how they handle fallible chipselect errors. Compare the three options in the issue, then confirm the intended policy with maintainers; done requires an agreed approach that prevents concurrent transactions after chipselect failure and corresponding tests or documentation.
Written by the indexing model from the issue text.
Description
This affects all the spi sharing utilities in some way, but I would like to highlight the worst case scenario:
- You are using the mutex or critical section impl
- In thread 1 you do a transaction to device A, but the cs pin fails to deassert
- Before thread 1 even has a chance to react to the error, it is preempted by thread 2
- Thread 2 does a transaction to device B while the cs for device A is still active
- This leads to data corruption at best, and physical damage at worst
Option 1: Infallible only
CS: OutputPin<Error = Infallible>
This is the simplest option. In fact it is even simpler than the existing impls because it allows us to remove the DeviceError enum.
You can still use fallible chipselect pins if you wrap them in some sort of adapter that panics on failure. We could even provide such an adapter.
Option 2: track "poisoned" chipselects
I have some idea how to implement this, it would involve adding a shared flag to each bus, as well as another flag for each SpiDevice. The impl would refuse to do any spi transactions until the offending chipselect is fixed.
This would be a lot of extra overhead and complexity that is completely unnecessary for 99.9% of users. I don't think the compiler will be able to remove the unnecessary overhead.
Option 3: do nothing
Document the broken error handling, how to work around it, and where it is impossible to work around.
If we want to be thorough, the amount of warnings and disclaimers will probably outweigh the amount of actual code:
expand me!
- how to detect a failed chipselect
DeviceError::Cs, obviously, but alsoDeviceError::Spi, because it can "hide" chipselect errors- basically any
SpiDeviceerror - downstream errors such as
DisplayError::BusWriteErrorfromdisplay_interface(but ironically notDisplayError::CSError)
- how to properly recover from a chipselect error
- before using any device on the bus:
- get access to the
SpiDeviceby destroying whichever driver was using it - get access to the chipselect by destroying the
SpiDevice - do whatever you need to do to fix and deassert the chipselect pin
- get access to the
- in multitheaded/concurrent code, it is impossible to ensure that this is done before another thread uses a device on the bus. It is not possible to correctly handle chipselect failure in this case
- before using any device on the bus:
- what happens if you use a device on the bus before fixing the chipselect error
- this is even a minor problem for
ExclusiveDevicebecause the framing will be messed up - if two chipselects are active at once
- the other device will receive garbage data
- both devices will fight to drive MISO, causing data corruption and possibly even physical damage
- this is even a minor problem for
Conclusions
If you only have infallible chipselect pins, or if you want to panic on chipselect failure, option 1 is the best.
If you actually want to try to gracefully recover from chipselect errors, option 2 is the only real option.
Option 3 isn't good for either group. It is slightly inconvenient for the first group, and it is almost impossible to use correctly for the second group
My recommendation is option 1 because it is the best option for most people. If someone really needs option 2, they can write it for themself, it doesn't need to be in our crate.
- Dominant language
- Rust
- Stars
- 2.7k
- Forks
- 282
- PR merge metrics
- No merged PRs in 30d
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from rust-embedded/embedded-hal
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
rust-embedded/embedded-hal#742 · 1 comment ·
-
Difficulty 5/5 Over a week Newbie friendliness 30/100
rust-embedded/embedded-hal#747 · 5 comments · 1 reaction ·
-
Difficulty 5/5 Over a week Newbie friendliness 35/100
rust-embedded/embedded-hal#746 · 2 comments · 1 reaction ·
-
Difficulty 4/5 3-5 days Newbie friendliness 48/100
rust-embedded/embedded-hal#745 · 1 reaction ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 55/100
rust-embedded/embedded-hal#744 · 1 comment ·
All issues in rust-embedded/embedded-hal
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
kwakseongjae/auto-hwp#319 ·
-
area:cli bug filter-quality good first issue priority:medium
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
-
Difficulty 1/5 Under an hour Newbie friendliness 72/100
bevyengine/bevy#25861 ·
-
comp-datalake
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
ClickHouse/ClickHouse#121222 ·
-
enhancement remote
Difficulty 2/5 1-3 hours Newbie friendliness 68/100