Skip to content

[WIP] Implemented transactional interfaces#33

Closed
ryankurte wants to merge 7 commits into
rust-embedded:masterfrom
ryankurte:feature/transactional
Closed

[WIP] Implemented transactional interfaces#33
ryankurte wants to merge 7 commits into
rust-embedded:masterfrom
ryankurte:feature/transactional

Conversation

@ryankurte

@ryankurte ryankurte commented Jan 21, 2020

Copy link
Copy Markdown
Contributor

Implementation of transactional SPI and I2C traits, blocked on merge of rust-embedded/embedded-hal#178

important: remove patch before merging

@ryankurte ryankurte requested a review from a team as a code owner January 21, 2020 20:32
@rust-highfive

Copy link
Copy Markdown

r? @posborne

(rust_highfive has picked a reviewer for you, use r? to override)

Comment thread src/lib.rs
// TODO: is spidev okay with the same array pointer
// being used twice? If not, need some kind of vector
// pool that will outlive the transfer
let w = unsafe {

@ryankurte ryankurte Mar 4, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@posborne is this too cursed? i can't seem to find any documentation as to whether it's okay or not.

Alternatively we might need some kind of memory pool to copy things into / keep the storage around till the transactions are all done (because the transfer object can't hold something owned)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the implementation I think it would work, as the buffers are copied in and out. However, a documented guarantee would be better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @eldruin notes, this should be fine with how the kernel works (performing a copy_from_user). That being said, I do agree that it does feel off, although I can't reason about any way it could go wrong in practice.

I would be OK with landing this here and considering adding an API to spidev which would be designed to explicitly provide safe semantics for this way of interacting with the syscall for future use. That is, something like (not sure about naming):

pub fn read_write_shared(txrx_buf: &'a mut [u8]) -> Self

@posborne

posborne commented Mar 4, 2020

Copy link
Copy Markdown
Member

LGTM; will hold off on final approval, etc. until dep is ready to go. Probably want to at least update docs for the read_write case based on discussions.

@ryankurte

Copy link
Copy Markdown
Contributor Author

Broke the embedded-hal PR in half, once the upstream has landed I will open another PR against https://github.com/ryankurte/linux-embedded-hal/tree/feature/spi-transactions

@ryankurte ryankurte mentioned this pull request Mar 10, 2020
@ryankurte

Copy link
Copy Markdown
Contributor Author

Closing for #35

@ryankurte ryankurte closed this Mar 10, 2020
bors Bot added a commit that referenced this pull request Nov 12, 2020
44: Implement transactional I2C interface and add example r=ryankurte a=eldruin

I implemented the transactional I2C interface from rust-embedded/embedded-hal#223 and added an example with a driver which does a combined Write/Read transaction.
This is based on previous work from #33 by @ryankurte, rust-embedded/rust-i2cdev#51 and is similar to #35.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
bors Bot added a commit that referenced this pull request Nov 14, 2020
44: Implement transactional I2C interface and add example r=ryankurte a=eldruin

I implemented the transactional I2C interface from rust-embedded/embedded-hal#223 and added an example with a driver which does a combined Write/Read transaction.
This is based on previous work from #33 by @ryankurte, rust-embedded/rust-i2cdev#51 and is similar to #35.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
bors Bot added a commit that referenced this pull request Nov 17, 2020
35: Add transactional SPI r=ryankurte a=ryankurte

Implementation of transactional SPI

- replaces #33
- blocked on:
  - ~rust-embedded/embedded-hal#191
  - ~#34

~Important: remove patch and bump hal version before merging~

Co-authored-by: ryan <ryan@kurte.nz>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Review is incomplete T-embedded-linux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants