Skip to content

PwmPin: allow get_duty to return the old value for some time#140

Closed
chrysn wants to merge 1 commit into
rust-embedded:masterfrom
chrysn-pull-requests:pwm-get-old-ok
Closed

PwmPin: allow get_duty to return the old value for some time#140
chrysn wants to merge 1 commit into
rust-embedded:masterfrom
chrysn-pull-requests:pwm-get-old-ok

Conversation

@chrysn

@chrysn chrysn commented May 28, 2019

Copy link
Copy Markdown
Contributor

PWM is often implemented in a buffered way to allow glitch-free operation; as a result, implementing a strict "you get what you last set" is not feasible for some implementations.


This change allows the implementation to decide which value to return for get_duty while it is being changed. The current wording is "current duty cycle", which seems to be quite clear but deviates from the usual semantics of getters and setters in a way that can lead to different interpretations.

In particular, the EFM32 implementation gives the old value until the cycle has actually been changed (at least since a semi-related WIP bugfix) -- but it seems to me that it makes sense to allow that given how it can be implemented, especially considering that basic embedded-hal traits should be widely implementable.

Alternatives:

  • Prescribe "it's always the current duty cycle"
    • If that's the original intent, I'd be happy to change this PR to instead add "Note that setting a PWM duty cycle may not take effect immediately. After set_duty has been called, this reports the old duty cycle until the change has been executed." or similar.
    • I don't know whether it can be implemented like that everywhere.
  • Prescribe "get after set gives last set value"
    • This may not be possible to implement on all platforms while keeping PwmPin zerosized
  • Make set_duty block (or nonblockingly refuse access to the peripheral while it's being changed)

PWM is often implemented in a buffered way to allow glitch-free
operation; as a result, implementing a strict "you get what you last
set" is not feasible for some implementations.
@rust-highfive

Copy link
Copy Markdown

r? @ryankurte

(rust_highfive has picked a reviewer for you, use r? to override)

@therealprof therealprof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense to me.

@no111u3

no111u3 commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

It's looks good for me, and I thinks it need to merge.

@eldruin eldruin added this to the v1.0.0 milestone Jul 15, 2020
@eldruin

eldruin commented Jul 15, 2020

Copy link
Copy Markdown
Member

LGTM.
@chrysn could you rebase this to master?

@thejpster thejpster mentioned this pull request Jul 15, 2020
@bors bors Bot closed this in 5b898bb Jul 22, 2020
bors Bot added a commit that referenced this pull request Jul 22, 2020
238:  Pwm: allow get_duty to return the old value for some time r=therealprof a=eldruin

This is #140 applied to the `Pwm` trait as well, since the same concerns apply. Follows up on #236.

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Review is incomplete T-hal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants