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
479 stars 172 forks source link

How to use `PinDriver` across threads #221

Closed cwoolum closed 1 year ago

cwoolum commented 1 year ago

I'm trying to build a generic helper that is able to take in a pin and can just be responsible for managing that pin from then on.

I'm having trouble with 2 pieces. First I'm having trouble finding the right abstraction to use to pass in any output pin. I've tried both OutputPin as well as AnyOutputPin but neither seem to work. Here's an example where I've just passed in Gpio4 for now.

pub struct PumpManager<'a> {
    pin: PinDriver<Gpio4, Output>,
}

impl PumpManager<'_> {
    pub fn new(pin: PinDriver<Gpio4, Output>) -> PumpManager {
        PumpManager { pin }
    }

    pub fn run_pump_for_period(self, time: u64) -> Result<(), EspError> {
        self.pin.set_high().unwrap();

        let once_timer = EspTimerService::new().unwrap().timer(move || {
            self.pin.set_low().unwrap();
        });

        once_timer.unwrap().after(Duration::from_secs(time))
    }
}

The other issue is that if I define my pin or PinDriver at the top level of my main function, I always get the error *const () cannot be shared between threads safely within [closure@src\main.rs:116:15: 116:35], the trait 'Sync' is not implemented for '*const ()'

I've also tried defining the PumpManager at the top level and just using it there but still get the const () cannot be shared error. What is the best practice for this?

ivmarkov commented 1 year ago

I'm not sure what you are saying exactly: does the above code snippet compile or not? I think it should, because PinDriver is currently Send (but not Sync indeed, which would be more complex to do and I'm unsure if we should do it)?

ivmarkov commented 1 year ago

As for using AnyOutputPin - this should work without any problems. Unless you show a code snippet where it does not work, I can only speculate what the issue at hand is.

cwoolum commented 1 year ago

My apologies! Let me address AnyOutputPin first.

use crate::pins::PumpPin;
use crate::EspError;

use embedded_svc::timer::OnceTimer;
use embedded_svc::timer::TimerService;
use esp_idf_hal::gpio::AnyIOPin;
use esp_idf_hal::gpio::*;
use esp_idf_hal::gpio::{self, InputOutput};
use esp_idf_hal::peripheral::Peripheral;
use esp_idf_hal::peripherals::Peripherals;

use esp_idf_svc::timer::*;
use std::fmt;
use std::{convert::Infallible, fmt::Display, time::Duration};

#[derive(Debug)]
pub enum PumpStatus {
    Idle,
    Pumping,
    PumpCompleted,
}

pub enum FluidPumps {
    Pump1,
}

pub struct PumpManager {
    pump1: PinDriver<'static, Gpio4, Output>,
}

impl PumpManager {
    pub fn new() -> PumpManager {
        let peripherals = Peripherals::take().unwrap();
        let pin4_pump = PinDriver::output(peripherals.pins.gpio4).unwrap();

        PumpManager { pump1: pin4_pump }
    }

    fn get_output_for_pump(self, pump_identifier: FluidPumps) -> PinDriver<'static, AnyOutputPin, Output> {
        let pump = match pump_identifier {
            FluidPumps::Pump1 => self.pump1,
        };

        return pump; // This throws an error
    }

    pub fn run_pump_for_period(
        self,
        time: u64,
        pump_identifier: FluidPumps,
    ) -> Result<(), EspError> {
        let pin = self.get_output_for_pump(pump_identifier);

        pin.set_high().unwrap();

        let once_timer = EspTimerService::new().unwrap().timer(move || {
            pin.set_low().unwrap();
        });

        once_timer.unwrap().after(Duration::from_secs(time))
    }
}

In get_output_for_pump, I want to return the matching output for a given pump. If I use the return type PinDriver<'static, AnyOutputPin, Output>, I get the error

mismatched types
expected struct `esp_idf_hal::gpio::PinDriver<'static, esp_idf_hal::gpio::AnyOutputPin, _>`
   found struct `esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, _>`
Vollbrecht commented 1 year ago

hmm your problem is not related to usage related accross threads. its simply an type error. A concrete Pin like Gpio4 is something different than AnyOutputPin. If you want to make it a bit more dynamic and for example allow the PumpManager to get any pin on creation you can crate something like the following:

struct Manager {
    item: PinDriver<'static, AnyIOPin, Output>
}

impl Manager {
    pub fn new(pin: impl IOPin + 'static) -> Manager {
        let pin = pin.downgrade();
        let item = PinDriver::output(pin).unwrap();
        Manager { item }
    }

    fn get_item(self) -> PinDriver<'static, AnyIOPin, Output> {
        self.item
    }

    pub fn do_stuff(self) {
        let mut driver = self.get_item();
        driver.set_low();
    }
}

and in your main you can create it with

let per = Peripherals::take().unwrap();
let manager = Manager::new(per.pins.gpio4);
cwoolum commented 1 year ago

I combined 2 problems in my original post which was confusing. I've scoped this down to just the types issue for now to simplify the problem

I gave pump1: PinDriver<'static, AnyIOPin, Output>, a try but get the error

mismatched types
expected struct `esp_idf_hal::gpio::PinDriver<'_, AnyIOPin, _>`
   found struct `esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, _>`

I also tried using into() with the following code:

let peripherals = Peripherals::take().unwrap(); // Peripherals
let pin4_pump = PinDriver::output(peripherals.pins.gpio4).unwrap(); // PinDriver<Gpio4, Output>

PumpManager { pump1: pin4_pump.into() }

but get the error:

  --> src\pump_manager.rs:42:40
   |
42 |         PumpManager { pump1: pin4_pump.into() }
   |                                        ^^^^ the trait `From<esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, esp_idf_hal::gpio::Output>>` is not implemented for `esp_idf_hal::gpio::PinDriver<'_, AnyIOPin, esp_idf_hal::gpio::Output>`
   |
   = note: required for `esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, esp_idf_hal::gpio::Output>` to implement `Into<esp_idf_hal::gpio::PinDriver<'_, AnyIOPin, esp_idf_hal::gpio::Output>>`
ivmarkov commented 1 year ago

I also tried using into() with the following code:

let peripherals = Peripherals::take().unwrap(); // Peripherals
let pin4_pump = PinDriver::output(peripherals.pins.gpio4).unwrap(); // PinDriver<Gpio4, Output>

PumpManager { pump1: pin4_pump.into() }

but get the error:

  --> src\pump_manager.rs:42:40
   |
42 |         PumpManager { pump1: pin4_pump.into() }
   |                                        ^^^^ the trait `From<esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, esp_idf_hal::gpio::Output>>` is not implemented for `esp_idf_hal::gpio::PinDriver<'_, AnyIOPin, esp_idf_hal::gpio::Output>`
   |
   = note: required for `esp_idf_hal::gpio::PinDriver<'_, esp_idf_hal::gpio::Gpio4, esp_idf_hal::gpio::Output>` to implement `Into<esp_idf_hal::gpio::PinDriver<'_, AnyIOPin, esp_idf_hal::gpio::Output>>`

You need to .into() the Gpio4 peripheral before instantiating the PinDriver onto it. As in:

let pin4_pump = PinDriver::output(peripherals.pins.gpio4.into()).unwrap(); // PinDriver<Gpio4, Output>

And your PumpManager struct layout should be:

pub struct PumpManager {
    pump1: PinDriver<'static, AnyOutputPin, Output>,
}

As for using PinManager across multiple threads (Sync) easiest way to achieve it is to wrap it in a Mutex. Reading on Sync and Send might help you understand what is going on. PinManager (for now) is not Sync. We might change it in future, as most of the ESP IDF GPIO code underneath seems sync-safe, but not for now.

cwoolum commented 1 year ago

I was able to make AnyOutputPin work but the compiler forced me to be explicit about the type for Into

let pin4_pump = PinDriver::output(<esp_idf_hal::gpio::Gpio4 as Into<AnyOutputPin>>::into(
    peripherals.pins.gpio4,
)).unwrap();