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 embedded-hal-async traits using maybe-async-cfg #31

Merged
merged 9 commits into from
Jun 24, 2024

Conversation

hdoordt
Copy link
Contributor

@hdoordt hdoordt commented Jun 20, 2024

Fixes #30

To do:

  • Add usage example in README
  • Add usage example in rustdoc
  • Add example for microbit v2
  • Fix errors caused by #[doc = include_str!("delay.md")]'
  • [ ] Test async stuff

@@ -102,6 +102,7 @@
//! ```

#![deny(unsafe_code, missing_docs)]
#![allow(async_fn_in_trait)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we actually want this. However, fixing the warning is going to take writing a bunch of boilerplate, I'm afraid

Copy link
Owner

Choose a reason for hiding this comment

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

I agree and would leave it as-is.

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.

Looks good to me so far, thank you!

@eldruin
Copy link
Owner

eldruin commented Jun 24, 2024

Could you also update the MSRV to Rust 1.75.0 and add a note about it in the changelog? Then the CI should stop complaining.

…return Poll::pending instead of nb::WouldBlock in case no data is ready
@hdoordt
Copy link
Contributor Author

hdoordt commented Jun 24, 2024

@eldruin I'm having some trouble getting the tests to run for async. Here's my current attempt: hdoordt@ded90ca

Sadly, the attributes don't seem to work together all that well. Can you maybe have a look?

@eldruin
Copy link
Owner

eldruin commented Jun 24, 2024

The tests themselves in hdoordt@ded90ca work fine for me after adding a single .await in accel_mode_and_odr.rs:297 and running cargo test --features='async' --lib.
However, the doctests and example cannot be compiled due to linux-embedded-hal not supporting async. That is expected and I would not try to do anything about it.
It is very interesting to be able to run async tests but honestly I would not have put the effort into it since the code is so far the same.

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.

The micro:bit example looks great :)

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
@hdoordt
Copy link
Contributor Author

hdoordt commented Jun 24, 2024

The tests themselves in hdoordt@ded90ca work fine for me after adding a single .await in accel_mode_and_odr.rs:297 and running cargo test --features='async' --lib. However, the doctests and example cannot be compiled due to linux-embedded-hal not supporting async. That is expected and I would not try to do anything about it. It is very interesting to be able to run async tests but honestly I would not have put the effort into it since the code is so far the same.

Problem is that now the sync tests don't work anymore. I'll leave the async testing out if it's all the same to you.

However, the test coverage is regressing, which makes CI fail.

@hdoordt hdoordt marked this pull request as ready for review June 24, 2024 12:44
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.

Looks great. Just one trivial thing left: Could you fix the code formatting?

@hdoordt
Copy link
Contributor Author

hdoordt commented Jun 24, 2024

Done and done.

Could you create a release that includes this feature to crates.io? I'd like to use the driver in a training.

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.

Great work, thank you!

@eldruin eldruin merged commit 9a3666e into eldruin:master Jun 24, 2024
21 of 22 checks passed
@eldruin
Copy link
Owner

eldruin commented Jun 24, 2024

I have released this in version 1.1.0.
I have also removed the microbit-example feature, since we are not running the tests with async and async is a requirement for the example. Let me know if that is a problem for you.

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.

Support async
2 participants