leds refactor/improvement

zephcore/adapters/board/ZephyrBoard.cpp

@@ -5,6 +5,7 @@
 #include "ZephyrBoard.h"
 #include "battery_curve.h"
 #include "led_gate.h"
+#include <NodePrefs.h>   /* LEDS_RADIO_* mode values */
 #include <zephyr/kernel.h>
 #include <zephyr/sys/reboot.h>
 #include <zephyr/drivers/sensor.h>
@@ -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 <zephyr/logging/log.h>
 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
 }
 

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;

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'",

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);
 	}

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);

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");
 	}
 

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

zephcore/helpers/NodePrefs.h

@@ -11,6 +11,11 @@
 #include <stdint.h>
 #include <string.h>
 
+/* 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 <typename T>
 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<uint8_t>(p->gps_enabled, 0);
 	p->rx_duty_cycle       = saneBool<uint8_t>(p->rx_duty_cycle, 0);
 	p->leds_disabled       = saneBool<uint8_t>(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<uint8_t>(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

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);
+}

zephcore/helpers/led_gate.h

@@ -20,6 +20,7 @@
 #define ZEPHCORE_LED_GATE_H
 
 #include <stdbool.h>
+#include <stdint.h>
 
 #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

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

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; }

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");
 

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);
 

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);