[RSDK-10188] esp-idf v5 ADC APIs #449
Conversation
mattjperez
changed the base branch from
main
to
RSDK-8990-platform-modernization
micro-rdk/src/esp32/board.rs
Outdated
| unsafe { ADC1::new() }, | ||
| &Config::new().calibration(true), | ||
| )?)); | ||
| let adc1 = AdcDriver::new(unsafe { ADC1::new() })?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
this was porting the same logic from the previous implementation, however we determined a few things:
- the previous implementation was wrong, initializing
ADC1multiple times - the previous implementation still allowed for multiple adc pins to be configured without user-observable 'error',
- the new
ADC1does not 'appear' to cause any issues when initialized more than once, however - the new
AdcDriverwill give a recoverable error if you try to initialize multiple with the same ADC (exADC1)
I've since moved theAdcDriverinit to outside of themapand wrapped in anArc. Confirmed that multiple adc pins come up when configured.
There was a problem hiding this comment.
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 adc1 = AdcDriver::new(unsafe { ADC1::new() })?; | ||
| let config = AdcChannelConfig { | ||
| attenuation: DB_11, | ||
| calibration: Calibration::Line, |
There was a problem hiding this comment.
What does that do and why do we set it to line?
There was a problem hiding this comment.
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).
| 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>>> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think that may actually make the type even more complex.
At the very least the new input parameters would either need to be:
(name: String, driver: AdcDriver<'d, T::Adc>, pin: impl Peripheral<P = T> + 'd, config: AdcChannelConfig)if we want to create theAdcChannelDriveron-demand(name: String, adc: impl Adc, pin: impl Peripheral<P = T> + 'd, config: AdcChannelConfig)if we want to handle both theAdcDriverandAdcChannelDriverinitialization in theEsp32AnalogReader.
with the struct holding the proper generics for whichever route we go (storing the pin, adc, adcdriver, etc)
| &Config::new().calibration(true), | ||
| )?)); | ||
| let adc1 = AdcDriver::new(unsafe { ADC1::new() })?; | ||
| let config = AdcChannelConfig { |
There was a problem hiding this comment.
that's new and cool might be a candidate for config
The main differences:
AdcChannelDriveris initialized with anAdcDriver, exampleArc<Mutex<>>from the struct entirely