Motor Control Pulse Width Modulator (MCPWM)

[usbalbin]

trafficstars

See #92

Example:

let peripherals = Peripherals::take().unwrap();
let config = OperatorConfig::default().frequency(25.kHz().into());
let mcpwm = Mcpwm::new(peripherals.mcpwm0.mcpwm)?;
let mut operator = Operator::new(
    peripherals.mcpwm0.operator0,
    &mcpwm,
    &config,
    peripherals.pins.gpio4,
    peripherals.pins.gpio5,
)?;

operator.set_duty_a(my_duty_percentage_a)?; // Set duty for pin 4
operator.set_duty_b(my_duty_percentage_b)?; // Set duty for pin 5

Todo:

• [-] Add dead time example - Maybe wait for a later PR?
  • [x] Figure out how daed time works with regards to MCPWMXB, have a sneak peak at IDF example - all dead time modes except MCPWM_ACTIVE_RED_FED_FROM_PWMXB makes set_duty_b do nothing
• [-] More examples/tests? - Maybe wait for a later PR?
• [x] Improve documentation
  • [x] Check with the really great new docs
• [x] Test on actual hardware ~~(no tests performed on actual hardware in any way yet!)~~
  • [x] ESP32
    • [x] Example: mcpwm-simple
    • [x] Dead time: DeadtimeConfig::ActiveHighComplement
  • [x] ESP32-S3
    • [x] Example: mcpwm-simple
    • [x] Dead time: DeadtimeConfig::ActiveHighComplement
• [ ] Cleanup? Do we need to implement Drop to clean up stuff?
  • [ ] What about release() ?
• [ ] More?

[usbalbin]

~~Have not have had time to check if this even compiles since my last changes...~~

Anyways is this somewhat what we want things to look like api wise? :)

[usbalbin]

One of my main reasons fo wanting to use MCPWM is for syncing multiple timers with a phase offset. How should that best be exposed? Something like a new type SyncedOperator which would offer the same functionality as Operator but with the added method fn set_phase(&mut self, phase_percentage: f32). Perhaps it could be used something like:

let operator_0 = Operator::new(...);
let operator_1 = Operator::new(...);

// This will be synced to operator_0 with a phase shift of 50%
let synced_operator_1 = SyncedOperator(operator_1, &operator_0, 50.0);

// Should SyncedOperator keep a reference to its sync source(operator_0 in this case)?
// That would prevent updating phase shift when chaining multiple levels (& xor &mut).
// will try to elaborate on my though another day...

[usbalbin]

~~Is this perhaps to be considered blocked on some sort of ESP/ESP-S3 CI?~~ - #96 merged

[MabezDev]

Sorry! I went on vacation, and I was also waiting on some hardware so I can test this out :). Taking a look into this now.

[usbalbin]

No worries :)

[usbalbin]

Just found a problem in release mode in the mcpwm-simple example

[usbalbin]

The same thing in debug mode works fine

[MabezDev]

Just found a problem in release mode in the mcpwm-simple example

I can't seem to reproduce here (judging by the output, it looks like there were some extra changes to the example which haven't been pushed).

Could you rebuild, capture the output again and upload the elf too?

[usbalbin]

Oh right sorry. I'll push when I get home. I believe I reduced the step size to something like 0.1% and sleep time to 10ms, only printing on integer values of duty.

Same error on both ESP and ESP32-S3(clean build).

Again, I will try to push the code in a couple of hours

[usbalbin]

I am currently at:

#[cfg(any(esp32, esp32s3))]
fn main() -> anyhow::Result<()> {
    use embedded_hal::delay::blocking::DelayUs;
    use esp_idf_hal::delay::FreeRtos;

    esp_idf_sys::link_patches();

    let v = f32::from(159_u16) * 0.1;
    println!("159 to float is={v}");  // 159 to float is=15.900001 as expected

    let v = f32::from(160_u16) * 0.1; // Boom
    println!("160 to float is={v}");       // Or perhaps boom here?

    loop {
        FreeRtos.delay_ms(10)?;
    }
}

So seems to be unrelated to MCPWM. Also, removing the * 0.1 makes everything work fine

[usbalbin]

This also works fine:

#[cfg(any(esp32, esp32s3))]
fn main() -> anyhow::Result<()> {
    use embedded_hal::delay::blocking::DelayUs;
    use esp_idf_hal::delay::FreeRtos;

    esp_idf_sys::link_patches();

    let v = f32::from(159_u16) * 0.1;
    println!("159 to float is={v}");

    let v = f32::from(160_u16) * 0.1;
    println!("160 to float is=some number");
    
    let v = v + 0.1;
    println!("is={v}");
    

    loop {
        FreeRtos.delay_ms(10)?;
    }
}

[usbalbin]

#[cfg(any(esp32, esp32s3))]
fn main() -> anyhow::Result<()> {
    esp_idf_sys::link_patches();

    let v = 16.0;
    println!("f32 literal is={v}");

    let v = f32::from(16_u16);
    println!("Converting the number seems to work");
    println!("from u16 to f32 is={v}"); // Boom
    loop {}
}

[usbalbin]

Created #112 since I think the problem might be unrelated to this PR

[usbalbin]

Reading the IDF 4.4 to 5.0 migration guide

The legacy driver has an inappropriate assumption, that is the MCPWM operator should be connected to different MCPWM timer. In fact, the hardware doesn’t have such limitation. In the new driver, a MCPWM timer can be connected to multiple operators, so that the operators can achieve the best synchronization performance.

This changes a lot of things. Something we should think of now right away or save for a later PR?

[MabezDev]

Sorry, I haven't forgotten about this! I will try to review this early next week.

[usbalbin]

Hi @MabezDev ,

No rush, just a friendly reminder :)

[liebman]

These four methods are needed for Operator when dealing with brushed motors:

    /// set 'a' low
    pub fn set_low_a(&mut self) -> Result<(), EspError> {
        unsafe {
            esp!(esp_idf_sys::mcpwm_set_signal_low(
                U::unit(),
                O::timer(),
                esp_idf_sys::mcpwm_generator_t_MCPWM_GEN_A
            ))
        }
    }

    /// set 'b' low
    pub fn set_low_b(&mut self) -> Result<(), EspError> {
        unsafe {
            esp!(esp_idf_sys::mcpwm_set_signal_low(
                U::unit(),
                O::timer(),
                esp_idf_sys::mcpwm_generator_t_MCPWM_GEN_B
            ))
        }
    }

    /// set 'a' high
    pub fn set_high_a(&mut self) -> Result<(), EspError> {
        unsafe {
            esp!(esp_idf_sys::mcpwm_set_signal_high(
                U::unit(),
                O::timer(),
                esp_idf_sys::mcpwm_generator_t_MCPWM_GEN_A
            ))
        }
    }

    /// set 'b' high
    pub fn set_high_b(&mut self) -> Result<(), EspError> {
        unsafe {
            esp!(esp_idf_sys::mcpwm_set_signal_high(
                U::unit(),
                O::timer(),
                esp_idf_sys::mcpwm_generator_t_MCPWM_GEN_B
            ))
        }
    }

[usbalbin]

These four methods are needed for Operator when dealing with brushed motors:

Any suggestions for how to undo the effect of those methods? How do I restore the normal behaviour after, for example, set_low_a has been called? Also note that the answer might depend on whether MCPWM from IDF <= 4.4 or from IDF 5.0 is used. (IDF <= 4.4 for this PR, 5.0 for #132).

As far as I know, for IDF 4.4 I believe mcpwm_set_duty_type has to be called to restore output. Have not looked into any 5.0 equivalent yet. Please correct me if I am wrong

[liebman]

You are correct. mcpwm_set_duty_type is needed as well. (missed that when I pulled the others from my C++ brushless motor driver.)

[liebman]

Question: (and this probably reveals my naiveté with rust) How can I represent one of the motor operators generically - like in a system where I'm controlling three motors say left wheel, right wheel, and lidar motor. I want to store a reference (or pass ownership) to another struct... but the signature make it difficult as I'll have more than one instance each using separate operators:

Operator<U: Unit, O: HwOperator<U>, M: Borrow<Mcpwm<U>>, PA: OutputPin, PB: OutputPin>

Thoughts? (even RTFM if you have a pointer)

[usbalbin]

Question: (and this probably reveals my naiveté with rust) How can I represent one of the motor operators generically - like in a system where I'm controlling three motors say left wheel, right wheel, and lidar motor. I want to store a reference (or pass ownership) to another struct... but the signature make it difficult as I'll have more than one instance each using separate operators:

Operator<U: Unit, O: HwOperator<U>, M: Borrow<Mcpwm<U>>, PA: OutputPin, PB: OutputPin>

Thoughts? (even RTFM if you have a pointer)

Great question! Hard to say without knowing more. However assuming you want to have two operators live in the same struct sharing the same mcpwm module(also living in the struct), then perhaps something like(not tested):

struct Foo<U: Unit, O0: HwOperator<U>, O1: HwOperator<U>, P0B: OutputPin, P0B: OutputPin> 
where
    U: Unit,
    O0: HwOperator<U>,
    O1: HwOperator<U>,
    P0A: OutputPin, P0B: OutputPin,
    P1A: OutputPin, P1B: OutputPin
{
    mcpwm: Arc<Mcpwm<U>>,
    operator0: Operator<U, O0, Arc<Mcpwm<U>>, P0A, P0B>,
    operator1: Operator<U, O1, Arc<Mcpwm<U>>, P1A, P1B>,
}

or something like this if you want to have the mcpwm module live outside the struct(not tested):

struct Bar<'a, U, O0, O1, P0B, P0B> 
where
    U: Unit,
    O0: HwOperator<U>,
    O1: HwOperator<U>,
    P0A: OutputPin, P0B: OutputPin,
    P1A: OutputPin, P1B: OutputPin
{
    operator0: Operator<U, O0, &'a Mcpwm<U>, P0A, P0B>,
    operator1: Operator<U, O1, &'a Mcpwm<U>, P1A, P1B>,
}

Let me know if this answered your question :)

[liebman]

Yes - that was very helpful! This worked:

pub struct MotorMCPWM<'d, U: Unit, O: HwOperator<U>, PA: OutputPin, PB: OutputPin>
{
    operator: Operator<U, O, &'d Mcpwm<U>, PA, PB>,
    direction: Direction,
    percent: Percent,
    invert: bool,
}

impl<'d, U: Unit, O: HwOperator<U>, PA: OutputPin, PB: OutputPin> MotorMCPWM<'d, U, O, PA, PB>
{
    pub fn new(mcpwm: &'d Mcpwm<U>, operator: O, pin_ph: PA, pin_en: PB, invert: bool) -> Result<Self, EspError>
    {
        info!("MotorMCPWM::new ph={} en={}", pin_ph.pin(), pin_en.pin());
        let config = OperatorConfig::default().frequency(50.Hz());
        Ok(Self {
            operator: Operator::new(
                operator,
                &*mcpwm,
                &config,
                pin_ph,
                pin_en,
            )?,
            direction: Direction::Stopped,
            percent: 0 as Percent,
            invert: invert,
        })
    }
}

And code not shown implements the following trait to make the motor generic, letting me not have everything having to deal with the long type specification.

pub type Percent = f32;

#[derive(Clone, Copy, Debug)]
pub enum Direction {
    Stopped,
    Forward,
    Reverse,
}

pub trait Motor {
    type Error;
    fn set_percent(&mut self, percent: Percent) -> Result<(), Self::Error>;
    fn set_direction(&mut self, direction: Direction) -> Result<(), Self::Error>;
}

[MabezDev]

Hi @usbalbin, firstly I want to apologize for the time between reviews. We've had a succession of compiler issues in the last couple of releases (one of which you found :D) and most of my time has been spent digging into that.

Looking at this again, and the surrounding discussion this looks good to me! My understanding is that #132 builds on top of this PR, so we should merge this first?

I think this is ready to merge as is, would you mind rebasing on the main branch?

[usbalbin]

Hi @usbalbin, firstly I want to apologize for the time between reviews. We've had a succession of compiler issues in the last couple of releases (one of which you found :D) and most of my time has been spent digging into that.

No worries :)

[...] My understanding is that #132 builds on top of this PR, so we should merge this first?

In the sense git sense, sure. However it has since been modified beyond recognition to better fit with how things are exposed in MCPWM from IDF 5, see comment for more info.

One of the bigger differences being that timer and operator are two independent things in #132. So #132 would very much be a breaking change. However at the same time this pr (#93) is for IDF < 5 while #132 is for IDF >= 5...

Going the other way, using some clever generic bounds there might be possible to make something that looks like #132 work for IDF < 5 but with lots of restrictions like timer1 can only connect to operator1...

I am happy either way, and I will attempt a rebase soon

Example usage of #92 (this PR)

let peripherals = Peripherals::take().unwrap();
let config = OperatorConfig::default().frequency(25.kHz().into());
let mcpwm = Mcpwm::new(peripherals.mcpwm0.mcpwm)?;
let mut operator = Operator::new(
    peripherals.mcpwm0.operator0,
    &mcpwm,
    &config,
    peripherals.pins.gpio4,
    peripherals.pins.gpio5,
)?;

operator.set_duty_a(my_duty_percentage_a)?; // Set duty for pin 4
operator.set_duty_b(my_duty_percentage_b)?; // Set duty for pin 5

Example usage of #132

(Not tested)

let operator_config = OperatorConfig::default();
let timer = Timer::new(peripherals.mcpwm0.timer0, timer_config);

let mut timer = timer.into_connection().attatch_operator0( // This is the connection between a timer and 0-3 operators
    peripherals.mcpwm0.operator0,
    operator_config,
    peripherals.pins.gpio4,
    peripherals.pins.gpio5,
);


// This hands out mutable references to the contained timers and operators
let (timer, operator, _unused_operator_slot1, _unused_operator_slot2) = timer.split();

let period_ticks = timer.get_period_peak();
operator.set_duty_a(my_duty_ticks_a)?; // Set duty for pin 4 in units of timer ticks
operator.set_duty_b(my_duty_ticks_b)?; // Set duty for pin 5

[usbalbin]

Do you want me to reduce the number of commits? If so, then how? One commit or multiple?

[MabezDev]

One of the bigger differences being that timer and operator are two independent things in https://github.com/esp-rs/esp-idf-hal/pull/132. So https://github.com/esp-rs/esp-idf-hal/pull/132 would very much be a breaking change. However at the same time this pr (https://github.com/esp-rs/esp-idf-hal/pull/93) is for IDF < 5 while https://github.com/esp-rs/esp-idf-hal/pull/132 is for IDF >= 5...

Going the other way, using some clever generic bounds there might be possible to make something that looks like https://github.com/esp-rs/esp-idf-hal/pull/132 work for IDF < 5 but with lots of restrictions like timer1 can only connect to operator1...

Perhaps then we should actually develop the interface for esp-idf v5 first then, if it's more flexible? We can then re-use that same interface for v4.4 but with the caveat that we panic if operator0 is not used with the timer0 for example. Along the way, we might find we can enforce this with generic bounds, but If not I still think it's perfectly fine to panic in that situation.