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 oneshot driver #302

Merged
merged 10 commits into from
Sep 18, 2023
Merged

Adc oneshot driver #302

merged 10 commits into from
Sep 18, 2023

Conversation

liebman
Copy link
Contributor

@liebman liebman commented Sep 10, 2023

Implementation of the esp-idf v5 adc oneshot driver.

It would be nice to somehow replicate two C #define's info cfg's from esp-idf-sys so that we don't have to replicate logic determining if curve fitting and/or line fitting are suported based on version and cpu type:

  • ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED
  • ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED

examples/adc_oneshot.rs Outdated Show resolved Hide resolved
src/adc.rs Outdated Show resolved Hide resolved
src/adc.rs Outdated Show resolved Hide resolved
src/adc.rs Show resolved Hide resolved
src/adc.rs Outdated Show resolved Hide resolved
src/adc.rs Show resolved Hide resolved
src/adc.rs Show resolved Hide resolved
src/adc.rs Outdated Show resolved Hide resolved
src/adc.rs Outdated Show resolved Hide resolved
AdcChannelDriver: remove Sync implementation
examples: update adc_oneshot
remove AdcChannelDriver::channel
@liebman
Copy link
Contributor Author

liebman commented Sep 16, 2023

@ivmarkov I'm trying to use the updated OneShot driver in a battery voltage monitor but can't seem to be able to store the AdcChannelDriver in a struct. I'm getting this error

28 | impl<'d, T, M> Battery<'d, T, M> 
   |             - this type parameter
...
49 |             channel,
   |             ^^^^^^^ expected `AdcChannelDriver<'_, T, M>`, found `AdcChannelDriver<'_, T, &...>`
   |
   = note: expected struct `esp_idf_hal::adc::oneshot::AdcChannelDriver<'d, _, M>`
              found struct `esp_idf_hal::adc::oneshot::AdcChannelDriver<'_, _, &esp_idf_hal::adc::oneshot::AdcDriver<'_, <T as ADCPin>::Adc>>`

with this code

pub struct Battery<'d, T, M> 
where
    T: ADCPin,
    M: Borrow<AdcDriver<'d, T::Adc>>,
{
    adc: AdcDriver<'d, T::Adc>,
    channel: AdcChannelDriver<'d, T, M>,
    voltage: Option<f32>,
}

impl<'d, T, M> Battery<'d, T, M> 
where
    T: ADCPin,
    M: Borrow<AdcDriver<'d, T::Adc>>,
{
    pub fn new(
        adc: impl Peripheral<P = T::Adc> + 'd,
        pin: impl Peripheral<P = T> + 'd,
    ) -> Result<Self, EspError> {
        let adc = AdcDriver::new(adc)?;
        let channel = AdcChannelDriver::new(
            &adc,
            pin,
            AdcChannelConfig {
                attenuation: attenuation::DB_11,
                calibration: true,
                ..Default::default()
            },
        )?;
        Ok(Self {
            adc,
            channel,
            voltage: None,
        })
    }

    pub fn get_voltage(&mut self) -> Result<f32, EspError> {
        const SAMPLES: usize = 128;
        let mut max = 0u32;
        let mut min = 0xffffffffu32;
        let mut total = 0u32;
        for _ in 0..SAMPLES {
            let value = self.adc.read(&mut self.channel)? as u32;
            if value > max {
                max = value;
            }
            if value < min {
                min = value;
            }
            total += value;
            std::thread::sleep(std::time::Duration::from_millis(10));
        }
        // remove the min and max values
        let total = total - min - max;
        let value = total / (SAMPLES as u32 - 2);
        log::info!("Battery ADC value={value} min={min} max={max}");
        Ok((value*2) as f32 / 1000.0)
    }
}

Any ideas?

@ivmarkov
Copy link
Collaborator

@ivmarkov I'm trying to use the updated OneShot driver in a battery voltage monitor but can't seem to be able to store the AdcChannelDriver in a struct. I'm getting this error

28 | impl<'d, T, M> Battery<'d, T, M> 
   |             - this type parameter
...
49 |             channel,
   |             ^^^^^^^ expected `AdcChannelDriver<'_, T, M>`, found `AdcChannelDriver<'_, T, &...>`
   |
   = note: expected struct `esp_idf_hal::adc::oneshot::AdcChannelDriver<'d, _, M>`
              found struct `esp_idf_hal::adc::oneshot::AdcChannelDriver<'_, _, &esp_idf_hal::adc::oneshot::AdcDriver<'_, <T as ADCPin>::Adc>>`

with this code

pub struct Battery<'d, T, M> 
where
    T: ADCPin,
    M: Borrow<AdcDriver<'d, T::Adc>>,
{
    adc: AdcDriver<'d, T::Adc>,
    channel: AdcChannelDriver<'d, T, M>,
    voltage: Option<f32>,
}

impl<'d, T, M> Battery<'d, T, M> 
where
    T: ADCPin,
    M: Borrow<AdcDriver<'d, T::Adc>>,
{
    pub fn new(
        adc: impl Peripheral<P = T::Adc> + 'd,
        pin: impl Peripheral<P = T> + 'd,
    ) -> Result<Self, EspError> {
        let adc = AdcDriver::new(adc)?;
        let channel = AdcChannelDriver::new(
            &adc,
            pin,
            AdcChannelConfig {
                attenuation: attenuation::DB_11,
                calibration: true,
                ..Default::default()
            },
        )?;
        Ok(Self {
            adc,
            channel,
            voltage: None,
        })
    }

    pub fn get_voltage(&mut self) -> Result<f32, EspError> {
        const SAMPLES: usize = 128;
        let mut max = 0u32;
        let mut min = 0xffffffffu32;
        let mut total = 0u32;
        for _ in 0..SAMPLES {
            let value = self.adc.read(&mut self.channel)? as u32;
            if value > max {
                max = value;
            }
            if value < min {
                min = value;
            }
            total += value;
            std::thread::sleep(std::time::Duration::from_millis(10));
        }
        // remove the min and max values
        let total = total - min - max;
        let value = total / (SAMPLES as u32 - 2);
        log::info!("Battery ADC value={value} min={min} max={max}");
        Ok((value*2) as f32 / 1000.0)
    }
}

Any ideas?

Yes. TL; DR: You only need to keep around the AdcChannelDriver instance in the struct, as it anyways has a reference (or owned instance) of AdcDriver so you can always pull it from there.

Long story: basic Rust - you are trying to keep - in a single struct - an instance of a struct (AdcDriver) and then somehow another member - AdcChannelDriver - that potentially has a pointer to the member of the struct I just mentioned - AdcDriver. This is just not possible in Rust (self-referential structs) with safe code and without Pin. Think what will happen if you MOVE the struct - the address of the AdcDriver would change, but the AdcChannelDriver member won't know! - it would still point to the old location of the AdcDriver instance which was having a different address when the struct was not moved!

That's why Rust these days has Pin (though it still requires some unsafe). But then again - and as per the TL;DR - you don't need the AdcDriver instance around in the first place.

@ivmarkov ivmarkov merged commit 95dfc2f into esp-rs:master Sep 18, 2023
8 checks passed
@liebman liebman deleted the adc-oneshot-driver branch September 20, 2023 18:26
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.

2 participants