Passing multiple ADCs as arguments to generic function using embedded-hal.

[noonien]

trafficstars

I've got a generic function that requires access to multiple ADC pins, however, I can't think of a way to pass these as arguments.

Here's the code I have so far:

#![no_std]
#![no_main]
#![feature(type_alias_impl_trait)]

use esp32c3_hal::{
    adc::{AdcConfig, Attenuation, ADC},
    clock::ClockControl,
    peripherals::Peripherals,
    prelude::*,
    timer::TimerGroup,
    Rtc, IO,
};

use esp_backtrace as _;
use esp_println::println;

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let mut system = peripherals.SYSTEM.split();
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    // Disable the RTC and TIMG watchdog timers
    let mut rtc = Rtc::new(peripherals.RTC_CNTL);
    let timer_group0 = TimerGroup::new(peripherals.TIMG0, &clocks);
    let mut wdt0 = timer_group0.wdt;
    let timer_group1 = TimerGroup::new(peripherals.TIMG1, &clocks);
    let mut wdt1 = timer_group1.wdt;

    rtc.rwdt.disable();
    wdt0.disable();
    wdt1.disable();

    let io = IO::new(peripherals.GPIO, peripherals.IO_MUX);

    let throttle_pin = io.pins.gpio0;
    let regen_pin = io.pins.gpio1;

    let analog = peripherals.APB_SARADC.split();
    let mut adc_config = AdcConfig::new();
    let throttle = adc_config.enable_pin(throttle_pin.into_analog(), Attenuation::Attenuation11dB);
    let regen = adc_config.enable_pin(regen_pin.into_analog(), Attenuation::Attenuation11dB);

    let adc = ADC::adc(
        &mut system.peripheral_clock_control,
        analog.adc1,
        adc_config,
    )
    .unwrap();

    do_io(adc, throttle, adc, regen);

    loop {}
}

use embedded_hal::adc;
use nb;

pub fn do_io<TADC, TADCI, TPIN, RADC, RADCI, RPIN>(
    mut tadc: TADC,
    mut throttle: TPIN,
    mut radc: RADC,
    mut regen: RPIN,
) where
    TADC: adc::OneShot<TADCI, u16, TPIN, Error = ()>,
    RADC: adc::OneShot<RADCI, u16, RPIN, Error = ()>,
    TPIN: adc::Channel<TADCI>,
    RPIN: adc::Channel<RADCI>,
{
    let throttle_val: u16 = nb::block!(tadc.read(&mut throttle)).unwrap();
    let regen_val: u16 = nb::block!(radc.read(&mut regen)).unwrap();

    println!("throttle = {}, regen = {}", throttle_val, regen_val);
}

I've tried .clone() on adc and on analog.adc1, none work because their respective types do not implement Clone.

Is there a way to do this?

[noonien]

This is what I managed to make work so far:

use embedded_hal::adc::{self as eadc, OneShot};
use esp32c3_hal::adc;

use esp_println::println;
use nb;

pub fn do_io<ADCI, TPIN, RPIN>(
    mut adc: adc::ADC<'_, ADCI>,
    mut throttle: adc::AdcPin<TPIN, ADCI>,
    mut regen: adc::AdcPin<RPIN, ADCI>,
) where
    ADCI: adc::RegisterAccess,
    TPIN: eadc::Channel<ADCI, ID = u8>,
    RPIN: eadc::Channel<ADCI, ID = u8>,
{
    let throttle_val: u16 = nb::block!(adc.read(&mut throttle)).unwrap();
    let regen_val: u16 = nb::block!(adc.read(&mut regen)).unwrap();

    println!("throttle = {}, regen = {}", throttle_val, regen_val);
}

However, there are 2 problems with this:

1. It has to use both embedded_hal::adc and esp323_hal::adc. I would prefer to just use embedded_hal.
2. This only works if both channels are on the same ADC.

[bjoernQ]

I think the traits in embedded HAL are designed in a way which makes things like these very inconvenient.

That's also the reason why ADC is removed for now from embedded-hal 1.x ( see https://github.com/rust-embedded/embedded-hal/pull/376 )

[noonien]

I see. I don't mind not using embedded-hal::adc until a better implementation is suggested.

However, the issue still remains that an esp-hal ADC cannot be cloned/copied. So passing multiple ADC channels that can be part of different ADCs to a generic functions is impossible (as far as I can tell).

[bjoernQ]

I see. I don't mind not using embedded-hal::adc until a better implementation is suggested.

However, the issue still remains that an esp-hal ADC cannot be cloned/copied. So passing multiple ADC channels that can be part of different ADCs to a generic functions is impossible (as far as I can tell).

It's on purpose that those drivers are not clone or copy. You can pass a mutable reference instead like this

#![no_std]
#![no_main]

use esp_backtrace as _;
use esp_println::println;
use hal::{
    adc::{AdcConfig, Attenuation, ADC},
    clock::ClockControl,
    peripherals::Peripherals,
    prelude::*,
    timer::TimerGroup,
    Rtc, IO,
};
#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let mut system = peripherals.SYSTEM.split();
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    // Disable the RTC and TIMG watchdog timers
    let mut rtc = Rtc::new(peripherals.RTC_CNTL);
    let timer_group0 = TimerGroup::new(peripherals.TIMG0, &clocks);
    let mut wdt0 = timer_group0.wdt;
    let timer_group1 = TimerGroup::new(peripherals.TIMG1, &clocks);
    let mut wdt1 = timer_group1.wdt;

    rtc.swd.disable();
    rtc.rwdt.disable();
    wdt0.disable();
    wdt1.disable();

    println!("Hello world!");

    let io = IO::new(peripherals.GPIO, peripherals.IO_MUX);

    let throttle_pin = io.pins.gpio0;
    let regen_pin = io.pins.gpio1;

    let analog = peripherals.APB_SARADC.split();
    let mut adc_config = AdcConfig::new();
    let mut throttle =
        adc_config.enable_pin(throttle_pin.into_analog(), Attenuation::Attenuation11dB);
    let mut regen = adc_config.enable_pin(regen_pin.into_analog(), Attenuation::Attenuation11dB);

    let mut adc = ADC::adc(
        &mut system.peripheral_clock_control,
        analog.adc1,
        adc_config,
    )
    .unwrap();

    loop {
        do_io(&mut adc, &mut throttle, &mut regen);
    }
}

use embedded_hal::adc::{self as eadc, OneShot};
use hal::adc;
use nb;

pub fn do_io<ADCI, TPIN, RPIN>(
    adc: &mut adc::ADC<'_, ADCI>,
    throttle: &mut adc::AdcPin<TPIN, ADCI>,
    regen: &mut adc::AdcPin<RPIN, ADCI>,
) where
    ADCI: adc::RegisterAccess,
    TPIN: eadc::Channel<ADCI, ID = u8>,
    RPIN: eadc::Channel<ADCI, ID = u8>,
{
    let throttle_val: u16 = nb::block!(adc.read(throttle)).unwrap();
    let regen_val: u16 = nb::block!(adc.read(regen)).unwrap();

    println!("throttle = {}, regen = {}", throttle_val, regen_val);
}

[noonien]

Hmm, I don't see how that would solve the problem.

If a function needs to be generic over analog inputs, it needs to accept both an ADC and an ADC Channel for each input.

The example function you've provided cannot be called with channels from different ADCs, and adapting the function to accept an ADC for each ADC channel would look like this:

pub fn do_io<ADCI, TPIN, RPIN>(
    tadc: &mut adc::ADC<'_, ADCI>,
    throttle: &mut adc::AdcPin<TPIN, ADCI>,
    radc: &mut adc::ADC<'_, ADCI>,
    regen: &mut adc::AdcPin<RPIN, ADCI>,
) where
    ADCI: adc::RegisterAccess,
    TPIN: eadc::Channel<ADCI, ID = u8>,
    RPIN: eadc::Channel<ADCI, ID = u8>,
{
    let throttle_val: u16 = nb::block!(tadc.read(throttle)).unwrap();
    let regen_val: u16 = nb::block!(radc.read(regen)).unwrap();

    println!("throttle = {}, regen = {}", throttle_val, regen_val);
}

But now it cannot be called with two analog inputs that use the same ADC, because you cannot pass &mut adc twice.

[bjoernQ]

My comment was about the fact that when you move the ADC and PINs into a function and don't return them afterwards, they will get discarded. Like in your original code (not using references) you cannot use that function in the loop - or even just two times

[noonien]

Yes, I don't need to use that function in a loop or multiple times, the example is a bit artificial, in reality do_io is a separate task that never returns, that runs concurrently with other tasks, so moving the analog inputs into it is what I want to do anyway.

The issue was with passing multiple analog inputs to generic functions.

[bjoernQ]

Ah sorry! Now I understand the context better

[hansingt]

I had the same problem in my code. I think, the problem really is because of the traits in embedded-hal.

I was able to work around it using a RefCell to allow passing an immutable reference to a mutable object and borrowing it when required.

If the code needs to be thread safe, you'd have to wrap it into a critical_section::Mutex as well.

So your do_it function can look like this:

pub fn do_io<ADCI, TPIN, RPIN>(
    tadc: &Mutex<RefCell<adc::ADC<'_, ADCI>>>,
    throttle: &mut adc::AdcPin<TPIN, ADCI>,
    radc: &Mutex<RefCell<adc::ADC<'_, ADCI>>>,
    regen: &mut adc::AdcPin<RPIN, ADCI>,
) where
    ADCI: adc::RegisterAccess,
    TPIN: eadc::Channel<ADCI, ID = u8>,
    RPIN: eadc::Channel<ADCI, ID = u8>,
{
    let throttle_val: u16 = critical_section::with(|cs| {
       nb::block!(tadc.borrow(cs).borrow_mut().read(throttle)).unwrap())
    };
    let regen_val: u16 = critical_section::with(|cs| {
        nb::block!(radc.borrow(cs).borrot_mut().read(regen)).unwrap()
    };

    println!("throttle = {}, regen = {}", throttle_val, regen_val);
}

[jessebraham]

This issue is a bit old now, and I'm not sure that's there's anything specifically actionable in here to justify keeping it open. Our ADC driver has improved in the meantime but it definitely still needs some more work, this is something we're aware of and plan to address in the near future.

So, with that said, I'm going to close this issue. If anybody feels this is the wrong course of action please do not hesitate to speak up, and if anybody has any specific suggestions on how to improve the driver, issues and/or PRs are always appreciated!