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

Implement TryFrom<u32> for ledc::timer::config::Duty #1984

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

AnthonyGrondin
Copy link
Contributor

@AnthonyGrondin AnthonyGrondin commented Aug 22, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This is a resurrection for #814 that was closed by mistake a while ago. The intent is to provides a driver for a piezo-electric buzzer by abstracting LEDC and offering a user-friendly API to control the buzzer

Since the GPIO refactoring, I've solved the issue I had with providing a type for an optional volume pin.

Testing

An example was added in esp-hal-buzzer/examples/buzzer.rs and works on my hardware. The duty volume is working on the specific hardware that I have, using logic gates to decrease the output.

Edit: The esp-hal-buzzer package has been moved to https://github.com/esp-rs/esp-hal-community so I've kept only the part of this PR that touches esp-hal, which is to implement TryFrom<u32>

esp-hal/src/ledc/timer.rs Outdated Show resolved Hide resolved
examples/Cargo.toml Outdated Show resolved Hide resolved
examples/src/bin/buzzer.rs Outdated Show resolved Hide resolved
@jessebraham
Copy link
Member

jessebraham commented Sep 4, 2024

Sorry for hijacking your PR for this, but I guess we need to have some discussion with regards to where packages like this belong. We have esp-hal-smartled in the repo, but frankly it's just a bit of an annoyance from the perspective of maintenance/publishing, and barely anybody seems to even use it based off the download stats on crates.io. I'm not sure we really want to take on a number of packages like this given their limited use, though I do acknowledge their utility. Perhaps some of the other maintainers have thoughts on this.

@AnthonyGrondin
Copy link
Contributor Author

I agree with this. I see it like a chicken 🐔 and egg 🥚 situation. Such extra packages are an unnecessary burden because nobody uses them, but nobody uses them because such package doesn't exist.

My original intent was to add the Buzzer as a new driver in the HAL. But since it's not technically a hardware driver, but rather a combination of drivers, it was suggested to move the buzzer driver in its own specific package.

I agree with moving both esp-hal-buzzer and esp-hal-smartled into extras/, and I think we should treat those packages as tier 2 packages, having a lower priority, maintenance, etc. Tier 1 packages being the most important ones, part of the root of the repo.

@jessebraham
Copy link
Member

Sorry for the delay here, it took some time for us to discuss and decide on how we wanted to proceed.

We have decided to create the esp-hal-community repository, to which I have already moved esp-hal-smartled and its associated example. If you wouldn't mind opening a new PR there adding this package, that would be much appreciated!

I have not really documented the contribution process there yet (though I did open an issue), but I hope it is intuitive enough. Feel free to ping me if you need any assistance :)

AnthonyGrondin added a commit to AnthonyGrondin/esp-hal-community that referenced this pull request Sep 16, 2024
AnthonyGrondin added a commit to AnthonyGrondin/esp-hal-community that referenced this pull request Sep 16, 2024
@AnthonyGrondin AnthonyGrondin changed the title feat(buzzer): Add esp-hal-buzzer to drive a piezo-electric buzzer. Implement TryFrom<u32> for ledc::timer::config::Duty Sep 16, 2024
@AnthonyGrondin
Copy link
Contributor Author

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@MabezDev MabezDev added this pull request to the merge queue Sep 17, 2024
@MabezDev MabezDev removed this pull request from the merge queue due to a manual request Sep 17, 2024
@jessebraham jessebraham added this pull request to the merge queue Sep 17, 2024
Merged via the queue into esp-rs:main with commit a787a13 Sep 17, 2024
27 checks passed
jessebraham pushed a commit to esp-rs/esp-hal-community that referenced this pull request Sep 17, 2024
* Add `esp-hal-buzzer` to drive a piezo-electric buzzer

- Initially submitted in esp-rs/esp-hal#1984

* Add missing dependencies and `.cargo/config.toml`

* Use patched `esp-hal` until new release

* Add check CI check for `esp-hal-buzzer`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants