esp-rs / esp-idf-hal

embedded-hal implementation for Rust on ESP32 and ESP-IDF
https://docs.esp-rs.org/esp-idf-hal/
Apache License 2.0
472 stars 171 forks source link

Fix drop for AdcDriver in deregister event callbacks #487

Closed vpetryaev closed 1 month ago

vpetryaev commented 1 month ago

In dropping AdcDriver instead of deregister event callbacks, set pointer to struct adc_continuous_evt_cbs_t to core::ptr::null(). As result, esp_idf_hal raise error:

E (408) adc_continuous: adc_continuous_register_event_callbacks(514): invalid argument
thread 'main' panicked at /home/ptr/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-idf-hal-0.44.1/src/adc.rs:1330:18:
...
Rebooting...
...

Original documentation said "User can deregister a previously registered callback by calling this function and setting the to-be-deregistered callback member in the cbs structure to NULL". So, deregistration must set to NULL only function pointers, not pointer to their struct.

My test code:

use esp_idf_hal::peripherals::Peripherals;

fn main() -> anyhow::Result<()> {
    use esp_idf_svc::hal::adc::{AdcContConfig, AdcContDriver, AdcMeasurement, Attenuated};
    esp_idf_svc::sys::link_patches();
    esp_idf_svc::log::EspLogger::initialize_default();
    let peripherals = Peripherals::take()?;

    let config = AdcContConfig {
        sample_freq: esp_idf_hal::prelude::Hertz(1000),
        frame_measurements: 16,
        frames_count: 4
    };

    let adc_1_channel_0 = Attenuated::none(peripherals.pins.gpio0);
    let mut adc = AdcContDriver::new(peripherals.adc1, &config, adc_1_channel_0)?;
    adc.start()?;
    let mut samples = [AdcMeasurement::default(); 16];
    for _i in 0..5 {
        if let Ok(num_read) = adc.read(&mut samples, 100) {
            println!("Read {} measurement.", num_read);
            for index in 0..num_read {
                println!("ADC{} GPIO{} = {}, ", samples[index].unit(), samples[index].channel(), samples[index].data());
            }
        }
    }
    drop(adc);
    Ok(())
}
Vollbrecht commented 1 month ago

Thanks for the PR fixing stuff!

Against what ESP_IDF_VERSION did you run that test? Do you per chance know if the adc_continuous_register_event_callbacks call always took a adc_continuous_evt_cbs_t or did they changed "recently" ?