Skip to content
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

Implement async trait for i2c::RefCellDevice #600

Closed

Conversation

sjoerdsimons
Copy link

No description provided.

@sjoerdsimons sjoerdsimons requested a review from a team as a code owner May 8, 2024 18:58
@Dirbaio
Copy link
Member

Dirbaio commented May 8, 2024

this will panic if you actually try to do two transfers from two async tasks in parallel. First task will borrow the RefCell, second task will panic because it's already borrowed. I'm not sure if we should do this because it'd make it too easy to get panics. Ideally you'd use a "real" async mutex, for example from embassy-sync/embassy-embedded-hal or rtic-sync.

@sjoerdsimons
Copy link
Author

Right i didn't consider that as my mental model for anything RefCell is, you're responsible for keeping the reference invariants otherwise panic (e.g. the same would happen if you borrow the bus from the refcell before any i2c transfer) ;).

I'm using this to simply poll three ina230's one by one in an async task1. Which ofcourse works fine. However indeed this will break quickly if you try to use it in a select! like construct. Which again may simply be something that comes with the RefCell territory.

That said revisiting things I mostly ended up doing the above due to 1:1 porting from sync to async code; somehow missing the embassy-embedded-hal shared_bus module :) Which indeed is a nicer solution, so i guess we can close this PR and hopefully these notes will be useful for some in the future

Footnotes

  1. https://github.com/sjoerdsimons/ucpd-tools/blob/2c3116ab70a245a33a9eb1a0cccf9173c484d334/stm32g071b-disco-firmware/src/main.rs#L103

sjoerdsimons added a commit to sjoerdsimons/ucpd-tools that referenced this pull request May 9, 2024
Move from embedded-hal-bus to embassy-embedded-hal for the purposes of
implementing an async shared i2c bus. This also drops the need for a
patched embedded-hal-bus, which is unlikely to go upstream[^0]

[^0]: rust-embedded/embedded-hal#600
Signed-off-by: Sjoerd Simons <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants