search

Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.32k stars 223 forks source link

Feature request: add mode enum for WGMxn (TimernPwm atmega328-hal) #132

Open koutoftimer opened 3 years ago

koutoftimer commented 3 years ago

It is not handy to use Timer1Pwm if you need mod different from default one.

If I understand right, creating Timer1Pwm launches init which sets up CS2:0 bits and starts timer. Now, if you want to change mode you have to stop timer, by reseting CS2:0 bits, set right WGM3:0 bits and reset prescaler because it already have changed it's state. It is impossible to reset prescaler if you need to launch Timer1Pwm when some other timers are running because the all share single physical prescaler.

Easiest solution I can see is to implement enum with modes from datasheet "Table 15-5. Waveform Generation Mode Bit Description (1)" on page 109 and add 3rd parameter to constructor.

Rahix commented 3 years ago

Meta: Please refrain from opening this many issues at once. It comes off as super agressive and just produces noise in everyone's inbox. Instead please try to explain concisely in a single issue what you're trying to do, what's not working, what you think causes it and what we could do to fix it. We can then later decide whether this warrants separate issues to track progress on multiple topics.

Anyway, I am certain you acted in good faith here, just please keep this in mind for the future.

Regarding your actual issue:

The current Timer#Pwm abstraction is not meant and wasn't designed to support other modes than "Fast PWM". It is supposed to just cover the most simple use-case of a fixed-frequency PWM signal with a duty-cycle that can be varied with 256 steps. This is enough for e.g. super basic servo control or controlling LED brightness.

I guess the documentation should be much more clear about this, though.

For any more complex usecase I'm afraid the best way right now is to build your own custom abstraction for the time being or just interacting with the timer peripheral directly (like e.g. in uno-hc-sr04.rs).

In the future I definitely want to have HAL abstractions supporting the more complex use-cases as well, see #44 for example. Those should be built differently than what we have right now, though, as a part of the refactoring effort in #130 probably.

koutoftimer commented 3 years ago

@Rahix this is my try (bad) to expose WGM modes that are, at least, statically verified to match specified timer.

Intended usage 

    let mut timer1 = Timer1Pwm::new(pd.TC1, Prescaler::Prescale8);
    type WgmMode = <Timer1Pwm as TimerConf>::WgmMode;
    timer1.set_wgm(WgmMode::FpwmIcrBottomTop);

Perhaps TimerConf is not best naming but it intended to expose internal common interface (aka abstraction) for all timers. Internal in opposite to Pwm trait from embedded_hal.