From 1633274e4f8beddb817e6add9be8eebf84244931 Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:34:12 +0200 Subject: [PATCH] leds refactor/improvement --- docs/Repeater_CLI_commands.md | 27 +++++ releasenotes/RELEASE_NOTES_1.17.4-zephcore.md | 72 ++++++++++++ zephcore/adapters/board/ZephyrBoard.cpp | 103 ++++++++++++++++-- zephcore/adapters/board/ZephyrBoard.h | 1 + .../adapters/datastore/ZephyrDataStore.cpp | 17 ++- zephcore/adapters/radio/LoRaRadioBase.cpp | 6 + zephcore/app/RepeaterDataStore.cpp | 9 ++ zephcore/app/main_observer.cpp | 2 + zephcore/helpers/CommonCLI.cpp | 90 +++++++++++++++ zephcore/helpers/NodePrefs.h | 16 +++ zephcore/helpers/led_gate.c | 40 +++++++ zephcore/helpers/led_gate.h | 63 +++++++++++ zephcore/helpers/ui/ui_common.c | 50 ++++++++- zephcore/include/mesh/Board.h | 5 + zephcore/src/main_companion.cpp | 2 + zephcore/src/main_repeater.cpp | 29 +++-- zephcore/src/main_room_server.cpp | 29 +++-- 17 files changed, 530 insertions(+), 31 deletions(-) diff --git a/docs/Repeater_CLI_commands.md b/docs/Repeater_CLI_commands.md index 4a04dd1..51f1186 100644 --- a/docs/Repeater_CLI_commands.md +++ b/docs/Repeater_CLI_commands.md @@ -264,6 +264,8 @@ All `set uplink.*` changes are saved immediately and only applied after reboot. | `get owner.info` | Owner/contact info (pipes `\|` display as newlines) | | `get int.thresh` | Interference threshold | | `get leds` | LED master switch: `on` or `off` | +| `get leds.radio` | Activity-LED mode: `tx`, `rx`, `all` or `off`. Appends `(no radio LED on this board)` where the board has no `lora-tx-led` alias | +| `get leds.hb` | Heartbeat-LED mode: `all`, `hb`, `unread` or `off`. Appends `(no heartbeat LED on this board)` where the board has neither `led0` nor `led1` | | `get buzzer` | *(room server only)* Buzzer/vibration mode as ` ()`: `0 (silent)`, `1 (sound+vib)`, `2 (vibrate)`, `3 (sound)`. Compiled out on repeater builds (`#ifndef ZEPHCORE_REPEATER`) — a repeater answers `unknown config: buzzer`. | | `get agc.reset.interval` | Removed — replies `Removed - Automatic AGC reset is on`. Periodic AGC recalibration was deleted (it reset the noise floor to its unseeded sentinel on every fire). Use `set rxduty` to cut RX current. | | `get multi.acks` | Extra ACK transmit count (`0` or `1`) | @@ -337,6 +339,8 @@ four radio parameters together, since they are one interop-critical set. | `set int.thresh ` | | Interference detection threshold | | `set buzzer <0\|1\|2\|3>` | or `off` / `on` / `vibrate` / `sound` | *(room server only)* `0`/`off` silent, `1`/`on` sound + vibration, `2`/`vibrate` vibration only, `3`/`sound` sound only. Modes 2 and 3 need a vibration motor; without one the node replies `Error: no vibration motor on this board - use 0 or 1`. Applied live and persisted. Compiled out on repeater builds. | | `set leds ` | default **on** | Master switch for every LED on the node, applied live and persisted: heartbeat, unread-message and LoRa TX-activity LEDs, plus the message and shutdown flashes. Works on every role, including headless repeaters where the TX LED is the only one that ever lights. Does **not** cover the display backlight, which is a separate UI brightness setting. | +| `set leds.radio ` | default **tx** | What the LoRa activity LED reacts to, applied live and persisted. `tx` lights it for the duration of each transmit (the behaviour before this setting existed), `rx` gives a 30 ms blink per valid packet received, `all` does both, `off` keeps it dark. Sits **below** `set leds` — the master switch off keeps it dark whatever this says. Only boards defining the `lora-tx-led` alias have this LED; elsewhere the value is stored but does nothing, and the reply says so. | +| `set leds.hb ` | default **all** | What the heartbeat LED reacts to, applied live and persisted. `all` is the 4 s liveness tick that widens from 20 ms to 200 ms while messages are unread (the behaviour before this setting existed), `hb` never widens, `unread` stays dark until there are unread messages, `off` keeps it dark. Also sits below `set leds`. See the LED-topology notes below for what this does on single-LED boards. | | `set agc.reset.interval ` | Accepted, ignored | Removed — replies `Removed - Automatic AGC reset is on`. The prefs byte is still read and written so the on-flash layout stays byte-exact, but nothing acts on it. | | `set multi.acks <0\|1>` | | Enable extra ACK transmits | | `set path.hash.mode ` | 0, 1, or 2 | Path hashing algorithm | @@ -363,6 +367,29 @@ four radio parameters together, since they are one interop-critical set. ## Notes +- **LED topology differs by board, and it changes what `leds.radio` / `leds.hb` can do.** Two DT aliases + decide it: `lora-tx-led` drives the radio activity LED, and `led0` (falling back to `led1`) drives the + heartbeat. Of the 35 boards in tree: + - **Separate pins (6)** — `rak4631`, `sensecap_solar`, `thinknode_m1`, `thinknode_m6`, `lilygo_techo`, + `xiao_nrf52840`. Both settings are fully independent. `xiao_nrf52840` has three LEDs: blue heartbeat, + green unread, red radio. + - **One shared pin (8)** — `heltec_t114`, `heltec_t096`, `heltec_wireless_tracker_v2`, `ttgo_tbeam`, + `gat562_30s`, `rak3401_1watt`, `rak_wismesh_tag`, `lilygo_timpulse_plus`. Here `lora-tx-led` **is** the + heartbeat pin, so both settings drive one LED. Radio activity takes priority and the heartbeat yields + while the radio holds the pin, so a transmit is never blanked mid-packet by the heartbeat's off-timer + (and vice versa). Use `set leds.hb off` for an unambiguous radio indicator. + - **No radio LED (20)** — including `wio_tracker_l1`, `t1000_e`, `meshtracker_x1`, Heltec V3/V4/V43 and + both ProMicros. `set leds.radio` is accepted and stored (so the setting survives onto a board that does + have the LED) but does nothing; the reply says `(no radio LED on this board)`. + - **No heartbeat LED (1)** — `lilygo_t3s3` has `lora-tx-led` but neither `led0` nor `led1`, so `set leds.hb` + is stored and inert, and says so. +- **`unread` needs two things the board may not have.** The second LED lights only where `led0` **and** `led1` + both exist **and** the build is not a repeater; elsewhere unread indication degrades to the heartbeat pulse + widening from 20 ms to 200 ms on the single LED. More importantly the unread count comes from the + **button UI only** — the joystick UI and every repeater/observer report 0 permanently. On those, + `leds.hb all` behaves identically to `hb`, and `leds.hb unread` leaves the LED **dark forever**. +- **`set leds off` still wins.** Both `leds.radio` and `leds.hb` sit below the master switch; with `leds off` + the node stays dark whatever they say. All three are applied live and persisted. - **USB-only commands** — `get acl`, `get prv.key`, `set freq`, `log` (dump), `stats-packets`, `stats-radio`, `stats-core`, `erase` — are blocked when the command arrives over the mesh (remote admin). These are the only ones gated on `sender_timestamp == 0`; `get public.key` and `set prv.key` are **not** among them. - **Adaptive contention window** — `txdelay`, `rxdelay`, and `direct.txdelay` are accepted and stored for Arduino prefs compatibility but have no effect. Use `get txdelay` to inspect the current adaptive state and `set backoff.multiplier` to tune reactive backoff. - **Region load mode** — after `region load`, every line received is parsed as a region entry until a blank line is sent. The loaded map is only committed to the live region tree at that point; use `region save` to persist it. Region rows must be indented by at least one space, so an **unindented line that starts with a name character aborts the mode and is executed as a normal command** — the escape hatch if a `region load` is started by accident or a client dies mid-transfer. An abort discards the partial map, leaving the live region tree untouched. The exported wildcard header line `*` stays unindented and is ignored as before, so pasting the output of `region` still loads cleanly. diff --git a/releasenotes/RELEASE_NOTES_1.17.4-zephcore.md b/releasenotes/RELEASE_NOTES_1.17.4-zephcore.md index a78dc32..0136f55 100644 --- a/releasenotes/RELEASE_NOTES_1.17.4-zephcore.md +++ b/releasenotes/RELEASE_NOTES_1.17.4-zephcore.md @@ -206,6 +206,78 @@ now means zero, and a genuinely absent setting still arrives as 0.2. --- +## The LEDs are yours now + +Until now the node's LEDs were all-or-nothing: `set leds off` killed every one of them, and that was the +only choice on offer. Two new settings sit underneath that master switch and say *what* each LED reacts to. + +**`set leds.radio `** — the LoRa activity LED. + +- `tx` — lit for the duration of each transmit. This is what it has always done, and stays the default. +- `rx` — a short blink for each packet received. +- `all` — both. +- `off` — dark. + +`rx` is the interesting one. It turns the node into a passive activity monitor: you can see at a glance +whether a repeater is hearing anything at all, which is otherwise a question you can only answer by +plugging into the console. The blink fires on valid packets only, so it means "a real packet landed" and +not "there was noise on the channel". + +**`set leds.hb `** — the heartbeat LED. + +- `all` — the 4-second liveness tick, widening from a 20 ms flicker to a 200 ms pulse while you have unread + messages. Unchanged, and still the default. +- `hb` — liveness only. Never widens, never signals unread. +- `unread` — the reverse: dark until something is waiting for you, then the long pulse. A quiet node that + only speaks up when it has something to say. +- `off` — dark. + +Both are applied the moment you set them and survive a reboot. `set leds off` still overrides both, so if +you have a node you want dark, one command is still all it takes. + +> [!NOTE] +> **Upgrading changes nothing.** Both settings default to exactly what your node does today, so an upgrade +> looks identical until you change something. + +### What your board can actually do + +This depends on how many LEDs the board has, and it varies more than you might expect. + +Six boards — RAK4631, SenseCAP Solar, ThinkNode M1 and M6, LilyGo T-Echo and XIAO nRF52840 — wire the +heartbeat and the radio to different LEDs, so the two settings are completely independent. The XIAO has a +third for unread messages. + +Eight boards — Heltec T114, T096 and Wireless Tracker v2, TTGO T-Beam, GAT562, RAK3401, RAK WisMesh Tag and +LilyGo T-Impulse Plus — drive both from a **single** LED. They still work, and radio activity now takes +priority so a transmit is no longer cut short by the heartbeat's timer firing underneath it. But one LED +showing two things is hard to read, so `set leds.hb off` is worth considering there. + +Twenty boards have no radio LED at all, including the Wio Tracker L1, T1000-E, MeshTracker X1 and the +Heltec V3/V4/V43 family. `set leds.radio` is still accepted and remembered — so the setting follows your +node if you move it to hardware that does have one — but it does nothing, and the reply tells you so. + +> [!NOTE] +> **`unread` needs a companion with buttons.** The unread count only exists in the button UI. On repeaters, +> observers and joystick-UI boards it is permanently zero, which means `leds.hb unread` leaves the LED dark +> for good and `leds.hb all` behaves the same as `hb`. Use `hb` or `off` on those. + +--- + +## A repeater told to turn its LEDs off turned them back on + +On a repeater or room server, `set leds off` worked — until the next reboot, when the LEDs started blinking +again. `get leds` still answered `off`, so the node insisted it was doing what you asked while visibly not +doing it, and the only fix was to issue `set leds off` a second time. + +The setting was being applied about forty lines before the saved settings were read from flash, so it was +always acting on the factory default rather than on yours. The stored value was never wrong and never lost; +it just arrived too late to be used. + +This is why it looked intermittent rather than broken: it took a reboot to show up, and a solar-powered node +picks its own moments to reboot. Companions and observers were never affected. + +--- + ## Also in this release - **ESP32-S3 companions now connect over USB.** Heltec V4 and V43, Wireless Tracker v2, XIAO S3, diff --git a/zephcore/adapters/board/ZephyrBoard.cpp b/zephcore/adapters/board/ZephyrBoard.cpp index be208d9..95aa504 100644 --- a/zephcore/adapters/board/ZephyrBoard.cpp +++ b/zephcore/adapters/board/ZephyrBoard.cpp @@ -5,6 +5,7 @@ #include "ZephyrBoard.h" #include "battery_curve.h" #include "led_gate.h" +#include /* LEDS_RADIO_* mode values */ #include #include #include @@ -81,7 +82,9 @@ #endif #endif -/* LoRa TX activity LED (optional — defined per-board via DT alias) */ +/* LoRa radio activity LED (optional — defined per-board via DT alias). Still + * called tx_led after "set leds.radio" gave it an RX mode, because the DT alias + * it comes from is named lora-tx-led on every board that has one. */ #if DT_NODE_EXISTS(DT_ALIAS(lora_tx_led)) static const struct gpio_dt_spec tx_led = GPIO_DT_SPEC_GET(DT_ALIAS(lora_tx_led), gpios); @@ -90,6 +93,22 @@ static const struct gpio_dt_spec tx_led = #define HAS_TX_LED 0 #endif +/* True when this board wires the activity LED to the same pin as the heartbeat + * (8 of the supported boards do). Only those pay for the arbitration hold in + * led_gate.c — everywhere else the two LEDs are independent and the calls + * compile out. led1 is checked as well because ui_common.c falls back to it + * when a board has no led0. */ +#if HAS_TX_LED && DT_NODE_EXISTS(DT_ALIAS(led0)) && \ + DT_SAME_NODE(DT_ALIAS(led0), DT_ALIAS(lora_tx_led)) +#define ZEPHCORE_LED_PIN_SHARED 1 +#elif HAS_TX_LED && !DT_NODE_EXISTS(DT_ALIAS(led0)) && \ + DT_NODE_EXISTS(DT_ALIAS(led1)) && \ + DT_SAME_NODE(DT_ALIAS(led1), DT_ALIAS(lora_tx_led)) +#define ZEPHCORE_LED_PIN_SHARED 1 +#else +#define ZEPHCORE_LED_PIN_SHARED 0 +#endif + #include LOG_MODULE_REGISTER(zephcore_board, CONFIG_ZEPHCORE_BOARD_LOG_LEVEL); @@ -139,13 +158,44 @@ static const struct device *const fuel_gauge_dev = #define HAS_FUEL_GAUGE 0 #endif -/* Initialize TX LED GPIO at boot */ +/* Initialize activity LED GPIO at boot */ #if HAS_TX_LED +/* Width of the receive blink. A transmit holds the LED for its whole airtime, + * but a receive is a single edge — the packet is over by the time the driver + * hands it up — so RX has to be a fixed one-shot. 30 ms is deliberately longer + * than the heartbeat's 20 ms tick so the two read differently on the boards + * that share one pin, and short enough that a busy channel gives a flicker + * rather than a solid glow. */ +#define RX_PULSE_MS 30 + +/* Set only while a transmit is actually holding the LED lit. It is the + * interlock that keeps an RX one-shot from clearing a pin that TX still owns: + * the two are driven from different threads (radio TX path vs system work + * queue), so without it a pulse landing mid-transmit would blank the LED for + * the rest of the packet. */ +static atomic_t s_tx_lit; +static struct k_work_delayable s_rx_pulse_off; + +static void rx_pulse_off_handler(struct k_work *work) +{ + ARG_UNUSED(work); + /* Leave the pin alone if a transmit started while this pulse was in + * flight — onAfterTransmit() owns clearing it in that case. */ + if (atomic_get(&s_tx_lit)) { + return; + } + gpio_pin_set_dt(&tx_led, 0); +#if ZEPHCORE_LED_PIN_SHARED + zephcore_led_radio_hold_pin(false); +#endif +} + static int tx_led_init(void) { if (gpio_is_ready_dt(&tx_led)) { gpio_pin_configure_dt(&tx_led, GPIO_OUTPUT_INACTIVE); } + k_work_init_delayable(&s_rx_pulse_off, rx_pulse_off_handler); return 0; } SYS_INIT(tx_led_init, APPLICATION, 90); @@ -295,11 +345,21 @@ const char *ZephyrBoard::getManufacturerName() const void ZephyrBoard::onBeforeTransmit() { #if HAS_TX_LED - /* Honour the LED master gate ("set leds off"). On a headless repeater this - * is the only LED that ever lights, so the gate has to be checked here and - * not just in the UI layer. onAfterTransmit() still clears the pin - * unconditionally, so a gate flipped mid-transmit can't strand it lit. */ - if (!zephcore_leds_disabled()) { + /* Honour the LED master gate ("set leds off") and then the activity mode + * ("set leds.radio"). On a headless repeater this is the only LED that ever + * lights, so both have to be checked here and not just in the UI layer. + * onAfterTransmit() still clears the pin unconditionally, so a gate or mode + * flipped mid-transmit can't strand it lit. */ + uint8_t mode = zephcore_leds_radio_mode(); + if (!zephcore_leds_disabled() && + (mode == LEDS_RADIO_TX || mode == LEDS_RADIO_ALL)) { + /* A receive blink may still be in flight; take the pin from it so its + * handler doesn't clear the LED partway through this transmit. */ + k_work_cancel_delayable(&s_rx_pulse_off); + atomic_set(&s_tx_lit, 1); +#if ZEPHCORE_LED_PIN_SHARED + zephcore_led_radio_hold_pin(true); +#endif gpio_pin_set_dt(&tx_led, 1); } #endif @@ -308,7 +368,36 @@ void ZephyrBoard::onBeforeTransmit() void ZephyrBoard::onAfterTransmit() { #if HAS_TX_LED + atomic_set(&s_tx_lit, 0); gpio_pin_set_dt(&tx_led, 0); +#if ZEPHCORE_LED_PIN_SHARED + zephcore_led_radio_hold_pin(false); +#endif +#endif +} + +void ZephyrBoard::onPacketReceived() +{ +#if HAS_TX_LED + uint8_t mode = zephcore_leds_radio_mode(); + if (zephcore_leds_disabled() || + (mode != LEDS_RADIO_RX && mode != LEDS_RADIO_ALL)) { + return; + } + /* Never interrupt a transmit that is holding the LED. Half-duplex makes + * this all but impossible in practice, but the two run on different + * threads and the cost of being wrong is an LED stuck dark for a whole + * packet. */ + if (atomic_get(&s_tx_lit)) { + return; + } +#if ZEPHCORE_LED_PIN_SHARED + zephcore_led_radio_hold_pin(true); +#endif + gpio_pin_set_dt(&tx_led, 1); + /* Reschedule rather than schedule: back-to-back packets should extend the + * blink, not have the first one's handler cut the second one short. */ + k_work_reschedule(&s_rx_pulse_off, K_MSEC(RX_PULSE_MS)); #endif } diff --git a/zephcore/adapters/board/ZephyrBoard.h b/zephcore/adapters/board/ZephyrBoard.h index da32be0..8dce9d0 100644 --- a/zephcore/adapters/board/ZephyrBoard.h +++ b/zephcore/adapters/board/ZephyrBoard.h @@ -19,6 +19,7 @@ public: const char *getManufacturerName() const override; void onBeforeTransmit() override; void onAfterTransmit() override; + void onPacketReceived() override; void reboot() override; void rebootToBootloader(); /* Reboot into UF2 mass storage bootloader */ bool getBootloaderVersion(char *version, size_t max_len) override; diff --git a/zephcore/adapters/datastore/ZephyrDataStore.cpp b/zephcore/adapters/datastore/ZephyrDataStore.cpp index d5cc05c..cafb140 100644 --- a/zephcore/adapters/datastore/ZephyrDataStore.cpp +++ b/zephcore/adapters/datastore/ZephyrDataStore.cpp @@ -852,6 +852,18 @@ void ZephyrDataStore::loadPrefs(NodePrefs &prefs) prefs.tz_offset = (int8_t)buf[off++]; } + /* Offset 172-173: leds_radio_mode / leds_hb_mode (ZephCore extension). + * Absent in pre-existing files → both stay at the initNodePrefs() defaults + * of 0, and 0 is deliberately the behaviour every already-deployed node has + * (activity LED on transmit, heartbeat with unread indication). Range is + * re-checked by sanitizeNodePrefs() below. */ + if (off < len) { + prefs.leds_radio_mode = buf[off++]; + } + if (off < len) { + prefs.leds_hb_mode = buf[off++]; + } + sanitizeNodePrefs(&prefs); } @@ -963,7 +975,10 @@ void ZephyrDataStore::savePrefs(const NodePrefs &prefs) /* Offset 171: tz_offset (ZephCore extension, signed whole hours from UTC, * display only — the stored clock is always UTC) */ buf[off++] = (uint8_t)prefs.tz_offset; - /* Total: 172 bytes */ + /* Offset 172-173: leds_radio_mode / leds_hb_mode (ZephCore extension). */ + buf[off++] = prefs.leds_radio_mode; + buf[off++] = prefs.leds_hb_mode; + /* Total: 174 bytes */ bool ok = atomicReplaceFile(PREFS_FILE, buf, off); LOG_DBG("savePrefs: wrote %s, ok=%d (%d bytes), name='%.16s'", diff --git a/zephcore/adapters/radio/LoRaRadioBase.cpp b/zephcore/adapters/radio/LoRaRadioBase.cpp index 66465fd..c0e2d45 100644 --- a/zephcore/adapters/radio/LoRaRadioBase.cpp +++ b/zephcore/adapters/radio/LoRaRadioBase.cpp @@ -296,6 +296,12 @@ void LoRaRadioBase::rxCallbackStatic(const struct device *dev, uint8_t *data, self->_last_snr = (float)snr; atomic_inc(&self->_packets_recv); + /* Activity LED ("set leds.radio rx|all"). Deliberately below the CRC and + * header-error early return above, so the blink means a valid packet + * landed rather than that something was heard on the channel. Cheap and + * non-blocking: the board raises a GPIO and arms a one-shot. */ + self->_board->onPacketReceived(); + if (self->_rx_cb) { self->_rx_cb(self->_rx_cb_user_data); } diff --git a/zephcore/app/RepeaterDataStore.cpp b/zephcore/app/RepeaterDataStore.cpp index c3521f2..a627457 100644 --- a/zephcore/app/RepeaterDataStore.cpp +++ b/zephcore/app/RepeaterDataStore.cpp @@ -287,6 +287,12 @@ bool RepeaterDataStore::loadPrefs(NodePrefs& prefs) { * no-op EOF read leaves 0 = UTC, which is what every already-deployed * node shows today. Range is re-checked by sanitizeNodePrefs(). */ fs_read(&file, &prefs.tz_offset, sizeof(prefs.tz_offset)); + /* LED activity/heartbeat modes, offsets 309-310. Absent in <311-byte + * files; the no-op EOF read leaves the initNodePrefs() defaults of 0/0, + * which are deliberately the behaviour every already-deployed node has + * (activity LED on transmit, heartbeat with unread indication). */ + fs_read(&file, &prefs.leds_radio_mode, sizeof(prefs.leds_radio_mode)); + fs_read(&file, &prefs.leds_hb_mode, sizeof(prefs.leds_hb_mode)); fs_close(&file); @@ -442,6 +448,9 @@ bool RepeaterDataStore::savePrefs(const NodePrefs& prefs) { /* Display timezone offset (offset 308) — signed whole hours from UTC, * applied only when formatting the on-device clock */ fs_write(&file, &prefs.tz_offset, sizeof(prefs.tz_offset)); + /* LED activity/heartbeat modes (offsets 309-310) */ + fs_write(&file, &prefs.leds_radio_mode, sizeof(prefs.leds_radio_mode)); + fs_write(&file, &prefs.leds_hb_mode, sizeof(prefs.leds_hb_mode)); ret = fs_sync(&file); fs_close(&file); diff --git a/zephcore/app/main_observer.cpp b/zephcore/app/main_observer.cpp index 2bc2ae6..bf78338 100644 --- a/zephcore/app/main_observer.cpp +++ b/zephcore/app/main_observer.cpp @@ -383,6 +383,8 @@ int main(void) { bool leds_off = prefs->leds_disabled != 0; zephcore_leds_set_disabled(leds_off); + zephcore_leds_set_radio_mode(prefs->leds_radio_mode); + zephcore_leds_set_hb_mode(prefs->leds_hb_mode); LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); } diff --git a/zephcore/helpers/CommonCLI.cpp b/zephcore/helpers/CommonCLI.cpp index aae78b1..9eddfee 100644 --- a/zephcore/helpers/CommonCLI.cpp +++ b/zephcore/helpers/CommonCLI.cpp @@ -66,6 +66,48 @@ static const NodePrefs* cliDefaults() { return &s_cli_defaults; } +/* ---- leds.radio / leds.hb mode names ----------------------------------- */ +/* + * Which LEDs this board actually has, so the CLI can tell the user when a + * setting it just accepted will not do anything here. The pref is still + * stored either way: the same prefs file follows a node onto a board that does + * have the LED, and silently dropping the value would be worse than storing a + * setting that is dormant. + * + * These mirror the aliases the drivers use — lora-tx-led in ZephyrBoard.cpp, + * led0 with an led1 fallback in helpers/ui/ui_common.c. + */ +#define CLI_HAS_RADIO_LED DT_NODE_EXISTS(DT_ALIAS(lora_tx_led)) +#define CLI_HAS_HB_LED (DT_NODE_HAS_PROP(DT_ALIAS(led0), gpios) || \ + DT_NODE_HAS_PROP(DT_ALIAS(led1), gpios)) + +struct CliModeName { const char* name; uint8_t val; }; + +static const CliModeName LEDS_RADIO_NAMES[] = { + { "tx", LEDS_RADIO_TX }, + { "rx", LEDS_RADIO_RX }, + { "all", LEDS_RADIO_ALL }, + { "off", LEDS_RADIO_OFF }, +}; + +static const CliModeName LEDS_HB_NAMES[] = { + { "all", LEDS_HB_ALL }, + { "hb", LEDS_HB_HB }, + { "unread", LEDS_HB_UNREAD }, + { "off", LEDS_HB_OFF }, +}; + +static const char* cliModeName(const CliModeName* tbl, size_t n, uint8_t v) { + for (size_t i = 0; i < n; i++) { + if (tbl[i].val == v) return tbl[i].name; + } + return "?"; +} + +/* Returns the mode value, or -1 if the argument matches nothing. "default" + * resolves through cliDefaults() like every other setting. */ +static int cliModeValue(const CliModeName* tbl, size_t n, const char* arg, uint8_t def_val); + static const char* cliSkipSpace(const char* s) { while (*s == ' ') s++; return s; @@ -77,6 +119,15 @@ static bool cliIsDefault(const char* arg) { return *cliSkipSpace(s + 7) == '\0'; } +static int cliModeValue(const CliModeName* tbl, size_t n, const char* arg, uint8_t def_val) { + const char* s = cliSkipSpace(arg); + if (cliIsDefault(s)) return (int)def_val; + for (size_t i = 0; i < n; i++) { + if (strcmp(s, tbl[i].name) == 0) return (int)tbl[i].val; + } + return -1; +} + /* Integer argument, or "default". false = unparseable; the caller reports * usage rather than acting on a silently-zeroed value. */ static bool cliNum(const char* arg, long def_val, long* out) { @@ -357,6 +408,17 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch snprintf(reply, CLI_REPLY_SIZE, "> %.2f", (double)_prefs->airtime_factor); } else if (memcmp(config, "int.thresh", 10) == 0) { snprintf(reply, CLI_REPLY_SIZE, "> %u", (uint32_t)_prefs->interference_threshold); + /* MUST stay above the "leds" branch: that one compares only the first + * four characters, so "leds.radio" and "leds.hb" both match it and + * would otherwise return the master switch instead. */ + } else if (memcmp(config, "leds.radio", 10) == 0) { + snprintf(reply, CLI_REPLY_SIZE, "> %s%s", + cliModeName(LEDS_RADIO_NAMES, 4, _prefs->leds_radio_mode), + CLI_HAS_RADIO_LED ? "" : " (no radio LED on this board)"); + } else if (memcmp(config, "leds.hb", 7) == 0) { + snprintf(reply, CLI_REPLY_SIZE, "> %s%s", + cliModeName(LEDS_HB_NAMES, 4, _prefs->leds_hb_mode), + CLI_HAS_HB_LED ? "" : " (no heartbeat LED on this board)"); } else if (memcmp(config, "leds", 4) == 0) { snprintf(reply, CLI_REPLY_SIZE, "> %s", _prefs->leds_disabled ? "off" : "on"); #ifndef ZEPHCORE_REPEATER @@ -638,6 +700,34 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch strcpy(reply, "OK"); } } + } else if (memcmp(config, "leds.radio ", 11) == 0) { + /* What the LoRa activity LED reacts to. Below the master switch: + * "set leds off" keeps it dark whatever this says. */ + int m = cliModeValue(LEDS_RADIO_NAMES, 4, &config[11], + cliDefaults()->leds_radio_mode); + if (m < 0) { + strcpy(reply, "Error: must be tx, rx, all, off or default"); + } else { + _prefs->leds_radio_mode = (uint8_t)m; + zephcore_leds_set_radio_mode((uint8_t)m); + savePrefs(); + snprintf(reply, CLI_REPLY_SIZE, "OK%s", + CLI_HAS_RADIO_LED ? "" : " (no radio LED on this board)"); + } + } else if (memcmp(config, "leds.hb ", 8) == 0) { + /* What the heartbeat LED reacts to. "unread" is the same cycle + * widening its pulse, not a separate blink — see led_gate.h. */ + int m = cliModeValue(LEDS_HB_NAMES, 4, &config[8], + cliDefaults()->leds_hb_mode); + if (m < 0) { + strcpy(reply, "Error: must be hb, unread, all, off or default"); + } else { + _prefs->leds_hb_mode = (uint8_t)m; + zephcore_leds_set_hb_mode((uint8_t)m); + savePrefs(); + snprintf(reply, CLI_REPLY_SIZE, "OK%s", + CLI_HAS_HB_LED ? "" : " (no heartbeat LED on this board)"); + } } else if (memcmp(config, "leds ", 5) == 0) { /* Master switch for every LED on the node: heartbeat, unread-message * and LoRa TX activity, plus the message and shutdown flashes. Not diff --git a/zephcore/helpers/NodePrefs.h b/zephcore/helpers/NodePrefs.h index a4b9984..2779a81 100644 --- a/zephcore/helpers/NodePrefs.h +++ b/zephcore/helpers/NodePrefs.h @@ -11,6 +11,11 @@ #include #include +/* LEDS_RADIO_* / LEDS_HB_* mode values for the leds_radio_mode and leds_hb_mode + * fields below. They live in led_gate.h because the heartbeat consumers are C + * and this header is C++; see the note there. */ +#include "led_gate.h" + #define TELEM_MODE_DENY 0 #define TELEM_MODE_ALLOW_FLAGS 1 #define TELEM_MODE_ALLOW_ALL 2 @@ -88,6 +93,8 @@ struct NodePrefs { uint8_t flood_max_advert; // hop limit for ADVERT floods (curbs advert churn) uint8_t interference_threshold; uint8_t leds_disabled; // 1 = all LEDs off (heartbeat, unread, LoRa TX) + uint8_t leds_radio_mode; // LEDS_RADIO_* — activity LED source (0 = TX, as before) + uint8_t leds_hb_mode; // LEDS_HB_* — heartbeat LED behaviour (0 = all, as before) // Power saving uint8_t powersaving_enabled; // GPS settings @@ -232,6 +239,11 @@ static inline T sanePrefFloat(T v, T lo, T hi, T fallback) { template static inline T saneBool(T v, T fallback) { return (v == 0 || v == 1) ? v : fallback; } +/* Same idea as saneBool for a small enum: anything outside 0..max carries no + * user intent, so fall back to 0 — which every LEDS_* enum defines as the + * behaviour the firmware had before the setting existed. */ +static inline uint8_t saneEnum(uint8_t v, uint8_t max) { return (v <= max) ? v : 0; } + static inline void sanitizeNodePrefs(NodePrefs* p) { p->node_name[sizeof(p->node_name) - 1] = '\0'; p->password[sizeof(p->password) - 1] = '\0'; @@ -270,6 +282,8 @@ static inline void sanitizeNodePrefs(NodePrefs* p) { p->gps_enabled = saneBool(p->gps_enabled, 0); p->rx_duty_cycle = saneBool(p->rx_duty_cycle, 0); p->leds_disabled = saneBool(p->leds_disabled, 0); + p->leds_radio_mode = saneEnum(p->leds_radio_mode, LEDS_RADIO_MAX); + p->leds_hb_mode = saneEnum(p->leds_hb_mode, LEDS_HB_MAX); p->meshtimesync = saneBool(p->meshtimesync, 0); /* Fallback must be the initNodePrefs() default (ON). It was 0, so a byte * that was neither 0 nor 1 silently switched adaptive CAD off instead of @@ -370,6 +384,8 @@ static inline void initNodePrefs(NodePrefs* prefs) { prefs->flood_max_advert = 8; // ADVERT flood hop limit (upstream default) prefs->interference_threshold = 0; prefs->leds_disabled = 0; // LEDs on + prefs->leds_radio_mode = LEDS_RADIO_TX; // activity LED on transmit, as before + prefs->leds_hb_mode = LEDS_HB_ALL; // heartbeat + unread, as before prefs->powersaving_enabled = 0; prefs->gps_enabled = 0; prefs->gps_interval = 300; // 5 minutes diff --git a/zephcore/helpers/led_gate.c b/zephcore/helpers/led_gate.c index ca3815e..610bbb6 100644 --- a/zephcore/helpers/led_gate.c +++ b/zephcore/helpers/led_gate.c @@ -34,3 +34,43 @@ void zephcore_leds_set_disabled(bool disabled) atomic_set(&s_leds_disabled, disabled ? 1 : 0); zephcore_leds_ui_sync(disabled); } + +/* + * Both modes default to 0, which every LEDS_* enum defines as the behaviour the + * firmware had before these settings existed. That matters beyond tidiness: a + * role whose boot path forgets to apply the pref, or a build with no CLI at + * all, still lands on the historical behaviour rather than something new. + */ +static atomic_t s_radio_mode; /* LEDS_RADIO_TX */ +static atomic_t s_hb_mode; /* LEDS_HB_ALL */ +static atomic_t s_radio_holds_pin; + +uint8_t zephcore_leds_radio_mode(void) +{ + return (uint8_t)atomic_get(&s_radio_mode); +} + +void zephcore_leds_set_radio_mode(uint8_t mode) +{ + atomic_set(&s_radio_mode, mode); +} + +uint8_t zephcore_leds_hb_mode(void) +{ + return (uint8_t)atomic_get(&s_hb_mode); +} + +void zephcore_leds_set_hb_mode(uint8_t mode) +{ + atomic_set(&s_hb_mode, mode); +} + +bool zephcore_led_radio_holds_pin(void) +{ + return atomic_get(&s_radio_holds_pin) != 0; +} + +void zephcore_led_radio_hold_pin(bool held) +{ + atomic_set(&s_radio_holds_pin, held ? 1 : 0); +} diff --git a/zephcore/helpers/led_gate.h b/zephcore/helpers/led_gate.h index b993628..020ce72 100644 --- a/zephcore/helpers/led_gate.h +++ b/zephcore/helpers/led_gate.h @@ -20,6 +20,7 @@ #define ZEPHCORE_LED_GATE_H #include +#include #ifdef __cplusplus extern "C" { @@ -38,6 +39,68 @@ void zephcore_leds_set_disabled(bool disabled); * cycle and refresh the UI's LED page. Not meant to be called directly. */ void zephcore_leds_ui_sync(bool disabled); +/* + * Mode values for the two per-LED settings. They live here rather than in + * NodePrefs.h, where the rest of the prefs enums are, because both consumers of + * the heartbeat modes are C (helpers/ui/ui_common.c) and NodePrefs.h is C++. + * NodePrefs.h includes this header so the persisted fields and these values + * still have exactly one definition between them. + * + * leds.radio — what the LoRa activity LED (the `lora-tx-led` DT alias) reacts + * to. TX is deliberately value 0: it is what every node did before the setting + * existed, so an absent byte in an old prefs file, a short read, and a + * zero-filled byte all decode to the historical behaviour. Same reasoning + * applies to LEDS_HB_ALL. Boards without the alias have no activity LED at all + * and ignore this setting whatever it says. + */ +#define LEDS_RADIO_TX 0 /* lit for the duration of each transmit (default) */ +#define LEDS_RADIO_RX 1 /* short pulse on each packet received */ +#define LEDS_RADIO_ALL 2 /* both of the above */ +#define LEDS_RADIO_OFF 3 /* activity LED stays dark */ +#define LEDS_RADIO_MAX LEDS_RADIO_OFF + +/* + * leds.hb — what the heartbeat LED (`led0`, or `led1` on a board with no + * `led0`) reacts to. "unread" is not a separate blink: it is the existing + * cycle widening its pulse from 20 ms to 200 ms, plus the second LED on boards + * that have one. So LEDS_HB_HB is "never widen", and LEDS_HB_UNREAD is "only + * blink when it would have widened". + */ +#define LEDS_HB_ALL 0 /* liveness tick + unread indication (default) */ +#define LEDS_HB_HB 1 /* liveness tick only, never widens */ +#define LEDS_HB_UNREAD 2 /* dark unless there are unread messages */ +#define LEDS_HB_OFF 3 /* heartbeat LED stays dark */ +#define LEDS_HB_MAX LEDS_HB_OFF + +/* + * Per-LED modes, below the master gate: "set leds off" still wins over both. + * + * Kept here rather than read from NodePrefs directly because the consumers are + * on different threads from the CLI that writes them: the heartbeat runs on the + * system work queue and the activity LED on the radio's TX path, so both need a + * lock-free snapshot rather than a pointer into a struct the main thread edits. + */ +uint8_t zephcore_leds_radio_mode(void); +void zephcore_leds_set_radio_mode(uint8_t mode); + +uint8_t zephcore_leds_hb_mode(void); +void zephcore_leds_set_hb_mode(uint8_t mode); + +/* + * Shared-pin arbitration. On 8 of the supported boards the `lora-tx-led` alias + * IS `led0`, so the heartbeat cycle and the radio activity LED drive the same + * physical GPIO from two different modules. Without this the two clip each + * other: the heartbeat's off-work clears the pin in the middle of a transmit, + * and onAfterTransmit() truncates a heartbeat pulse. + * + * ZephyrBoard raises the hold for the transmit window and for each RX pulse; + * ui_common.c's heartbeat handlers skip lighting and clearing the pin while it + * is held, so radio activity wins and the liveness tick yields. Boards whose + * pins differ compile the check out entirely (see ZEPHCORE_LED_PIN_SHARED). + */ +bool zephcore_led_radio_holds_pin(void); +void zephcore_led_radio_hold_pin(bool held); + #ifdef __cplusplus } #endif diff --git a/zephcore/helpers/ui/ui_common.c b/zephcore/helpers/ui/ui_common.c index ff90fa3..46d6171 100644 --- a/zephcore/helpers/ui/ui_common.c +++ b/zephcore/helpers/ui/ui_common.c @@ -133,14 +133,48 @@ static struct k_work_delayable s_led_off_work; */ __attribute__((weak)) uint16_t ui_led_get_msg_count(void) { return 0; } +/* + * Does the heartbeat LED light on this pass? "unread" is not a separate blink + * — it is this same cycle widening its pulse — so the modes are expressed as + * two questions over one cycle: may it light at all right now, and how wide. + * + * The cycle keeps running in every mode including LEDS_HB_OFF. That is on + * purpose: on companions with two LEDs the unread indicator is lit from inside + * this work chain, so stopping the chain would take unread indication down with + * the heartbeat. An idle pass costs one work item every 4 s. + */ +static bool hb_should_light(uint16_t msg_count) +{ + switch (zephcore_leds_hb_mode()) { + case LEDS_HB_OFF: return false; + case LEDS_HB_UNREAD: return msg_count > 0; /* dark unless there is news */ + default: return true; /* LEDS_HB_ALL, LEDS_HB_HB */ + } +} + +/* Pulse width for this pass. LEDS_HB_HB is the "liveness tick only" mode, so + * it never widens even when messages are waiting. */ +static uint16_t hb_pulse_ms(uint16_t msg_count) +{ + if (zephcore_leds_hb_mode() == LEDS_HB_HB) { + return LED_ON_MS; + } + return (msg_count > 0) ? LED_ON_MSG_MS : LED_ON_MS; +} + static void led_off_work_handler(struct k_work *work) { ARG_UNUSED(work); - gpio_pin_set_dt(&s_heartbeat_led, 0); + /* Yield the pin if radio activity is holding it (shared-pin boards only; + * everywhere else this always reads false). Clearing here would blank the + * LED in the middle of a transmit. */ + if (!zephcore_led_radio_holds_pin()) { + gpio_pin_set_dt(&s_heartbeat_led, 0); + } #if HAS_MSG_LED gpio_pin_set_dt(&s_msg_led, 0); #endif - uint16_t on_ms = (ui_led_get_msg_count() > 0) ? LED_ON_MSG_MS : LED_ON_MS; + uint16_t on_ms = hb_pulse_ms(ui_led_get_msg_count()); k_work_reschedule(&s_led_on_work, K_MSEC(LED_CYCLE_MS - on_ms)); } @@ -149,12 +183,18 @@ static void led_on_work_handler(struct k_work *work) { ARG_UNUSED(work); uint16_t mc = ui_led_get_msg_count(); - uint16_t on_ms = (mc > 0) ? LED_ON_MSG_MS : LED_ON_MS; + uint16_t on_ms = hb_pulse_ms(mc); if (!zephcore_leds_disabled()) { - gpio_pin_set_dt(&s_heartbeat_led, 1); + if (hb_should_light(mc) && !zephcore_led_radio_holds_pin()) { + gpio_pin_set_dt(&s_heartbeat_led, 1); + } #if HAS_MSG_LED - if (mc > 0) { + /* The unread LED is a separate pin, so it is governed by the mode + * but not by the radio's hold on the heartbeat pin. LEDS_HB_HB is + * the liveness-only mode and deliberately suppresses it. */ + if (mc > 0 && zephcore_leds_hb_mode() != LEDS_HB_OFF && + zephcore_leds_hb_mode() != LEDS_HB_HB) { gpio_pin_set_dt(&s_msg_led, 1); } #endif diff --git a/zephcore/include/mesh/Board.h b/zephcore/include/mesh/Board.h index 7b20e73..ce67cfe 100644 --- a/zephcore/include/mesh/Board.h +++ b/zephcore/include/mesh/Board.h @@ -24,6 +24,11 @@ public: virtual const char *getManufacturerName() const = 0; virtual void onBeforeTransmit() {} virtual void onAfterTransmit() {} + /* A valid packet has just been received. Unlike the transmit pair this is + * a single edge, not a window: the packet is already over by the time the + * radio tells us, so an implementation that drives an LED has to fire a + * one-shot rather than hold a level. */ + virtual void onPacketReceived() {} virtual void reboot() = 0; virtual void powerOff() {} virtual void sleep(uint32_t secs) { (void)secs; } diff --git a/zephcore/src/main_companion.cpp b/zephcore/src/main_companion.cpp index 127bc86..a347485 100644 --- a/zephcore/src/main_companion.cpp +++ b/zephcore/src/main_companion.cpp @@ -1601,6 +1601,8 @@ int main(void) * LED and is linked into every build, UI or not. */ bool leds_off = companion_mesh.prefs.leds_disabled != 0; zephcore_leds_set_disabled(leds_off); + zephcore_leds_set_radio_mode(companion_mesh.prefs.leds_radio_mode); + zephcore_leds_set_hb_mode(companion_mesh.prefs.leds_hb_mode); ui_set_heartbeat_led(!leds_off); LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); diff --git a/zephcore/src/main_repeater.cpp b/zephcore/src/main_repeater.cpp index d87c733..a6ab9e7 100644 --- a/zephcore/src/main_repeater.cpp +++ b/zephcore/src/main_repeater.cpp @@ -713,15 +713,6 @@ int main(void) oled_sleep(); #endif - /* Apply the persisted LED master switch ("set leds on|off"). After ui_init() - * so the heartbeat cycle exists to be stopped; before the radio starts so the - * first transmit already honours it. */ - { - bool leds_off = repeater_mesh.getNodePrefs()->leds_disabled != 0; - zephcore_leds_set_disabled(leds_off); - LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); - } - /* Log environment sensor availability */ if (env_sensors_available()) { LOG_INF("Environment sensors available"); @@ -767,6 +758,26 @@ int main(void) data_store.loadPrefs(*repeater_mesh.getNodePrefs()); lora_radio.setPrefs(repeater_mesh.getNodePrefs()); + /* Apply the persisted LED master switch ("set leds on|off"). MUST come + * after loadPrefs(): this used to sit just after ui_init(), ~45 lines + * earlier, where getNodePrefs() still held the initNodePrefs() default of + * leds_disabled=0. A node with "off" persisted therefore opened the gate on + * every boot and never closed it again -- `get leds` read the (correct) RAM + * prefs and said "off" while the LEDs kept blinking, until the user issued + * `set leds off` a second time to drive the gate directly. The other two + * ordering constraints still hold here: ui_init() has already run, so the + * heartbeat cycle exists to be stopped, and repeater_mesh.begin() -> + * Dispatcher::begin() -> Radio::begin() is still below, so the first + * transmit honours it. */ + { + const NodePrefs *lp = repeater_mesh.getNodePrefs(); + bool leds_off = lp->leds_disabled != 0; + zephcore_leds_set_disabled(leds_off); + zephcore_leds_set_radio_mode(lp->leds_radio_mode); + zephcore_leds_set_hb_mode(lp->leds_hb_mode); + LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); + } + /* Start mesh with data store - loads ACL, regions */ repeater_mesh.begin(&data_store); diff --git a/zephcore/src/main_room_server.cpp b/zephcore/src/main_room_server.cpp index ba037fc..0c7718e 100644 --- a/zephcore/src/main_room_server.cpp +++ b/zephcore/src/main_room_server.cpp @@ -599,15 +599,6 @@ int main(void) oled_sleep(); #endif - /* Apply the persisted LED master switch ("set leds on|off"). After ui_init() - * so the heartbeat cycle exists to be stopped; before the radio starts so the - * first transmit already honours it. */ - { - bool leds_off = room_mesh.getNodePrefs()->leds_disabled != 0; - zephcore_leds_set_disabled(leds_off); - LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); - } - /* Log environment sensor availability */ if (env_sensors_available()) { LOG_INF("Environment sensors available"); @@ -652,6 +643,26 @@ int main(void) data_store.loadPrefs(*room_mesh.getNodePrefs()); lora_radio.setPrefs(room_mesh.getNodePrefs()); + /* Apply the persisted LED master switch ("set leds on|off"). MUST come + * after loadPrefs(): this used to sit just after ui_init(), ~45 lines + * earlier, where getNodePrefs() still held the initNodePrefs() default of + * leds_disabled=0. A node with "off" persisted therefore opened the gate on + * every boot and never closed it again -- `get leds` read the (correct) RAM + * prefs and said "off" while the LEDs kept blinking, until the user issued + * `set leds off` a second time to drive the gate directly. The other two + * ordering constraints still hold here: ui_init() has already run, so the + * heartbeat cycle exists to be stopped, and room_mesh.begin() -> + * Dispatcher::begin() -> Radio::begin() is still below, so the first + * transmit honours it. */ + { + const NodePrefs *lp = room_mesh.getNodePrefs(); + bool leds_off = lp->leds_disabled != 0; + zephcore_leds_set_disabled(leds_off); + zephcore_leds_set_radio_mode(lp->leds_radio_mode); + zephcore_leds_set_hb_mode(lp->leds_hb_mode); + LOG_INF("LEDs: %s (from prefs)", leds_off ? "disabled" : "enabled"); + } + /* Start mesh with data store - loads ACL, regions */ room_mesh.begin(&data_store);