Added two new constructors to ExclusiveDevice to allow for SPI devices that don't require a CS pin #650
Conversation
uliano
force-pushed
the
exclusive_device_no_cs
branch
from
33a7a6e to
c209484
Compare
There was a problem hiding this comment.
Can we be sure there are no use-cases for an exclusive device which still expects a proper chip-select signal? I vaguely remember having seen such things before...
Beyond that, removing the CS from ExclusiveDevice now is a breaking change. I think it's a better option to leave it in and give users the option to opt-out by providing a second constructor:
impl<BUS, D> ExclusiveDevice<BUS, NoPin, D> {
pub fn without_cs(bus: BUS, delay: D) -> Self {
ExclusiveDevice::new(bus, NoPin, delay).unwrap_infallible()
}
}
|
About the use case if there is such a thing as an exclusive device that needs to control its CS for some reason (can't figure out any) it can be done with an additional OutputPin to be contolled as required. But then it should be proved that such a case exists. To me this has been oversighted. To be follwing to the letter the naming found here It should have been named ExclusiveBus from the beginning (but then all the Buses are Exclusive so still redundant) About the breaking change, I normally quite agree with you. Here my reason for that is that the name of this SpiDevice implementation is contradictory and misleading. Is it better to have users correct their code (just remove the pin) or to keep a misnomer in the Interface? Convenience (your proposal) but still one should take the fuss of making its own NoPin or Correctness (my PR) I sincerely can't decide |
|
there's many SPI chips that need CS toggles for each transaction, so they know where a command begins/ends, and they don't work if you tie CS permanently low. |
I see, then @Rahix is the only choice. I see what I can do along that approach |
…wing to have devices without chip select namely new_no_cs and new_no_cs_no_delay
uliano
force-pushed
the
exclusive_device_no_cs
branch
from
c209484 to
3450491
Compare
uliano
changed the title
| impl<BUS, D> ExclusiveDevice<BUS, NoPin, D> { | ||
| /// Create a new [`ExclusiveDevice`] without a Chip Select (CS) pin. | ||
| pub fn new_no_cs(bus: BUS, delay: D) -> Self { | ||
| ExclusiveDevice::new(bus, NoPin, delay).unwrap_infallible() |
There was a problem hiding this comment.
To not pull another dependency, you can also do
pub fn new_no_cs(bus: BUS, delay: D) -> Self {
match ExclusiveDevice::new(bus, NoPin, delay) {
Ok(this) => this,
// no Err() branch
}
}Less readable, but avoids dependency-bloat... Not sure what the official stance of e-h on that topic is...
There was a problem hiding this comment.
yeah avoiding deps is always nice.
| // Implement `OutputPin` | ||
| impl OutputPin for NoPin { | ||
| fn set_low(&mut self) -> Result<(), Infallible> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn set_high(&mut self) -> Result<(), Infallible> { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess also implement all the other digital traits for this type?
There was a problem hiding this comment.
@Rahix what's the use case for a non existent InputPin? what is the value is supposed to return?
Summary
NoPin(inembedded-hal::digital) as a dummy pin for hardware-managed CS scenarios.ExclusiveDevice, namelynew_no_csandnew_no_cs_no_delay(could't figure out any better) to allow for SPI devices that don't require a CS pin.Why?
The existing implementation required an
OutputPin, which for some chips can be unnecessary and effectively wasted.Edited
I refactoerd this PR along the lines hinted by @Rahix below.