Repository navigation
leds: PWM brightness control, persisted, with TX/RX pin-share arbitration - #79
Conversation
5261aab to
268bd01
Compare
|
Thanks for this! Can't merge yet — a few things after rebasing on
Also, please slim it down: one shared PWM/GPIO write helper instead of a copy in |
3db37c9 to
b3dc7c8
Compare
|
Tested point 4 on a T96 repeater built from b3dc7c8 (rebased on dev 9bb7ccf).
|
|
Thanks, much better — prefs layout, PWM config and Two leftovers before I squash-merge:
And one test if you can: the activity LED on a V4.3 repeater build. The T096 result is good, but my worry was the ESP32 boards specifically. |
|
Tested the V4.3 repeater activity LED (point 4), built from 3471e97. Short version: the PWM path on this LED does not drive the physical LED at all, on a board where the same pin works fine over plain GPIO and over a separate Arduino-based firmware's PWM. This looks isolated to Zephyr's ESP32-S3 LEDC driver on this pin, not to this PR's logic, prefs, or power management. Setup: heartbeat-pwm-led and lora-tx-pwm-led both alias pwm_led_white (GPIO35, LEDC channel 0), the same physical white LED as led0's existing plain-GPIO binding. leds.radio=all, leds.hb=all, leds.brightness=100, master switch on. What was ruled out, in order:
So: GPIO35 itself, and the plain-GPIO path on it, both work. Zephyr's LEDC/PWM output on that exact pin produces nothing, with no error anywhere in the stack. That is as far as source reading and rebuilds can take it; confirming the actual cause would need scope-level probing of the LEDC peripheral's registers. One unrelated thing noticed along the way: CLI_HAS_RADIO_LED (CommonCLI.cpp, not touched by this PR) only checks DT_ALIAS(lora_tx_led), never the PWM alias, so "set leds.radio" replies "(no radio LED on this board)" on V4.3 even though the LED exists. Cosmetic text issue, unrelated to the PWM problem above, flagging separately so it does not get read as part of the same bug. |
|
Thanks for the thorough V4.3 test. I don't think it's a Zephyr LEDC bug — it's the pin clash from point 4:
That's why companion works (it never touches Could you skip that Please make the guard alias-based (any board with |
Single LED (P0.28) doubles as heartbeat, message flash, shutdown flash and LoRa TX indicator, all sharing the same physical pin. Adds one PWM channel on that pin (heartbeat-pwm-led/lora-tx-pwm-led aliases) and a single 0-100 brightness knob (get/set led) covering every one of those events at once, deliberately RAM-only: resets to a 10% default on every reboot regardless of CLI changes made in a previous session. The existing persisted on/off gate (set leds on|off) is untouched. Boards without heartbeat-pwm-led/lora-tx-pwm-led aliases keep their current plain-GPIO behavior unchanged.
Reuses the existing leds on|off switch for brightness too, same "one command, wording vs number" convention as led.tx/led.hb on _scratch_leds_test, rather than introducing a separate led command. Drops the legacy '1'/'0' single-char aliases from set leds while at it: same 1%/0% shadowing bug already found and fixed on led.tx/led.hb on 2026-08-16, latent here until a numeric value became meaningful. get leds now reports both: "on N%" / "off (N%)".
Reverts the previous merge: "leds" now only ever parses on/off (a pure word command again, no numeric fallback), and brightness moves to its own "leds.brightness <0-100>" command instead of overloading "leds" with a value that could be either a word or a number. Checked before "leds " in the chain since it shares the same 4-byte prefix. Still RAM-only, no savePrefs() call in the brightness branch. This is the syntax settled on after comparing two options: reuse "leds" via a namespaced brightness command (chosen), vs a full per-LED led.hb/led.tx split like the RAK3401 branch (not chosen, decided to apply the "leds"/"leds.brightness" shape everywhere without exception, including any future RAK3401 rework).
Ports the T096 "Option 1" design (single LED, get/set leds on|off persisted + get/set leds.brightness 0-100 RAM-only) to the Wireless Tracker V2, which has the same single-LED-multi-purpose situation (status_led doubles as heartbeat/message-flash/shutdown-flash/TX indicator, same as T096's P0.28). Adds a new ledc0 PWM channel on GPIO18 (same physical pin as the existing plain-GPIO status_led) with heartbeat-pwm-led/lora-tx-pwm-led aliases -- the shared C++ logic (helpers/ui/ui_common.c, adapters/board/ZephyrBoard.cpp, helpers/led_gate.c/h, helpers/CommonCLI.cpp) already existed from the T096 commits and needed no changes, being board-agnostic behind DT_NODE_EXISTS checks. CONFIG_PWM=y deliberately NOT in the board defconfig: WT2 builds via --sysbuild (mcuboot + app as two images sharing that defconfig), and mcuboot's minimal kernel config has no k_sem_give()/k_sem_take(), which the ESP32 PWM LED driver needs -- CONFIG_PWM=y there links the app image fine but fails mcuboot's link. New boards/common/esp32s3_pwm_led.conf scopes it to the app image only via EXTRA_CONF_FILE, alongside the existing esp32s3_usb.conf.
leds.brightness (PWM-capable boards) was RAM-only since it was added: it always reset to ZEPHCORE_LED_DEFAULT_BRIGHTNESS_PCT (10%) on every boot, regardless of what a user had last set it to. New field led_brightness on NodePrefs, following the exact same append-only pattern as leds_radio_mode/leds_hb_mode added just before it: offset 311 in RepeaterDataStore.cpp (repeater/room-server/observer, shared file), offset 174 in ZephyrDataStore.cpp (companion). Both are backward compatible by construction: a prefs file written before this field existed simply leaves it at the initNodePrefs() default (10%), which is exactly the behaviour every already-deployed node has today -- no special-casing needed to keep "10% on a new/not-yet-upgraded node" as the effective default. CommonCLI.cpp's "set leds.brightness" handler now writes the prefs field and calls savePrefs(), same as the leds.radio/leds.hb handlers right above it; "get leds.brightness" now reads from prefs instead of the RAM getter directly, for the same reason those two already do (always in sync, prefs is the source of truth). The four boot call sites (main_companion.cpp, main_repeater.cpp, main_room_server.cpp, main_observer.cpp) each gained a zephcore_led_set_brightness_pct() call seeding the RAM cache from the persisted value, mirroring the existing leds_radio_mode/leds_hb_mode lines right next to them. Verified building for heltec_t096/nrf52840 (companion) and rak3401_1watt (repeater), the two roles/datastores touched by this change.
The RX-pulse/pin-hold interlock (s_tx_lit/s_rx_pulse_off/ zephcore_led_radio_hold_pin) was gated by HAS_TX_LED alone, so it never compiled in for any PWM-capable board (HAS_TX_LED_PWM). Left two real gaps: - T096 and Wireless Tracker V2 alias heartbeat and TX activity to the SAME physical PWM LED (ZEPHCORE_LED_PIN_SHARED's intent exactly), but had none of the arbitration that protects every plain-GPIO board with the same wiring -- a transmit and a heartbeat tick could still clip each other on the shared PWM channel. - All PWM boards, RAK3401 included, had no onPacketReceived() blink at all: "set leds.radio rx" (or "all") silently did nothing on the activity LED, since that whole code path only existed under HAS_TX_LED. Fixed by widening every "#if HAS_TX_LED" guard around this machinery to "#if HAS_TX_LED_PWM || HAS_TX_LED", and adding a small tx_led_write(bool) helper (mirroring heartbeat_led_write() in helpers/ui/ui_common.c) so the shared interlock logic doesn't need to know which path is compiled in -- only the final write differs. ZEPHCORE_LED_PIN_SHARED itself gained a PWM branch: true when heartbeat-pwm-led and lora-tx-pwm-led alias the same DT node (T096, WT2), same as it already was for the GPIO led0/lora-tx-led case. RAK3401's two PWM LEDs are on distinct nodes, so it correctly stays unshared -- no arbitration overhead there, but it does now get the RX blink like every other PWM board. The heartbeat side (helpers/ui/ui_common.c) needed no change: it already checks zephcore_led_radio_holds_pin() unconditionally through heartbeat_led_write(), from the LED brightness persistence commit earlier today. Verified building for heltec_t096/nrf52840 (shared PWM LED), heltec_wireless_tracker_v2/esp32s3 (shared PWM LED), and rak3401_1watt (two independent PWM LEDs, confirms ZEPHCORE_LED_PIN_SHARED stays 0 there).
heltec_wifi_lora32_v43_procpu.dts already declared a pwm-leds node for its single onboard LED (pwm_led_white, LEDC0/GPIO35, generic pwm-led0 alias) -- it just never had the two project-specific aliases (heartbeat-pwm-led / lora-tx-pwm-led) that helpers/ui/ui_common.c and adapters/board/ZephyrBoard.cpp actually look for. T096 and Wireless Tracker V2 have had this wired for a while; V4.3 was simply never included in that earlier rollout, not blocked by anything in its hardware -- the PWM path was sitting there unused this whole time, confirmed by grepping for any other consumer of pwm-led0/pwm_led_white before touching it (none). Aliased both roles to the same node, same as T096/WT2: this board has only one physical LED, shared between heartbeat and LoRa TX activity. ZEPHCORE_LED_PIN_SHARED and the RX-pulse/pin-hold arbitration (extended to PWM boards earlier today, commit a57d4c7) apply automatically since both aliases point at the same node. Gives V4.3 the full leds.radio/leds.hb/leds.brightness set for the first time: leds.radio previously had no effect at all on this board (no activity LED aliased whatsoever), leds.brightness was accepted but inert (no PWM-capable LED reachable). Verified building for heltec_wifi_lora32_v43/esp32s3 with boards/common/esp32s3_pwm_led.conf (same flag already used for WT2).
ZEPHCORE_LED_DEFAULT_BRIGHTNESS_PCT was 10 on the branch this was developed on (a local preference for that project's own compiles). For general use it should default to what every PWM LED already ran at before this setting existed: full brightness. Also fixes a stale led_gate.h comment that still said "RAM-only, not persisted" after brightness became a persisted NodePrefs field.
…yout companionPrefsEncode()/serverPrefsEncode() never wrote the led_brightness byte that companionPrefsDecode()/serverPrefsDecode() already read, desyncing every field that follows it by one byte on the companion side (wifi_enabled/wifi_ssid/wifi_pwd) and silently truncating the server format by one byte. Caught by CI's ASan/UBSan run on the host regression suite: an out-of-bounds read/write in the pinned layout tests. Since led_brightness never existed upstream, nothing already accounted for it: fixing only the encoder still leaves the pinned byte tables and the documented 272/311-byte totals (COMPANION_PREFS_SIZE/ SERVER_PREFS_SIZE, used by the real prefs stores too, not just tests) one byte short of what the encoder now actually produces. - PrefsCodec.h: bump both size constants (272->273, 311->312). - PrefsCodec.cpp: add the missing w.put() in both encoders, mirroring the position already used by both decoders. - prefs_codec_tests.cpp: add led_brightness to both layout tables at its real file offset (shifts wifi_enabled/wifi_ssid/wifi_pwd by one byte on the companion side; server's is appended last, nothing else shifts), extend both historical-boundary length lists with the new full-format length, and give it a non-default value in sample() so the layout/roundtrip tests actually exercise it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
UNIT-PREFS-001 and UNIT-PREFS-006 still said 272/311-byte in catalog.json while prefs_codec_tests.cpp was bumped to 273/312 by f8a4992. Same ID sets on both sides (missing=[], unexpected=[]) but differing names made the executable/catalog dict comparison fail, which is exactly what CI's host regression job flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
led_brightness sat at companion offset 174, in front of the WiFi fields, so a 272-byte file written by upstream dev decoded with the wrong led_brightness and a shifted SSID. Append it after wifi_pwd (offset 272) and restore the original test offsets and the 208/272 historical lengths. WT2 and V4.3 only got CONFIG_PWM through esp32s3_pwm_led.conf, which the release build never passes, so release links would fail. Set CONFIG_PWM=y in each board.conf instead. board.conf is app-only and mcuboot does not inherit it, so the mcuboot link failure that rules out the defconfig does not apply. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The on/off-only parser dropped "default", "1" and "0" from set leds and reverted the shared cliOnOff() helper that the other toggles use. Only the leds.brightness handling needed to change; the master switch goes back to upstream's form. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The heartbeat and activity LED each computed the PWM pulse inline. zephcore_led_pwm_write() in led_gate.c now does it once. Comments added for the LED work described branch history, dates and release notes; keep only what the code needs. led_gate.c still said the brightness was RAM-only, which is no longer true, and the server decode comment gave the old 10% default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CONFIG_PWM now comes from each board.conf (see "prefs: move led_brightness after wifi_pwd; enable PWM in board.conf"), so the EXTRA_CONF_FILE fragment is no longer referenced anywhere and can go. The WT2 PWM LED node comment still said "Option 1" and "RAM-only", both stale: the design no longer needs that internal name, and the brightness has been persisted since the earlier commit in this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
server_main_common.cpp and main_observer.cpp both configured led0 as a plain GPIO output at boot, unconditionally. On ESP32 that calls esp_rom_gpio_matrix_out() under the hood, which routes the pin's output to the GPIO signal and takes it away from whatever peripheral had it before, including the LEDC channel a heartbeat-pwm-led or lora-tx-pwm-led alias had already claimed during the PWM driver's own init. The PWM write then succeeds (no error, no warning) but never reaches the pin: confirmed on V4.3 hardware, where the same pin lights up fine over plain GPIO, and over PWM in a different (non-Zephyr) firmware, but stays dark over Zephyr's PWM once this GPIO configure has run. Companion builds never hit this (they don't use server_main_common.cpp) and nRF52 isn't affected (no shared GPIO matrix), which is why T096 tested fine while V4.3 didn't. Fix is alias-based, not a board list, so any future board with either PWM alias is covered without extra wiring. Also: CLI_HAS_RADIO_LED only checked the plain lora-tx-led alias, so "set leds.radio" replied "(no radio LED on this board)" on boards whose radio LED exists only via lora-tx-pwm-led (V4.3). Check both. Documents the two PWM aliases in the board porting guide, and leds.brightness plus the corrected LED topology counts in docs/CLI_commands.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
93da13a to
6b9d205
Compare
|
Fix pushed and confirmed on hardware, both boards. Since rebased onto dev (now 6b9d205), CI green. V4.3 repeater: heartbeat visible, brightness change visible at leds.brightness=10, clear flash on advert, confirmed after a reboot. WT2 repeater (first hardware test of this PR on this board): heartbeat visible, longer flash on advert, brightness=5 read back correctly after an explicit reboot. CLI_HAS_RADIO_LED, the two PWM aliases in the board porting guide, and leds.brightness in docs/CLI_commands.md are all in the same commit. |
2 participants
What
Three related fixes/features for the PWM-capable heartbeat/TX LED (T096,
Wireless Tracker V2, RAK3401, and now V4.3), bundled together because the
later commits depend on the earlier ones:
set/get leds.brightness <0-100>for PWM-capable boards (alreadypresent locally for T096 and Wireless Tracker V2; this PR also wires
V4.3's existing-but-unused PWM LED node into the same mechanism, giving
it
leds.radio/leds.hb/leds.brightnessfor the first time -- it hadnone of the three before).
RAM-only, silently reset to the compiled default every boot).
interlock (prevents a transmit and a heartbeat tick from clipping each
other on a shared pin) was gated by
HAS_TX_LEDalone, so it nevercompiled in for
HAS_TX_LED_PWMboards. Two real gaps fixed:same physical PWM LED -- exactly
ZEPHCORE_LED_PIN_SHARED'sintended case -- but had no arbitration at all.
set leds.radio rx(orall) silently did nothing on the activity LED,since that whole code path only existed under plain
HAS_TX_LED.Default brightness is 100% (matches what every PWM LED already ran at
before this setting existed).
Testing
Compiled clean on
heltec_t096/nrf52840,heltec_wireless_tracker_v2/esp32s3,heltec_wifi_lora32_v43/esp32s3, andrak3401_1watt/nrf52840(companionrole; RAK3401 confirms
ZEPHCORE_LED_PIN_SHAREDcorrectly stays 0 there,since its two PWM LEDs are on distinct pins).
Flashed and tested on real hardware: T096 (persistence survives reboot,
heartbeat/TX arbitration confirmed visually) and V4.3 (new PWM wiring,
leds.radio/leds.hb/leds.brightnessall functional for the first timeon this board).
Not included
An on-screen settings menu for brightness (button-UI boards) exists on top
of this locally and works well, but it's a separate, more opinionated UI/UX
change -- happy to open it as a follow-up PR if there's interest, once this
one settles.