Make PWM frequency and resolution configurable per endpoint #65
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/per-endpoint-pwm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PwmConfighardcoded 490 Hz and the SoC's max LEDC resolution for everyLevelEndpoint. The bathroom extractor fan needs its own settings, sofrequencyHzandresolutionBitsare now fields onPwmConfig(defaulting to the old values), andLevelEndpoint's single-pin constructor is gone — the bathroom node, its only caller, now builds a fullPwmConfiglike the lighting node does. The extractor runs at 25 kHz/8-bit, above the audible range.Tested with
pio test -e native(new cases intest_pwm_configcover the default and an explicit override) andpio run -e node_bathroom -e node_lighting.Closes CCS-LLA-25.
Nothing blocking. One correctness gap worth a follow-up, rest is nits.
The removed
static_asserthad a job the new code doesn't replace: nothing stopsresolutionBitsexceeding the SoC's LEDC timer width.defaultPwmResolutionBitsis safe by construction, but an explicit value — the bathroom's 8-bit config today, anyone's tomorrow — isn't checked againstSOC_LEDC_TIMER_BIT_WIDE_NUManywhere. Both current configs are within range, so nothing is actually broken, but the failure mode the old comment called out ("asking LEDC for more bits than the timer has fails at runtime and leaves the circuit dark") is back, silently, for any future per-endpoint override. Worth a runtime check inbegin()(#if defined(ARDUINO)) that clamps or assertsresolutionBits <= SOC_LEDC_TIMER_BIT_WIDE_NUMbefore callingledcSetup.Related: in
applyOutput(),pwmMaxisuint32_tbutdutyisuint16_t. IfresolutionBitsever exceeds 16,static_cast<uint16_t>(ease(...) * pwmMax + 0.5f)casts a float outsideuint16_trange — undefined behaviour, not just truncation. Not reachable with today's configs (max 15 bits), but it's the same gap as (1): nothing stops a future config from hitting it, and this path is Arduino-only so UBSan in the native suite never exercises it.Everything else checks out: duty/pwmMax arithmetic is correct for 8-bit and 15-bit resolutions as used today,
defaultPwmResolutionBitsstays host-buildable,PwmConfig's trailing new fields don't disturb the existing positional aggregate inits innode_lighting, the removed single-pin constructor has no other callers, and the spec doesn't mention PWM frequency/resolution so no update is needed there.@ -39,1 +39,4 @@constexpr uint8_t extractorPins[] = {D1};ctrl::PwmConfig extractorConfig = {extractorPins, 0, false, ctrl::Ease::Linear, ctrl::Ease::Linear, 0, 0, 25000, 8};Can the ease function and ease durations be left out from this initialiser? They should be the same as the default values in the declaration
@ -9,1 +14,4 @@// Capped at 15 even on the classic ESP32's 20-bit LEDC, to match the previous generation of this system.#if defined(ARDUINO)constexpr uint8_t defaultPwmResolutionBits = SOC_LEDC_TIMER_BIT_WIDE_NUM < 15 ? SOC_LEDC_TIMER_BIT_WIDE_NUM : 15;Create a ticket to remove this cap. Don't want to test it now, but this is a legacy hang over
Done — extractorConfig now uses a designated initialiser with only
pins,frequencyHzandresolutionBits; fades and ease fall through to the defaults.Ticket raised for removing the 15-bit cap: CCS-NR-29 in Node Runtime.
@ -51,2 +60,4 @@uint16_t fadeInMs = 500;uint16_t fadeOutMs = 1000;uint32_t frequencyHz = 490;Is there a way to set this to the default for this SOC? Then this 490 value should be set in the lighting node, as this is a values specific to that
There's no SoC default for frequency — ESP-IDF's LEDC has none, and the Arduino core's own analogWrite uses 1 kHz — so the runtime default is now 1 kHz, and 490 Hz lives in the lighting node as
pwmFrequencyHz.