403: Make the async SpiDevice trait unsafe to implement r=Dirbaio a=GrantM11235
This is necessary due to the raw pointer workaround.
Co-authored-by: Grant Miller <GrantM11235@gmail.com>
398: Extract SPI exclusive device into shared crate r=Dirbaio a=eldruin
Some concerns have been raised regarding the error handling in `spi::blocking::ExclusiveDevice`.
I am not sure what the problem was, though.
I think `@GrantM11235` was the one to raise the concerns so maybe he can elaborate and then I can improve this description.
Since that struct is just a helpful wrapper for a very specific situation, I propose we move it out of `embedded-hal` itself. For example into an `embedded-hal-shared` crate (or some other name, feel free to bikeshed).
We should note that we (probably) have the same problem in `embedded-hal-async` where there is also an `ExclusiveDevice` for SPI, so if we create such a crate, we should agree on what to put in there:
1. Only blocking `ExclusiveDevice`
2. blocking + async `ExclusiveDevice`
3. any of the above + `RefCell`-based sharing
4. any of the above + some mutex / critical section
We should also note that the great `shared-bus` crate also solves some of these problems so we might also put (some?) of this there.
cc: `@Rahix`
Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
399: SpiDevice: add guidelines for behaviour of CS pin upon bus failures r=eldruin a=newAM
Currently the `SpiDevice` trait does not define the sequence of actions that occur after a bus error.
I discussed this with `@Dirbaio` in the matrix chat and the conclusion we reached was that this should be added to the documentation as a guideline to match what already exists for `ExclusiveDevice`. The rationale for a guideline instead of a requirement is that SPI error recovery is implementation specific.
Co-authored-by: Alex Martens <alex@thinglab.org>
393: Remove IoPin r=eldruin a=Dirbaio
As discussed in #340 it's unclear if the current IoPin trait is the best way to go (tldr: due to the fact it changes type it's hard to own it in driver structs).
I propose temporarily removing it, to unblock the 1.0 release. When discussion settles on a final design, we can add it back.
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
396: spi: clarify traits are for master mode. r=eldruin a=Dirbaio
All dirvers out there assume the traitsare for master mode. However, there are some HALs
out there that impl the traits for SPI in slave mode, which will break when used with drivers.
(stm32f1xx-hal, stm32f4xx-hal, stm32l4xx-hal, probably more).
If we add SPI slave traits in the future, they should be a separate set of traits because the
API would look quite different:
- you'd want the API to give extra extra info about the transaction lengths: if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high),
you'd want `transfer` to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.
- The bus/device split doesn't make sense in slave mode.
Also IMO the `spi, spi_slave` naming is fine even if inconsistent, since the vast majority
of uses are master mode. I wouldn't name the `spi` module `spi_master`.
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
All dirvers out there assume the traitsare for master mode. However, there are some HALs
out there that impl the traits for SPI in slave mode, which will break when used with drivers.
(stm32f1xx-hal, stm32f4xx-hal, stm32l4xx-hal, probably more).
If we add SPI slave traits in the future, they should be a separate set of traits because the
API would look quite different:
- you'd want the API to give extra extra info about the transaction lengths: if you start a transfer with 256-byte buffer but the master only sends 10 bytes (sets CS low, sends 10 bytes, sets CS high),
you'd want `transfer` to return, saying "I got only 10 bytes", instead of hanging waiting for the other 246 bytes.
- The bus/device split doesn't make sense in slave mode.
Also IMO the `spi, spi_slave` naming is fine even if inconsistent, since the vast majority
of uses are master mode. I wouldn't name the `spi` module `spi_master`.
391: Change missing_docs from deny to warn. r=eldruin a=Dirbaio
It is annoying to get hard errors while developing, it prevents testing things out
without documenting them because it fails the build.
This changes it to warn, which has the same effect in CI (because it already has `RUSTFLAGS: '--deny warnings'`),
but still allows buildilng.
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
It is annoying to get hard errors while developing, it prevents testing things out
without documenting them because it fails the build.
This changes it to warn, which has the same effect in CI (because it already has `RUSTFLAGS: '--deny warnings'`),
but still allows buildilng.
384: Make `can::Id` usable as key in `BTreeMap` and `HashMap` r=eldruin a=usbalbin
Implement `Ord` and `Hash` for `can::Id` to make it usable as key in `BTreeMap` and `HashMap`
Co-authored-by: Albin Hedman <albin.hedman@dectron.se>
Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
383: async: remove useless `+ 'a` in TAITs. r=ryankurte a=Dirbaio
These aren't needed, they're already implied by the type having `'a` as a generic param.
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
382: async/spi: make helpers default methods instead of an extension trait. r=ryankurte a=Dirbaio
It turns out you *can* have default methods on a GAT-based async trait. All
you have to do is move the TAITs out of the trait so Rust infers them
instead of the impl specifying them.
Impls can't override the methods, but they already couldn't before with
the extension trait.
This makes async SpiDevice more consistent with blocking, and makes the helpers
more discoverable.
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
It turns out you *can* have default methods on a GAT-based async trait. All
you have to do is move the TAITs out of the trait so Rust infers them
instead of the impl specifying them.
Impls can't override the methods, but they already couldn't before with
the extension trait.
This makes async SpiDevice more consistent with blocking, and makes the helpers
more discoverable.
380: async/spi: replace the "give back the Bus" hack with a raw pointer. r=ryankurte a=Dirbaio
The async SPI trait contains a "hack" to workaround limitations in Rust's borrow checker: #347
It turns out the hack doesn't allow many shared bus implementations, for example when using an async Mutex: The `transaction` method gets `&'a mut self`, and the closure wants `&'a mut Self::Bus`. This only works if the Bus is a direct field in `self`. In the mutex case, the Bus has to come from inside the `MutexGuard`, so it'll live for less than `'a`.
See https://gist.github.com/kalkyl/ad3075182d610e7b413b8bbe1228ab90 which fails with the following error:
```
error[E0597]: `bus` does not live long enough
--> src/shared_[spi.rs:78](http://spi.rs:78/):34
|
63 | fn transaction<'a, R, F, Fut>(&'a mut self, f: F) -> Self::TransactionFuture<'a, R, F, Fut>
| -- lifetime `'a` defined here
...
78 | let (bus, f_res) = f(&mut bus).await;
| --^^^^^^^^-
| | |
| | borrowed value does not live long enough
| argument requires that `bus` is borrowed for `'a`
...
89 | }
| - `bus` dropped here while still borrowed
```
This is an alternative hack. If lifetimes don't work, simply don't use lifetimes at all!
- Downside: it needs `unsafe{}` in all callers of transaction (but these should be rare, many users will use the `SpiDeviceExt` helpers.
- Upside: it's now possible to write sound shared bus impls.
- Upside: it no longer requires the "give back the bus" hack, it's now possible again to use `?` inside the closure for error handling.
cc `@kalkyl`
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
