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

Refactor the ADC to use the new API. #2698

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Refactor the ADC to use the new API. #2698

merged 5 commits into from
Feb 4, 2025

Conversation

floitsch
Copy link
Member

No description provided.

@floitsch floitsch requested a review from kasperl January 29, 2025 13:32
@cla-bot cla-bot bot added the cla-signed The contributors have signed the CLA label Jan 29, 2025
Copy link
Member

@kasperl kasperl 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.

lib/gpio/adc.toit Outdated Show resolved Hide resolved
$allow-restricted flag to allow its use.
*/
constructor .pin --max-voltage/float?=null --allow-restricted/bool=false:
constructor .pin --max-voltage/float?=null --allow-restricted/bool?=false:
Copy link
Member

Choose a reason for hiding this comment

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

Why is allow-restricted suddenly allowed to be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

src/resources/adc_esp32.cc Outdated Show resolved Hide resolved
#ifndef CONFIG_IDF_TARGET_ESP32C6
esp_adc_cal_characteristics_t calibration;
#endif
adc_oneshot_unit_handle_t* unit;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make these private?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -15,7 +15,7 @@

#include "../top.h"

#ifdef TOIT_ESP32
#if 0 // def TOIT_ESP32
Copy link
Member

Choose a reason for hiding this comment

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

Intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.
The I2S uses the ADC underneath. It's not allowed to use the old and new version of the ADC driver. Until the I2S code is updated it won't compile.

There is already a follow-up PR that fixes the I2S. (Otherwise, I would have added more comments).

@floitsch floitsch merged commit 1941468 into master Feb 4, 2025
27 of 29 checks passed
@floitsch floitsch deleted the floitsch/adc branch February 4, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The contributors have signed the CLA
Development

Successfully merging this pull request may close these issues.

2 participants