-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add non-blocking spi::Send trait #121
base: master
Are you sure you want to change the base?
Add non-blocking spi::Send trait #121
Conversation
4b8a646
to
a26a373
Compare
I think the |
If an implementation only provides the |
I have not seen a SPI peripheral which actually cared whether you have anything connected to the SPI (physically or logically). Clocking out and clocking in are supposed to happen at the same so you might as well do a dummy read to make sure you're not deasserting the CS/SS before communication is really done. |
Yeah, but if you don't have an miso line and can thus only implement |
So you're actually planning to introduce a |
HalfDuplex? No, I'm not reading anything, only writing. If we had a separate |
You speak in riddles. Anyway, my point was we don't have separate |
Ok, I'm just going to do a brain dump: There are a couple of common spi configuration as i understand them, these are a subset :
(2) & (3) are both (usually) a subset of (1). Currently the non-blocking traits can only represent the first one. That means drivers can't declare that they're only going to Read or Write. Which means that if you have a driver that only Reads/Writes, you can't easily just use a configuration like (2) or (3). You'd need to inspect the driver code you use and repeat that with every update. In the worst case, you have made a board using (2) or (3) and the driver switches to also using the other data line. Now you can't update the driver. This could happen with LCDs, as they commonly have a MOSI & MISO line, but the MISO line is often unused by drivers. Having a This is also the case for a potential future I think an implementation that only implements If we have a
That's what i want to change with this PR. |
"Officially" (2) and (3) do not exist so they're not a subset but a special case.
Every SPI master on this planet can do real 4-wire SPI, that is exactly how implementations work today. I have not idea why someone would add separate implementations to handle simplex SPI communication when you simply use a regular SPI communication and not (physically and/or logically) connect a GPIO to the peripheral. If those traits are implemented "correctly" they can't be used to compose a FullDuplex trait because e.g. for STM32 this would mean switching into Rx-only/Tx-only mode which does not compose (apart from the fact the Rx-only mode has totally weird semantics with the SPI clock signal being constantly on). Creating these traits would mean that drivers might actually use them which would bring the HALs not implementing them (for whatever reasons) into the trouble to not being able to support the driver.
Don't know, using SISO instead of MISO/MOSI is somewhat special; it can be bitbanged but I don't think all SPI hardware can directly support it.
It is still a misnomer. Flush has a different meaning in computer science. |
In the simplest case, you could just implement (I think) We could also implement the
Yeah, but then you have the issue that you have to verify all the code you depend on for every single update to make sure it doesn't utilize it. There also exists a widely used tx only software implementation ( (Probably) One of the Reasons why serial has a separate
But it's used with the exact same meaning in the
A very common special case, probably used more often than the FullDuplex case in the 'Maker' scene (if you look at the amount of shift registers & lcds). You currently have some driver crates that do their own software tx-only spi implementation (https://github.com/kellerkindt/pcd8544), which can't be what we want, if they could utilize a non-blocking Why would we even have |
How come?
That is a completely different situation. Serial communication is modelled after a regular I/O model because that's how it operates, you can have a receiving channel, you can have a sending channel, both are independent from one another and everything you communicate with are just self-contained Words at a certain speed with a certain signalling. In theory the communication parameters don't even need to be the same although for practical reasons they usually are.
It's also inaccurate there. No reason to repeat mistakes.
So? (BTW: LCDs are quite often FullDuplex, some have built-in memory, many allow configuration read-back)
I don't follow. Whatever the reason may be to not use the SPI implementation and instead to bitbang it, I don't see what it has to do with the lack of a Simplex trait. Maybe it was the lack of SPI peripherals in a particular MCU, maybe it wasn't deemed necessary due to the low resolution, maybe the implementation requires some additional synchronisation of signals to the SPI clock which is not easily possible with our traits... |
They can be, but it's often used in a tx-only way
You do a board design without a MISO pin, so you don't have to route it and you have one more pin to use otherwise, an often rare resource. You (of course) have an already existing peripheral, like an lcd, that can be used without miso. But you (or the hal implementation) has to provide a
(This is probably also because of a lack of a bit-banging lib, but) We want to provide interfaces for various usages that can be implemented in multiple ways (bit-banging, native peripheral, something in between) and not force people to bit-bang, because their needed interface isn't there (a simple nb
Ok, changed it. Ultimately I think it's better to provide a nb equivalent to a concept currently only existing in blocking form (the |
I've also implemented |
3c50894
to
a501daf
Compare
Hmm, if you want to leave the freedom of implementation you can use a pattern similar to the one used with eg. the |
The default implementation would still need participation on part of the hal authors. Not sure, but default is probably more consistent with the rest of the traits and a bit nicer. I'm also not sure if the blocking default write implementation should be changed to be based on the nb Write. It would be the nicer solution and probably force the hal authors a bit, but also a breaking change. |
@david-sawatzke in general you can't have a write-only SPI. Imagine that you have some weird SPI controller that generates an interrupt on every RX FIFO overflow. You still need to handle this RX data somehow or you will have an interrupt each time you send a byte. The most portable way is to handle RX every time you do a TX, even if you have MISO pin floating. Note that HAL is not only about portability across external device libraries, it's also about portability across main device controllers. |
I suspect an XY issue |
If you have a device that needs this, you could empty the rx fifo in the
I want to attach shift-registers, motor-drivers, lcd screens etc. to a micro. They don't need a miso pin, since they only receive data. To be able to use that unconnected miso pin otherwise, my hal would implement an spi, that can only send. This can already be done, needing a FullDuplex implementation that errors out or returns 0 with the read call. This may break when the used device library upgrades (without needing even an minor version bump) so that it uses this function, without warning. It also seems pretty hacky. (This specific use case is also "solved" this way in the mbed lib). This PR would introduce a way to solve this, by introducing an interface for this not particularly uncommon use case. |
And loose all the data if Implementing properly this as async Write will require some state, leading to non-zero overhead. You can still use |
Except the whole blocking thing (which makes any possible implementation cost negligible).
Maybe? You could also loose it just because your device works that way. If you depend on that, the code is device-specific anyway and you don't need this trait. |
a501daf
to
06204c6
Compare
Also add a default implementation for FullDuplex, that only needs an additional read function. Implies some constraints resulting from #120.
I also want to do an spi::Read trait, but the api seems less clear cut, as you have to start an spi transaction and read it at a later time, so two distinct things.