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

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

Open
koutoftimer opened this issue Feb 9, 2021 · 2 comments
Open
Labels
hal-api API design for the different components of avr-hal

Comments

@koutoftimer
Copy link

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

Rahix commented Feb 10, 2021

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

koutoftimer commented Feb 14, 2021

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

@Rahix Rahix added the hal-api API design for the different components of avr-hal label Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hal-api API design for the different components of avr-hal
Projects
None yet
Development

No branches or pull requests

2 participants