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 HardwarePWM class for Rp2040 and update Basic PWM sample #2908

Merged
merged 12 commits into from
Nov 5, 2024

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Oct 31, 2024

The Basic_HwPWM sample should work for all architectures. Esp32 is fixed but Rp2040 requires an implementation so may as well do that here.

Note that as with the Esp32, the Rp2040 PWM hardware is much more capable than the HardwarePWM class allows, but it is an easy way to port existing code and get started.

TODO:

  • Come up with suitable list of pins for testing. Probably needs a different list for each chip: LED pin will only be best-guess as massive variation with dev-boards.
  • Provide Rp2040 hardware PWM implementation and verify on hardware

@mikee47 mikee47 marked this pull request as draft October 31, 2024 20:21
@mikee47 mikee47 force-pushed the feature/basic-hwpwm branch from ea80d94 to 26c5cdd Compare October 31, 2024 20:28
@SmingHub SmingHub deleted a comment from what-the-diff bot Nov 3, 2024
@slaff slaff added this to the 6.0.0 milestone Nov 4, 2024
@mikee47 mikee47 force-pushed the feature/basic-hwpwm branch from 71b25a9 to ec644fc Compare November 4, 2024 11:54
@mikee47 mikee47 changed the title Update Basic PWM sample Implement HardwarePWM class for Rp2040 and update Basic PWM sample Nov 4, 2024
@mikee47 mikee47 marked this pull request as ready for review November 4, 2024 12:23
@mikee47
Copy link
Contributor Author

mikee47 commented Nov 4, 2024

@pljakobs This PR updates the Basic_HwPWM sample which I know you were intending to do, but wanted to get Rp2040 support in there first. Would you have a look and let me know if you'd like any changes?

// Not implemented
}

uint32_t HardwarePWM::getFrequency(uint8_t pin) const
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I note we can get the frequency for an individual pin, added for esp32 support I believe, but no corresponding setFrequency or setPeriod for a pin. We should either do that or get rid of the pin parameter I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

the way it's currently implemented for the esp32 is that the frequency is global, not per pin.
While the esp32 platform can set the frequency per timer (of which it has up to eight, depending on the actual Soc), timers are mapped to Pins and there can be more pins than timers.
Getting the frequency for a sigle pin, with the current implementation, makes indeed no sense.
I think this is something that should be part of an extended implementation that exposes the better hardware of the esp32 and rp2040 platforms.

On the BasicHWPwm, I have something that works a bit better for me. I'll clean it up and push it in the next days (I'm travelling right now)

@slaff
Copy link
Contributor

slaff commented Nov 5, 2024

@mikee47 Is this PR ready for merging?

@pljakobs
Copy link
Contributor

pljakobs commented Nov 5, 2024

@mikee47 Is this PR ready for merging?

I have no additional edits right now.

@mikee47
Copy link
Contributor Author

mikee47 commented Nov 5, 2024

Yes, I'm all done thanks.

@slaff slaff merged commit 6d94935 into SmingHub:develop Nov 5, 2024
32 checks passed
@mikee47 mikee47 deleted the feature/basic-hwpwm branch November 5, 2024 20:07
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.

3 participants