Skip to content

Conversation

@jessebraham
Copy link
Member

Quickly checked the other implementations as well, but I think they're all okay.

Closes #769

@jessebraham jessebraham requested a review from bjoernQ August 31, 2023 16:15
// depends on which chip is being used
cfg_if::cfg_if! {
if #[cfg(esp32c6)] {
const NUM_ATTENS: usize = 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea, what do you think about using constant in 'esp-hal-common/src/soc/esp32XX/mod.rs' instead of this config block?

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 1, 2023

This fixes the original problem and should be good enough for now.
I like Juraj's idea - I guess that would be nicer.

The ideal solution IMHO would be to let the impl_adc_interface macro define the it. (And maybe move the chip specific instantiation of that macro to soc)

As said - this fixes the problem and the other suggestions might be something for a refactoring probably.

However: I cannot get ADC to work on C6 at all. It's always reads 0. Just wanted to confirm that ... maybe a me problem?

I'll approve this since it fixes the original problem. It's up to you if you want to explore the other approaches.

Would be good if someone else could verify if ADC works at all for you on C6 - if not we should create an issue and see what broke it

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - see my previous comment

@jessebraham
Copy link
Member Author

jessebraham commented Sep 1, 2023

The ideal solution IMHO would be to let the impl_adc_interface macro define the it. (And maybe move the chip specific instantiation of that macro to soc)

This would require adding additional macros to define the configuration and driver types, not sure it's really worth it for a constant used in 3 places in a single file, IMO.

However: I cannot get ADC to work on C6 at all. It's always reads 0. Just wanted to confirm that ... maybe a me problem?

Was not the case for me. Will do some more testing, though.

@jessebraham jessebraham merged commit ce3933c into esp-rs:main Sep 5, 2023
@jessebraham jessebraham deleted the fixes/adc branch September 5, 2023 13:32
SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Sep 22, 2023
* Fix number of ADC attenuations for ESP32-C6

* Update CHANGELOG
playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Sep 26, 2023
* Fix number of ADC attenuations for ESP32-C6

* Update CHANGELOG
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.

ESP32-C6 unable to use ADC on GPIO5

3 participants