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.

Sign up for GitHub

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

[RSDK-10188] esp-idf v5 ADC APIs #449

Merged
merged 4 commits into from
Mar 31, 2025
Merged

[RSDK-10188] esp-idf v5 ADC APIs #449

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
80c19ed
wip
mattjperez Mar 25, 2025
6f423dd
no adc errors
mattjperez Mar 25, 2025
55ede33
allow for multiple pins to use ADC1
mattjperez Mar 27, 2025
88f338b
Merge branch 'RSDK-8990-platform-modernization' into rsdk-10188-idf5-adc
mattjperez Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 15 additions & 24 deletions micro-rdk/src/esp32/analog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,33 @@
#![allow(dead_code)]
use crate::common::analog::{AnalogError, AnalogReader, AnalogResolution};
use crate::esp32::esp_idf_svc::hal::adc::{AdcChannelDriver, AdcDriver};
use crate::esp32::esp_idf_svc::hal::{
adc::oneshot::{AdcChannelDriver, AdcDriver},
gpio::ADCPin,
};
use std::borrow::Borrow;

// TODO(RSDK-10188): Update to ESP-IDF 5 ADC API
use crate::esp32::esp_idf_svc::hal::gpio::ADCPin;
use std::sync::{Arc, Mutex};

pub struct Esp32AnalogReader<'a, const A: u32, T: ADCPin> {
channel: AdcChannelDriver<'a, A, T>,
driver: Arc<Mutex<AdcDriver<'a, T::Adc>>>,
pub struct Esp32AnalogReader<'a, T: ADCPin, M: Borrow<AdcDriver<'a, T::Adc>>> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the increase of complexity of types parameters, is there a way we can make it more readable. Like storing only the driver and building the channel on each read? Is building a channel expensive?

Copy link
Member Author

@mattjperez mattjperez Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think that may actually make the type even more complex.

At the very least the new input parameters would either need to be:

  1. (name: String, driver: AdcDriver<'d, T::Adc>, pin: impl Peripheral<P = T> + 'd, config: AdcChannelConfig) if we want to create the AdcChannelDriver on-demand
  2. (name: String, adc: impl Adc, pin: impl Peripheral<P = T> + 'd, config: AdcChannelConfig) if we want to handle both the AdcDriver and AdcChannelDriver initialization in the Esp32AnalogReader.

with the struct holding the proper generics for whichever route we go (storing the pin, adc, adcdriver, etc)

channel: AdcChannelDriver<'a, T, M>,
name: String,
}

impl<'a, const A: u32, T: ADCPin> Esp32AnalogReader<'a, A, T> {
pub fn new(
name: String,
channel: AdcChannelDriver<'a, A, T>,
driver: Arc<Mutex<AdcDriver<'a, T::Adc>>>,
) -> Self {
Self {
name,
channel,
driver,
}
impl<'a, T: ADCPin, M: Borrow<AdcDriver<'a, T::Adc>>> Esp32AnalogReader<'a, T, M> {
pub fn new(name: String, channel: AdcChannelDriver<'a, T, M>) -> Self {
Self { name, channel }
}
fn inner_read(&mut self) -> Result<u16, AnalogError> {
self.driver
.lock()
.unwrap()
.read(&mut self.channel)
self.channel
.read()
.map_err(|e| AnalogError::AnalogReadError(e.code()))
}
fn inner_name(&self) -> String {
self.name.clone()
}
}

impl<const A: u32, T: ADCPin> AnalogReader<u16> for Esp32AnalogReader<'_, A, T> {
impl<'a, T: ADCPin, M: Borrow<AdcDriver<'a, T::Adc>>> AnalogReader<u16>
for Esp32AnalogReader<'a, T, M>
{
type Error = AnalogError;
fn read(&mut self) -> Result<u16, Self::Error> {
self.inner_read()
Expand Down
74 changes: 36 additions & 38 deletions micro-rdk/src/esp32/board.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ use super::analog::Esp32AnalogReader;
// TODO(RSDK-10188): Update to ESP-IDF ADC API
#[cfg(esp32)]
use crate::esp32::esp_idf_svc::hal::adc::{
attenuation::adc_atten_t_ADC_ATTEN_DB_11 as Atten11dB, config::Config, AdcChannelDriver,
AdcDriver, ADC1,
attenuation::DB_11,
oneshot::{
config::{AdcChannelConfig, Calibration},
AdcChannelDriver, AdcDriver,
},
ADC1,
};

use crate::esp32::esp_idf_svc::hal::gpio::InterruptType;
Expand Down Expand Up @@ -84,104 +88,98 @@ impl EspBoard {
let analogs: Result<Vec<AnalogReaderType<u16>>, BoardError> = analogs
.iter()
.map(|v| {
let adc1 = Arc::new(Mutex::new(AdcDriver::new(
unsafe { ADC1::new() },
&Config::new().calibration(true),
)?));
let adc1 = AdcDriver::new(unsafe { ADC1::new() })?;
Copy link
Member

Choose a reason for hiding this comment

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

not sure how this would work, you would be configuring the adc for each analog reader so if you have more than one can we actually instantiate more that one ADC?

Copy link
Member Author

@mattjperez mattjperez Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this was porting the same logic from the previous implementation, however we determined a few things:

  1. the previous implementation was wrong, initializing ADC1 multiple times
  2. the previous implementation still allowed for multiple adc pins to be configured without user-observable 'error',
  3. the new ADC1 does not 'appear' to cause any issues when initialized more than once, however
  4. the new AdcDriver will give a recoverable error if you try to initialize multiple with the same ADC (ex ADC1)
    I've since moved the AdcDriver init to outside of the map and wrapped in an Arc. Confirmed that multiple adc pins come up when configured.

Copy link
Member Author

Choose a reason for hiding this comment

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

as a follow-up, we should probably either:
a. store the board Peripherals in our Esp32Board struct
b. store initialized Arc<Driver>s in our board

let config = AdcChannelConfig {
Copy link
Member

Choose a reason for hiding this comment

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

that's new and cool might be a candidate for config

attenuation: DB_11,
calibration: Calibration::Line,
Copy link
Member

Choose a reason for hiding this comment

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

What does that do and why do we set it to line?

Copy link
Member Author

Choose a reason for hiding this comment

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

previously we did &Config::new().calibration(true). The Calibration enum has three variants: None, Line, Curve according to the source code. However, only None and Line are available to our build. So I used Line to in place of the previous calibration(true).

Copy link
Member

Choose a reason for hiding this comment

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

make sense 👌

..Default::default()
};
let chan: Result<AnalogReaderType<u16>, BoardError> = match v.pin {
32 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio32::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
33 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio33::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
34 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio34::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
35 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio35::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
36 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio36::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
37 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio37::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
38 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio38::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
39 => {
let p: AnalogReaderType<u16> =
Arc::new(Mutex::new(Esp32AnalogReader::new(
v.name.to_string(),
AdcChannelDriver::<Atten11dB, _>::new(unsafe {
AdcChannelDriver::new(adc1, unsafe {
crate::esp32::esp_idf_svc::hal::gpio::Gpio39::new()
})
.map_err(BoardError::EspError)?,
adc1,
}, &config)
.map_err(BoardError::EspError)?
)));
Ok(p)
}
Expand Down
Loading