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

Fix for issue #1419 #1441

Merged
merged 5 commits into from
Apr 22, 2024
Merged

Fix for issue #1419 #1441

merged 5 commits into from
Apr 22, 2024

Conversation

Droog71
Copy link
Contributor

@Droog71 Droog71 commented Apr 16, 2024

Submission Checklist 📝

  • [x ] I have updated existing examples or added new ones (if applicable).
  • [ x] My changes were added to the CHANGELOG.md in the proper section.

Extra:

Pull Request Details 📖

Description

Fix for issue #1419. Removed ESP32 specific code for resolutions > 16 bit in ledc embedded_hal::pwm max_duty_cycle function. Fixed division by zero in ledc embedded_hal::pwm set_duty_cycle function and converted to set_duty_hw instead of set_duty to eliminate loss of granularity.

Testing

Wokwi (https://wokwi.com/)

Droog71 added 2 commits April 15, 2024 23:10
…s > 16 bit in ledc embedded_hal::pwm max_duty_cycle function. Fixed division by zero in ledc embedded_hal::pwm set_duty_cycle function and converted to set_duty_hw instead of set_duty to eliminate loss of granularity.
@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 16, 2024

FYI #1442 broke LEDC for ESP32 - not related to this PR

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 16, 2024

This looks good overall. Just needs Clippy and RustFmt issues addressed.
#1442 should be fixed in another PR - it's not really related to this

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 17, 2024

Seems like Clippy isn't happy about an unused import: https://github.com/esp-rs/esp-hal/actions/runs/8707574081/job/23882855215?pr=1441

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

Code LGTM and example seems to be working fine. It needs rebasing and fixing clippy warnings to make CI happy, though :)

@Droog71
Copy link
Contributor Author

Droog71 commented Apr 18, 2024

Removed unused ChannelIFace import for ehal mod in ledc channel.rs
Looks like all checks are passing now.

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.

Thanks for the fix

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@MabezDev MabezDev added this pull request to the merge queue Apr 22, 2024
Merged via the queue into esp-rs:main with commit 7e5895c Apr 22, 2024
21 checks passed
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