feat(led): Expand LED API some more #472
Conversation
There was a problem hiding this comment.
Pull Request Overview
A new feature to expand the LED driver API with dynamic frequency control, returning status and logging errors for PWM operations.
- Changed set_duty and set_fade_with_time to return a boolean status and log failures.
- Added get_frequency, set_frequency, and set_pwm methods.
- Extended header docs and added fmt::formatter specializations for logging.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| components/led/src/led.cpp | Updated set_duty to return bool, added error checks/logging, introduced get_frequency, set_frequency, set_pwm, and updated set_fade_with_time. |
| components/led/include/led.hpp | Updated function signatures to return bool, added docs for new APIs, and added fmt::formatter specializations. |
Comments suppressed due to low confidence (3)
components/led/include/led.hpp:149
- [nitpick] The name
set_pwmmay be ambiguous. Consider renaming to something likeset_frequency_and_dutyto clearly convey that both parameters are configured.
bool set_pwm(ledc_channel_t channel, size_t frequency_hz, float duty_percent);
components/led/src/led.cpp:147
- There are no visible unit tests for
get_frequency. Consider adding tests for both managed and unmanaged channels to verify correct optional behavior.
std::optional<size_t> Led::get_frequency(ledc_channel_t channel) const {
components/led/src/led.cpp:175
- New
set_pwmfunctionality should be covered by unit tests for both success and error paths when setting frequency and duty cycle.
bool Led::set_pwm(ledc_channel_t channel, size_t frequency_hz, float duty_percent) {
|
|
||
| /** | ||
| * @brief Set the frequency of the LEDC channel. | ||
| * @note This function will block until until a current fade process |
There was a problem hiding this comment.
Typo detected: the phrase 'until until' is duplicated. Consider revising to 'block until a current fade process completes'.
| * @note This function will block until until a current fade process | |
| * @note This function will block until a current fade process |
Copilot uses AI. Check for mistakes.
|
|
||
| /** | ||
| * @brief Set the PWM frequency and duty cycle for this channel. | ||
| * @note This function will block until until a current fade process |
There was a problem hiding this comment.
Typo detected: the phrase 'until until' is duplicated. Consider revising to 'block until a current fade process completes'.
| * @note This function will block until until a current fade process | |
| * @note This function will block until a current fade process |
Copilot uses AI. Check for mistakes.
| @@ -118,36 +118,117 @@ std::optional<float> Led::get_duty(ledc_channel_t channel) const { | |||
| return (float)raw_duty / (float)max_raw_duty_ * 100.0f; | |||
| } | |||
|
|
|||
| void Led::set_duty(ledc_channel_t channel, float duty_percent) { | |||
| bool Led::set_duty(ledc_channel_t channel, float duty_percent) { | |||
There was a problem hiding this comment.
The semaphore take/give pattern is repeated across multiple methods. Consider extracting this into a RAII helper or utility function to reduce duplication and potential for mismatched calls.
Copilot uses AI. Check for mistakes.
|
✅Static analysis result - no issues found! ✅ |
Description
Motivation and Context
Some boards I have have buzzers, and this seems like a reasonable low-level driver for them.
How has this been tested?
Build and run
led/example, build as part of a buzzer project which will come later.Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.