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

ADC: Add async support for oneshot reads for esp32c3 and esp32c6 #2925

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

davoclavo
Copy link
Contributor

@davoclavo davoclavo commented Jan 9, 2025

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Add support for ADC async oneshot reads for esp32c3 and esp32c6. Fixes #1643

Testing

Added QA example that uses embassy executor and reads every second from GPIO5. I am able to measure distinct values just by handling the devkit board with my hands.

Extra notes

  1. It seems the double oneshot read hack for esp32c6 is not required if interrupts are used. If someone is able to help me figure out a way to replicate the issue I could try to see if we need to add that hack back.

  2. Not sure if this is critical, but I am unsure if its possible to perform concurrent reads to multiple channels using the same ADC interface, in order to address the potential issue that is currently solved by using the active_channel attribute for the blocking implementation, in order to implement some kind of synchronization mechanism for the async version.

  3. Should I add the Dm: crate::DriverMode generic and only provide the Blocking implementation for the xtensa version of the Adc driver (and the esp32 Driver too) or leave it as is without the extra parameter? Done

@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch 11 times, most recently from c07f1db to 23ef634 Compare January 14, 2025 17:35
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

So sorry for the delay in review here, we cut a release in between this PR meaning most of resources were dedicated to that, I hope you understand :).

I have a few initial comments, mostly because I'm not that familiar with ADC in general.

esp-hal/src/analog/adc/riscv.rs Outdated Show resolved Hide resolved
esp-hal/src/analog/adc/riscv.rs Outdated Show resolved Hide resolved
esp-hal/CHANGELOG.md Outdated Show resolved Hide resolved
@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch from 23ef634 to a71f402 Compare January 22, 2025 00:56
@davoclavo
Copy link
Contributor Author

Thanks for the feedback, and totally understandable that things were busy during the release :)

Unrelated question: I have been trying to tackle pending issues marked with "help wanted" in gh to learn and contribute, but is there any other higher priority work that could be tackled via contributions or is that the right approach?

@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch 2 times, most recently from 0f66a79 to f5319f2 Compare January 28, 2025 18:10
@MabezDev
Copy link
Member

MabezDev commented Jan 28, 2025

Unrelated question: I have been trying to tackle pending issues marked with "help wanted" in gh to learn and contribute, but is there any other higher priority work that could be tackled via contributions or is that the right approach?

Thanks for the interest! We're definitely keen on any helping hands, and your contributions so far have been stellar! We haven't really been using the help wanted label that much, but I can try and go through our backlog in the next few days and label some good issues for community contributions.

In general though, we're happy to take any contributions - this is an early stage project, and the teams current focus is a stable release. If there is anything that interests you in the issue tracker, then please feel free to work on it, you're welcome to ask to be assigned to certain issues too. We're more than happy to help and review PRs (in a more timely fashion from now on, I promise :D).

@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch 3 times, most recently from 3e98cc5 to ec02c10 Compare January 29, 2025 03:58
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this!

I have a small nitpick about that todo comment, if you don't mind addressing that

esp-hal/src/analog/adc/riscv.rs Outdated Show resolved Hide resolved
@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch from ec02c10 to 95a6f6c Compare January 30, 2025 20:29
esp-hal/MIGRATING-0.23.md Outdated Show resolved Hide resolved
- fix migrating document
- add ADC2 reading qa example and fix sensor reading
@davoclavo davoclavo force-pushed the feat/async_adc_read_oneshot branch from 95a6f6c to f7be59f Compare January 30, 2025 23:18
Copy link
Contributor

@bugadani bugadani 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!

@bugadani bugadani added this pull request to the merge queue Jan 31, 2025
Merged via the queue into esp-rs:main with commit 9a28bdf Jan 31, 2025
28 checks passed
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.

async ADC read
3 participants