embassy icon indicating copy to clipboard operation
embassy copied to clipboard

Custom timer

Open usbalbin opened this issue 1 month ago • 10 comments

This is a (very rough draft of) flexible timer driver for all STM32 timers. Tries to give quite unhindered access to most timer features while offering some type level protection for incorrect configuration.

Example usage

let out_pin = PwmPin::new(out_pin, crate::gpio::OutputType::PushPull);

let mut tim = CustomPwmBuilder::new(tim)
    //.frequency(Hertz(123))
    .prescaler_and_period(0, 1337)
    .ch1_input(
        trigger_pin,
        FilterValue::FDTS_DIV32_N8,
        InputCaptureMode::BothEdges,
        InputTISelection::Normal,
        1,
    )
    .trigger_from_ch1(TriggerMode::TriggerMode, TriggerSource::Filtered)
    .ch2(out_pin, OutputCompareMode::PwmMode2, 800)
    .ch3_input(
        capture_pin,
        FilterValue::FCK_INT_N2,
        InputCaptureMode::Rising,
        InputTISelection::Normal,
        0,
    )
    .one_pulse_mode()
    .finalize();

tim.set_compare_value(150, super::Channel::Ch2);
tim.waveform_up(dma, super::Channel::Ch1, &[100, 400, 800, 1100, 1200])
    .await;
let _capture = tim.wait_for_configured_edge(super::Channel::Ch3).await;

While the example above is just made up. I could see it useful for something like a triac controlled soft start with some current limit or other event synchronized lo the period and a zero detect input which triggers the timer on both edges.

usbalbin avatar Nov 15 '25 21:11 usbalbin

You need to drop the pins in the new constructor, so that the pin types do not propagate into the PwmBuilder. The builder nor the timer should store the pins, which will solve the type issues.

xoviat avatar Nov 16 '25 14:11 xoviat

@usbalbin see https://github.com/Dectron-AB/embassy/pull/1/files

xoviat avatar Nov 16 '25 16:11 xoviat

Thanks for the explanation!

usbalbin avatar Nov 16 '25 17:11 usbalbin

We already have two timer APIs: low_level, and then the high-level helpers (Qei, PWM, etc). I think we shouldn't add a third one, unless there's a really really good reason.

  • What can you do with this API that you can't do with the low_level API?
  • Why does it need a new API instead of extending the low_level API?

Dirbaio avatar Nov 16 '25 20:11 Dirbaio

  • What can you do with this API that you can't do with the low_level API?

Safe construction with guardrails to prevent lots of configuration errors.

  • Why does it need a new API instead of extending the low_level API?

See above. Since low_level has all levers available without any checks we can not really add things to make the bad configs go away.

When trying to implement a triac motor control at work we looked at the options. We need external trigger and one pulse mode to trigger on mains zero. Therefore one_pulse timer made sense. However that does not offer any outputs. For the output simple/complementary PWM would have worked. So we ended up having to use the low_level timer. Maybe this is just a weird use case but we felt it useful to allow as much as possible of what the hardware can do without having to drop down to the pac level.

I personally find that there is quite a huge gap between low_level and the high level types. The high level types, while very convenient when you do exactly what they intend, are very hard to work with once you do something slightly different. At that point you lose all(most) the nice high level features when you have to drop down to the low_level timer.

The custom timer(I am open to name suggestions) sits somewhere in between. It can do(once complete) pretty much everything that the high level things does almost as nicely but with less tailored method names and configuration is slightly more involved.

With that said, I am very open to suggestions for how to do this differently/alternatives :)


Perhaps it does not alleviate your concern but what if the high level types where just very thin wrappers around custom timer? Or even just different constructors?

usbalbin avatar Nov 16 '25 21:11 usbalbin

Perhaps it does not alleviate but what if the high level types where just very thin wrappers around custom timer? [...]

On second thought, I suppose that is pretty much just making the low_level timer more high level...

usbalbin avatar Nov 16 '25 22:11 usbalbin

Yeah. I think 3 APIs is too much.

I'm open to making the low_level timer easier to use. For example we could add methods for all settings so you never have to drop down to the PAC.

About ruling out invalid configs: the builder solution you're proposing with 7 generic params and more traits is too complex. The design philosophy of Embassy is to avoid art installations of generics and traits. I don't think it's doable without.

Dirbaio avatar Nov 16 '25 22:11 Dirbaio

Thanks for the read :)

You absolutely have a point. I'll think about it

usbalbin avatar Nov 16 '25 22:11 usbalbin

I have removed CustomPwm and turned the builder into one that builds a low_level::Timer instead. I have also removed all type arguments except 'd and T. I have removed all but channel1 and not updated the examples for now until we agree more on the way forward.

Is this somewhat closer to something you'd be happy with?

usbalbin avatar Nov 17 '25 22:11 usbalbin

Updated the above example(does not compile since not channels are done yet)

let out_pin = PwmPin::new(out_pin, crate::gpio::OutputType::PushPull);

let mut tim = CustomPwmBuilder::new(tim)
    //.frequency(Hertz(123))
    .prescaler_and_period(0, 1337)
    .ch1_input_as_trigger(
        trigger_pin,
        FilterValue::FDTS_DIV32_N8,
        InputCaptureMode::BothEdges,
        InputTISelection::Normal,
        1,
        TriggerMode::TriggerMode, TriggerSource::Filtered
    )
    .ch2(out_pin, OutputCompareMode::PwmMode2, 800)
    .ch3_input(
        capture_pin,
        FilterValue::FCK_INT_N2,
        InputCaptureMode::Rising,
        InputTISelection::Normal,
        0,
    )
    .one_pulse_mode()
    .finalize();

tim.set_compare_value(150, super::Channel::Ch2);
tim.waveform_up(dma, super::Channel::Ch1, &[100, 400, 800, 1100, 1200])
    .await;
//let _capture = tim.wait_for_configured_edge(super::Channel::Ch3).await;  <--- TODO?

I think it would probably make sense to turn the chX_input args into a config struct with sensible defaults

usbalbin avatar Nov 17 '25 22:11 usbalbin