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

Motor Control Pulse Width Modulator (MCPWM) #93

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Jun 26, 2022

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?
    • 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?
  • Improve documentation
    • Check with the really great new docs
  • Test on actual hardware (no tests performed on actual hardware in any way yet!)
    • ESP32
      • Example: mcpwm-simple
      • Dead time: DeadtimeConfig::ActiveHighComplement
    • ESP32-S3
      • Example: mcpwm-simple
      • Dead time: DeadtimeConfig::ActiveHighComplement
  • Cleanup? Do we need to implement Drop to clean up stuff?
    • What about release() ?
  • More?

@usbalbin
Copy link
Contributor Author

usbalbin commented Jun 26, 2022

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

This comment was marked as duplicate.

@usbalbin
Copy link
Contributor Author

usbalbin commented Jun 30, 2022

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

@MabezDev
Copy link
Member

MabezDev commented Jul 5, 2022

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
Copy link
Contributor Author

usbalbin commented Jul 5, 2022

No worries :)

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long to review this! Appreciate your patience :). It's looking good! I don't have any strong opinions on the API, what you have come up with seems pretty good to me!

I think some TODO's need to be addressed, I've commented on most of them where I think they should be. The rest could be left as TODO, as I understand its quite an amount of effort implementing the whole MCPWM API

src/mcpwm.rs Outdated Show resolved Hide resolved
src/mcpwm.rs Outdated Show resolved Hide resolved
src/mcpwm.rs Outdated
// * High res => small prescaler => better maintan accuracy at high frequency, unable to reach lower freq
// * Lower res => larger prescaler => worse accuracy at high frequency(still as good at low frequency),
// able to reach lower freq
// Would it make more sense to expose it as "lowest reachable frequency"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to expose it as "lowest reachable frequency"

I think this is the best way to express it, makes more sense this way imo.

What happens if prescaler register value does not fit into the least significant 8 bits?

Probably bad things :D. We can either add the check here or PR to esp-idf and fix it there.

src/mcpwm.rs Outdated Show resolved Hide resolved
src/mcpwm.rs Outdated Show resolved Hide resolved
examples/mcpwm-simple.rs Outdated Show resolved Hide resolved
examples/mcpwm-simple.rs Outdated Show resolved Hide resolved
src/mcpwm.rs Show resolved Hide resolved
return Err(EspError::from(ESP_ERR_INVALID_ARG).unwrap());
}

let resolution = u32::from(config.lowest_frequency) * MAX_PWM_TIMER_PERIOD;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to what I mentioned earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we clamp resolution to not be greater than mcpwm_module.operator_source_frequency?

src/mcpwm.rs Show resolved Hide resolved
src/mcpwm.rs Outdated
// TODO: Is this actually true? --------
// |
// v
/// MCPWM_ACTIVE_HIGH_COMPLIMENT_MODE - The most common deadtime mode
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most common deadtime mode
This mode seemed like the one making most sense to me... Is this true or should I remove that comment? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be true (idk :D) but I'd say remove the comment regardless.

src/mcpwm.rs Show resolved Hide resolved
src/mcpwm.rs Show resolved Hide resolved
src/mcpwm.rs Outdated Show resolved Hide resolved
src/mcpwm.rs Outdated
// TODO: Is this actually true? --------
// |
// v
/// MCPWM_ACTIVE_HIGH_COMPLIMENT_MODE - The most common deadtime mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be true (idk :D) but I'd say remove the comment regardless.

src/mcpwm.rs Show resolved Hide resolved
src/mcpwm.rs Show resolved Hide resolved
src/mcpwm.rs Outdated Show resolved Hide resolved
@usbalbin
Copy link
Contributor Author

usbalbin commented Aug 6, 2022

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
Copy link
Contributor Author

usbalbin commented Aug 6, 2022

#[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
Copy link
Contributor Author

usbalbin commented Aug 6, 2022

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

@usbalbin
Copy link
Contributor Author

usbalbin commented Aug 6, 2022

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?

@usbalbin usbalbin marked this pull request as ready for review August 12, 2022 15:24
@MabezDev
Copy link
Member

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

@usbalbin
Copy link
Contributor Author

usbalbin commented Sep 2, 2022

Hi @MabezDev ,

No rush, just a friendly reminder :)

@usbalbin usbalbin mentioned this pull request Sep 16, 2022
@liebman
Copy link
Contributor

liebman commented Sep 23, 2022

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
Copy link
Contributor Author

usbalbin commented Sep 24, 2022

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
Copy link
Contributor

liebman commented Sep 24, 2022

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
Copy link
Contributor

liebman commented Sep 24, 2022

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
Copy link
Contributor Author

usbalbin commented Sep 26, 2022

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
Copy link
Contributor

liebman commented Sep 26, 2022

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
Copy link
Member

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
Copy link
Contributor Author

usbalbin commented Nov 14, 2022

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
Copy link
Contributor Author

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

@MabezDev
Copy link
Member

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...

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.

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.

4 participants