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

PWM: Single-phase initialization creates flicker #15121

Open
chrysn opened this issue Sep 29, 2020 · 2 comments
Open

PWM: Single-phase initialization creates flicker #15121

chrysn opened this issue Sep 29, 2020 · 2 comments
Assignees
Labels
Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Comments

@chrysn
Copy link
Member

chrysn commented Sep 29, 2020

Description

The current initialization sequence for PWM is:

  • pwm_init(), which sets all values to 0
  • pwm_set()

In active-low situations, this creates flickering between the init and the set.

Steps to reproduce the issue

Check out #15115 and run the saul example on an nrf52840 (whose LEDs are wired active-low). In a brief instant, all LEDs are on before going to their natural (per saul_pwm autoinitialization that sets LEDs dark) state.

Roadmap

The API addition I propose to solve this is simple: Introduce pwm_init_poweroff() (pwm_init2? pwm_init_off?) that does the same as init but leaves the device in its poweroff mode. Then, the application can set its initial state, and finally pwm_poweron() to make it actually run.

I hope to do this without introducing a feature that's actually a bug-workaround and will never ever go away again. Thus my initial PR will, unless someone comes up with a better option,

  • Add the pwm_init_poweroff function with a documentation comment flagging it as experimental.
  • Implement it for platforms I can test it on.
  • Replace those platforms' pwm_init() with pwm_init_poweroff(); pwm_poweron() to reduce code duplication.

I'd then issue a call for platform maintainers to follow the changes.

If the new function is not implemented everywhere after a release cycle, we can still put it behind a feature and remove the experimental label; otherwise, the experimental can go when the last platform is merged.

@chrysn chrysn added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers labels Sep 29, 2020
@chrysn chrysn self-assigned this Sep 29, 2020
@chrysn
Copy link
Member Author

chrysn commented Dec 11, 2020

A follow-up discussion of this could become a more principal one; current driver guides state that

Device drivers should be as easy to use as possible. This implies an 'initialize->ready' paradigm, meaning, that device drivers should be ready to use and in operation right after they have been initialized.

which is not what'd be happening there. On the long run, should we reconsider that paradigm?

@chrysn
Copy link
Member Author

chrysn commented Dec 11, 2020

For DAC we probably have the same issue (there, the typical situation is not "it's active low", but "its idle value is 32768" because the output is driving a speaker and supposed to be DC-free on an unbalanced line).

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

No branches or pull requests

2 participants