Skip to content

Add support for async polling #11

Merged
regexident merged 10 commits intoregexident:mainfrom
leighleighleigh:eh0-async-features
Jan 24, 2025
Merged

Add support for async polling #11
regexident merged 10 commits intoregexident:mainfrom
leighleighleigh:eh0-async-features

Conversation

@leighleighleigh
Copy link
Contributor

@leighleighleigh leighleighleigh commented Jan 8, 2025

This PR adds a few useful feature flags, resulting from my own usage with the atsamd HAL, which has a mix of embedded-hal v0.2.7 and v1.0.0 scattered throughout.

Original PR text, relevant to first commit only.

At a high level, the eh1 and eh0 features simply change the trait requirement for the encoder pins - using InputPin, but from different versions of the embedded-hal crate. eh1 uses v1.0.0, eh0 uses v0.2.7.

The async feature adds the Wait trait requirement to the encoder pins, and enables a new poll_async function.
This function select()s the wait_for_any_edge() function on each pin, until an edge is detected, then calls the regular poll() function. I haven't tested this poll_async() on any hardware yet - I shall do so in the next 24 hours.

I've added a few new dependencies, most are fairly self-explanatory. I chose to use embassy-futures instead of the regular futures crate because of it's nice select3 function, and the crate has no further dependencies of it's own, so felt safe.

Thanks for the handy crate!

Updated Description

...updated on epoch 1737691201.

After a bit of testing in the wild, I realized that my approach to both async and eh0/eh1 compatibility had a couple fatal flaws:

  1. It wasn't possible to use blocking AND async encoders within the same program, partially due to how the internal InputPin trait was affected by the feature flags in use.
  2. There's really no need to provide eh0-compatibility 'from inside the library'. Even though it's 'ugly', I think it's better to rely on a user-wrapped Forward<Pin> from the embedded-hal-compat crate instead. I've added rotary_eh0 and linear_eh0 examples that demonstrates this (although there is probably no need to have both).
  3. The way I had sprinkled #[cfg(feature = XYZ)] around resulted in non-additive features, and it was a bit ugly.
  4. It changed the API of poll() to poll_async(), which is an admittedly a minor change, but one that is inconsistent with how async rust is usually done.

To address these shortcomings, I've opted to create an empty PollMode marker trait, akin to the DriverMode trait in projects such as esp-hal's peripheral drivers. This allows for unique implementations of poll() function, depending on this mode, which can be either Blocking or Async. Common logic for each implementation (updating the Decoder) has been moved to a private method update().

Apologies for hacking on this existing PR - it was a bit more of a WIP than I expected.

@leighleighleigh leighleighleigh force-pushed the eh0-async-features branch 2 times, most recently from f3fa8e1 to 860afe4 Compare January 8, 2025 12:38
@regexident
Copy link
Owner

@leighleighleigh thanks! I'll try to give this a review this weekend.

@leighleighleigh
Copy link
Contributor Author

FWIW I just tested this on real hardware, and poll_async() seems to be working well for me.

@leighleighleigh
Copy link
Contributor Author

leighleighleigh commented Jan 9, 2025

The approach using wait_for_any_edge() proved to be unreliable at high speed. I've opted to instead track the last-observed pin state in some internal variables, and then check for rising/falling edges conditionally based on those states. In my testing with a pretty low-quality encoder, I got reliable operation with the FullStep and HalfStep modes. QuadStep seems to skip steps occasionally, which is to be expected.

Copy link
Owner

@regexident regexident left a comment

Choose a reason for hiding this comment

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

Thanks for this great contribution! 🙇

Thanks for the handy crate!

Glad you find it useful! 🥳

Looking at the CLI failures it looks like the next steps to get the PR ready for merging are:

  1. Resolve change requests.

  2. Fix formatting by running cargo fmt.

  3. Remove stray quadrature-encoder/src/main.rs file.

  4. Add a section on async usage to quadrature-encoder/README.md file.

  5. Add entry to CHANGELOG.md file:

    ## [Unreleased]
    
    ### Added
    
    - Added support for async-await (via optional `async` crate feature):
      - Added `async fn poll_async()` method to `IncrementalEncoder<…>`.
      - Added `async fn poll_async()` method to `IndexedIncrementalEncoder<…>`.

@regexident
Copy link
Owner

@leighleighleigh lemme know if I should take it from here or if you want to do it yourself.

@leighleighleigh
Copy link
Contributor Author

I have updated the original description - apologies for the delay, I've been AFK.
I've incorporated most of your feedback, thank you for that, but I am yet to add a section on async usage to the README.md. I'm still happy to do that, as long you are comfortable with how I've handled the new into_async() approach!

I've also got a full embassy-mock example waiting in the wings, pending this embedded-hal-mock PR.

@leighleighleigh leighleighleigh changed the title Added eh1 (default), eh0, and async features. Add support for async polling Jan 24, 2025
@regexident
Copy link
Owner

I have updated the original description - apologies for the delay, I've been AFK.

No worries! I just wanted to make sure you weren't waiting on me for a release with your features.

I've incorporated most of your feedback, thank you for that, but I am yet to add a section on async usage to the README.md. I'm still happy to do that, as long you are comfortable with how I've handled the new into_async() approach!

I like the .into_async()/.into_blocking() approach you took! Very ergonomic.

FYI: I've just rebased the branch onto main (I try to keep a linear history) and pushed a few commits, including a README section.

I'll wait for CI to pass, merge it and release a 0.2.0.

I've also got a full embassy-mock example waiting in the wings, pending this embedded-hal-mock PR.

👨🏻‍🍳👌🏻

That looks like a nice improvement to embedded-hal-mock indeed. Looking forward to it being merged!

@regexident regexident merged commit 6b1d1f6 into regexident:main Jan 24, 2025
7 checks passed
@leighleighleigh
Copy link
Contributor Author

Awesome, thanks!

@regexident
Copy link
Owner

I just published 0.2.0 with your changes. 🚀

Thanks for this great contribution! 🙏

@leighleighleigh leighleighleigh deleted the eh0-async-features branch January 24, 2025 21:06
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