Add PM1 PWM control and use it for the ToughC5 buzzer - #318
Conversation
The PM1 exposes two PWM channels, on GPIO3 and GPIO4. Follow the naming and the units of the standalone M5PM1 driver so that code can move between the two without surprises: setPwmDuty takes percent, setPwmDuty12bit takes the raw 12-bit value, and the two boolean arguments are ordered polarity then enable. The return value is bool for the I2C result, matching the rest of this class. Both channels share a single frequency register, so changing it affects a channel that is already running as well.
There was a problem hiding this comment.
Pull request overview
This pull request adds PM1 PWM register access to M5PM1_Class and uses it during ToughC5 board initialization to ensure the PM1-driven buzzer line (GPIO4 / PWM ch1) is placed into a known-safe state at boot.
Changes:
- Added PM1 PWM channel enum plus public APIs to set shared PWM frequency and per-channel duty (percent and raw 12-bit).
- Implemented PM1 PWM register writes for frequency, duty, polarity, and enable.
- Updated ToughC5 power-init to disable PM1 PWM ch1 at startup and configure GPIO4 for the PWM “special” function.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/utility/power/M5PM1_Class.hpp | Adds PWM channel enum and public PWM frequency/duty APIs to the PM1 driver interface. |
| src/utility/power/M5PM1_Class.cpp | Implements PWM register writes (frequency + per-channel duty with enable/polarity flags). |
| src/utility/Power_Class.cpp | Initializes ToughC5 buzzer pin via PM1 (disable PWM at boot, normalize GPIO4, then select PWM function). |
Suppressed comments (1)
src/utility/power/M5PM1_Class.hpp:141
- Same concern as
setPwmDuty:setPwmDuty12bittakes twoboolparameters ordered(polarity, enable), which differs fromM5IOE1_Class::setPwmDuty’s(enable, polarity)ordering. With identical types, this is easy to misuse at call sites; consider aligning the order or using non-boolean types for clarity.
bool setPwmDuty12bit(pwm_channel_t channel, std::uint16_t duty12, bool polarity = false, bool enable = true);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| M5pm1.setPwmDuty12bit(M5PM1_Class::pwm_ch1, 0, false, false); | ||
| M5pm1.setGPIODrive(M5PM1_Class::gpio4, M5PM1_Class::push_pull); | ||
| M5pm1.setGPIOPull(M5PM1_Class::gpio4, M5PM1_Class::pull_none); | ||
| M5pm1.setGPIOOutput(M5PM1_Class::gpio4, false); | ||
| M5pm1.setGPIOMode(M5PM1_Class::gpio4, M5PM1_Class::output); | ||
| M5pm1.setGPIOFunction(M5PM1_Class::gpio4, M5PM1_Class::special); |
There was a problem hiding this comment.
Good catch, this was a real hole. Fixed.
The PWM-off write is now retried a few times and its result gates the function
switch: special is only selected once the channel is confirmed off.
One addition to the suggestion. Leaving the function unchanged is not enough on
its own, because the PM1 keeps its pin mux across an ESP reset and may already
be in the PWM function, in which case doing nothing leaves it sounding. The
fallback therefore writes the plain GPIO function explicitly, with the output
latch already cleared and the pin an output, so it is silent whatever PWM state
was retained. That loses the buzzer until the next boot, which is the right
direction for a step whose purpose is to guarantee silence, and the failure is
logged.
| /// @param duty duty cycle in percent (0-100). | ||
| /// @param polarity false=normal / true=inverted. | ||
| /// @param enable true=enable / false=disable. | ||
| bool setPwmDuty(pwm_channel_t channel, std::uint8_t duty, bool polarity = false, bool enable = true); |
There was a problem hiding this comment.
The order follows the standalone drivers for these chips rather than this
repository. Both m5stack/M5PM1 and m5stack/M5IOE1 declare
setPwmDuty(channel, duty, polarity, enable) with duty in percent, plus a
separate setPwmDuty12bit for the raw value, and this change matches them.
M5IOE1_Class here is the one that differs, and not only in the boolean order:
its setPwmDuty takes the raw 12-bit value under the name both standalone
drivers use for percent. Reading the M5IOE1 documentation and writing
setPwmDuty(ch, 50, false, true) against it compiles and silently means
"duty 50/4095, disabled". Aligning with it would move away from both standalone
drivers rather than towards consistency, so the mismatch is better resolved on
that side.
The concern about two adjacent booleans is fair, and typed flags would be an
improvement over both conventions. That is worth doing across the existing PWM
APIs together rather than introducing a third convention here.
The ToughC5 buzzer is driven by PM1 PWM channel 1, so GPIO4 has to be set up for that function before an application can drive it at all. The PM1 keeps running across ESP resets and retains its PWM state, so the channel is also put off at boot, the same way the vibration motor is on the StopWatch. GPIO4 is normalized in an order that does not drive the buzzer on the way: the output latch is cleared before the pin becomes an output, and the PWM function is selected last. Selecting that function is also what would make a retained duty audible again, so it is only done once the channel is confirmed off. The write is retried a few times, and if it still cannot be confirmed the pin is left as a plain output driving low, which is silent whatever the retained PWM state is. Failing closed loses the buzzer until the next boot, which is the better direction for a step whose purpose is to guarantee silence. Stopping the channel on the way into sleep is deliberately left to the application. The PM1 stays powered while the ESP sleeps, so the application may intend the PWM output to remain active; whether to stop it is application policy rather than board initialization.
b787fda to
388d61c
Compare
Problem
The PM1 exposes two PWM channels but
M5PM1_Classhad no way to reach them.On the ToughC5 this leaves the buzzer unusable: it is driven by PM1 PWM
channel 1, and nothing configures GPIO4 for that function. The PM1 also keeps
running across ESP resets and retains its PWM state, so a firmware that leaves
the channel enabled hands a sounding buzzer to whatever boots next.
Fix
Add PWM control to M5PM1_Classadds the frequency and duty setters. Thenaming and the units follow the standalone M5PM1 driver so code can move
between the two:
setPwmDutytakes percent,setPwmDuty12bittakes the raw12-bit value, and the boolean arguments are ordered polarity then enable. The
return value is
boolfor the I2C result, matching the rest of the class.Both channels share one frequency register, which the doc comment states.
Configure the ToughC5 buzzer line at startupsets the pin up. The channel isput off at boot, the same way the vibration motor is on the StopWatch, and the
output latch is cleared before GPIO4 becomes an output so the pin never drives
the buzzer directly while the PWM function is being selected.
Stopping the channel before sleep is deliberately left to the application, in
line with how the StopWatch motor is handled: the PM1 stays powered while the
ESP sleeps, so the output may be meant to keep running.
The second commit sits inside the ESP32-C5 target guard and a board check, so
other boards are untouched.
Verification
Built for ESP32-C5 and for ESP32, and the first commit builds on its own.
On ToughC5 hardware: tones across the audible range, which also confirmed the
frequency register takes a plain value in Hz. The startup shutdown was checked
by leaving the buzzer sounding, flashing over it while it sounded, and
confirming it went quiet once the new firmware reached
begin().