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

Add async capability behind feature flag #13

Closed

Conversation

nils-van-zuijlen
Copy link
Contributor

As requested in #11 , I propose this async implementation of the driver behind a feature flag.

The async behaviour is based on traits from embedded-hal-async v1.

I split the code in 3 parts:

  • agnostic/common
  • synchronous/blocking
  • asynchronous/non-blocking

Without the async feature, the sync API is available, the async one is not. The opposite happens when the async feature is set. One downside with this is that the sync and async APIs cannot coexist, but I don't think that's an issue.

Copy link
Owner

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you for this work!
However, the changes are too big for me to meaningfully review them.
Could you split this into smaller PRs?
Maybe something like extract common methods, move stuff into blocking module and then adding async module?

Move sync code that interacts with the I2C to a separate module,
in preparation for adding an async module.
The async parts of the code are placed in the non_blocking module,
which should reflect the sync module exactly, in order to allow for
the documentation to handle both.
@nils-van-zuijlen
Copy link
Contributor Author

Hello,

I split it in two commits, one to move sync code, the other to copy it in async.

Since starting this PR, I learned about the maybe_async crate, and its downsides, so I don't know if this is the right way to go for dual-paradigm code anymore.

I am unsure for how to implement tests for the async part, in particular, I would like to add feature-parity tests, but I found out in https://users.rust-lang.org/t/how-can-i-test-that-two-traits-have-the-same-set-of-function-names/104973 that these can be complicated.

With all this, I don't believe that making other PRs wait for this one is the right move, I can rebase and take their changes in as they come.

@eldruin
Copy link
Owner

eldruin commented Jan 19, 2024

That is a nice article, thanks!
I heard about the maybe-async-cfg crate, which is apparently an improved version of maybe-async. I have not used it, though.
I think the problem with this approach highlighted in the article does not apply here since this crate will realistically only be included once in a dependency tree.
A single project using 2 PCA9685 through different crates with one in blocking and one in async mode seems very unrealistic to me.
So if you are into it, I would give the maybe-async-cfg/maybe-async a try rather than having everything duplicated.

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