-
Notifications
You must be signed in to change notification settings - Fork 221
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
I2C transaction_iter should take an iterator of mutable references #367
Comments
If the iterator yields owned Operations, it can generate them on the fly and return them. If it yields references, it has to allocate all the operations upfront and keep them alive throughout the entire However, even with the current "yields owned Operations", the same problem applies to the buffers, they must still be all allocated upfront. Given this I'm not sure Also, the whole problem disappears if we change i2c to use a |
You make a good point about needing a way of yielding generated operations. You also make a good point that I'll look at putting together a PR adding something like a what is seen in #351. |
As can be expected, while I was far too busy to actually work on this other's have worked at a solution. I think something like #392 is the right path forward. |
It would be nice to have a default implementation for some of these trait items. If Also, what is the reasoning behind keeping the transaction methods within the main |
440: I2c: simplify, expand docs, document shared bus usage. r=eldruin a=Dirbaio ~Depends on #441 -- check that one out first.~ This does some simplifications to the trait that I think we should do: - Implement all methods in terms of `transaction`. This way HALs have to implement just that. - Removed byte-wise-iteration methods: `write_iter` and `write_iter_read`. The reason is that they're quite inefficient, especially with DMA implementations. We've already removed these on other traits, so I think we should do as well here. - Removed `transaction_iter`. I don't think it's much useful in practice, because the way iterators work all the yielded `Operation`s must have the same lifetime. This means that, even if the user can generate the `Operation`s on the fly, they can't allocate buffers for these on the fly, all buffers must be pre-allocated. So it's not useful for, say, streaming a large transfer by reusing some small buffer repeatedly. See #367 - Removed useless lifetimes - Standardized buffer names on `read` and `write`, I think they're clearer. It also specifies how i2c bus sharing is supposed to work. This is an alternative to #392 . After the discussions there, I don't think we should split I2C into Bus and Device anymore. For SPI it makes sense, because drivers want to enforce that there's a CS pin (`SpiDevice`) or not (`SpiBus`). This is not the case with I2C, the API is exactly the same in the shared and non-shared case. Drivers shouldn't care which case it is. So all we have to do to "support" bus sharing is docs, This PR does: - Document that it's allowed for implementations to be either shared or not. - Document some guidelines for drivers and HALs on how to best use the traits, expand the examples. Co-authored-by: Dario Nieuwenhuis <[email protected]>
Thank you everybody for the discussion. |
I was recently porting an I2C controller device to
1.0.0-alpha.7
which forced me to implementtransaction
andtransaction_iter
for the first time. I initially tried to implementtransaction
as follows:This, of course, did not work as
&mut [Operation<'a>]
produces an iterator withItem = &mut Operation<'a>
rather than theItem = Operation<'a>
thattransaction_iter
requires.In my opinion,
transaction_iter
's signature should be this instead:This signature mainly allows the implementation of
transaction
that I wrote above. By changing this signature almost all functions ofI2C
can be implemented as calls totransaction_iter
even on systems withoutalloc
support, withwrite_iter
andwrite_iter_read
being the exceptions. This would allow de-duplication of logic, which is rarely a bad thing. Additionally, I see no reason thattransaction_iter
needs an iterator over ownedOperations
in the first place.Thoughts?
The text was updated successfully, but these errors were encountered: