Skip to content

Pwm API #7

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

Closed
japaric opened this issue Jun 9, 2017 · 8 comments
Closed

Pwm API #7

japaric opened this issue Jun 9, 2017 · 8 comments

Comments

@japaric
Copy link
Member

japaric commented Jun 9, 2017

  • What is missing in this API?
  • Can this API be implemented for different devices?
@japaric japaric added the RFC label Jun 9, 2017
@japaric
Copy link
Member Author

japaric commented Jun 9, 2017

I'm wondering how useful are the get_period and set_period methods get it comes to building other abstractions, or in even in generic programming. cf. #3 (comment)

@japaric
Copy link
Member Author

japaric commented Jan 20, 2018

Update: this trait is available in release v0.1.0 behind the "unproven" Cargo feature. But there's also a PwmPin trait that's not feature gated.

@eldruin
Copy link
Member

eldruin commented Jan 6, 2020

I could not implement either of the PWM traits for the PCA9685 driver because all the methods are infallible. I did not feel like starting another digital::v2 debacle so I've just dropped it.

About set_period/get_period I think it would be cumbersome to implement since it depends on the "real world" frequency on which everything is running, which may be simply an external input and thus subject to external change plus configurable through a prescaler or power configuration or so.
Even when solving this, the driver will need to calculate an approximation to a combination of prescaler and/or other configs and I feel this is something that the driver user would rather determine themselves so that the authoritatively provided frequency is followed exactly.
Additionally, choosing a Time type seems difficult because of precision and conversion issues.

@ryankurte
Copy link
Contributor

@eldruin i believe the fallible component is resolved, do you still have concerns with the current implementation?

@eldruin
Copy link
Member

eldruin commented Jun 20, 2020

Indeed. I believe I could implement a split yielding PwmPins in the PCA9685 driver using the embedded-hal 1.0.0 alpha. I will have a try at this soon and see how it works. It will probably be similar to the pcf857x I/O expander driver.

Regarding set_period/get_period for the Pwm trait, my concerns about the frequency calculation still stand.

@therealprof
Copy link
Contributor

Can you describe your API concerns in a separate issue for better visibility so we can close this ticket and avoid the impression PWM still doesn't exist? 😅

@eldruin
Copy link
Member

eldruin commented Jun 20, 2020

Sure. I filed #226. This can be closed now.

@therealprof
Copy link
Contributor

Thank you!

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
7: move to v0.2.0 of embedded-hal r=japaric a=japaric



Co-authored-by: Jorge Aparicio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants