From 269b974e9606a8e1c5db8dabb95f0a3ec6f168d7 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:28:25 -0500 Subject: [PATCH] feat: populate MyNodeInfo.device_id on all platforms (#10995) * feat: populate MyNodeInfo.device_id on all platforms RP2040/RP2350 use the 64-bit pico unique board id, STM32WL the 96-bit silicon UID, ESP32-S2 joins the existing OPTIONAL_UNIQUE_ID efuse branch, and everything else (classic ESP32 in particular) falls back to a deterministic factory-MAC-derived id, resolving the long-standing FIXME. Portduino keeps the config-supplied id preferred and now uses the MAC fallback when the config omits one. No proto or persistence changes; the id is re-read from silicon each boot and PhoneAPI still zeroes it for unauthenticated clients. Co-Authored-By: Claude Fable 5 * fix: declare zero_mac const to satisfy cppcheck Co-Authored-By: Claude Fable 5 * fix: address review feedback on device_id derivation Clear any disk-loaded device_id before the silicon derivation so a failed read leaves it unset rather than stale (Copilot), and size the portduino config copy with sizeof instead of a literal 16 (CodeRabbit). Co-Authored-By: Claude Fable 5 * refactor: extract device_id generation into per-arch getDeviceId() Per review feedback on #10995: move the platform-specific MyNodeInfo.device_id derivation out of the #if/#elif ladder in NodeDB.cpp into a getDeviceId() interface (target_specific.h) implemented per-architecture alongside each platform's getMacAddr(): - esp32: efuse OPTIONAL_UNIQUE_ID (C3/S2/S3/C6); classic ESP32 -> MAC - nrf52: FICR DEVICEID + DEVICEADDR - nrf54l15: FICR->INFO.DEVICEID + DEVICEADDR (NRF_FICR-guarded, MAC fallback) - rp2xx0: pico_get_unique_board_id() - stm32wl: HAL_GetUIDw0/1/2() - portduino: config-supplied id preferred, else MAC The shared MAC-derived fallback moves to meshUtils as getMacAddrDeviceId(). NodeDB.cpp now zero-inits the field and makes a single getDeviceId() call, dropping ~65 lines of platform boilerplate plus the esp_efuse/pico/stm32 includes that came with it. No behavior change: device_id is still re-read from silicon each boot and never persisted. Builds green: native-macos, tbeam, heltec-v3, rak4631, rak11310, rak3172. Co-Authored-By: Claude Opus 4.8 * fix: address review of device_id refactor - getMacAddrDeviceId(): zero-init the mac[6] buffer. getMacAddr() can return without writing (e.g. Portduino with no MAC source), so the old uninitialized buffer let stack garbage pass the all-zeros guard and become device_id. The pre-refactor code relied on the zero-initialized static ourMacAddr; restore that guarantee. - nrf54l15 getDeviceId(): drop the `#if defined(NRF_FICR)` guard and read FICR unconditionally (as the pre-refactor NodeDB code did). The guarded #else fell back to getMacAddr()'s hard-coded placeholder MAC, which would give every unit an identical device_id; a missing NRF_FICR should be a loud compile error. - NodeDB.cpp: `#include "target_specific.h"` instead of hand-copied externs for getMacAddr/getDeviceId; retire the stale FIXME. Same for meshUtils.cpp (whose extern comment wrongly claimed the TU was Arduino-free). - Delete the orphaned commented-out device_id hex-dump block in NodeDB.cpp. - Trim/de-duplicate the getDeviceId contract comments (single-sourced in target_specific.h). Builds green: native-macos, tbeam. Co-Authored-By: Claude Opus 4.8 * docs: trim device_id comments to the repo's 1-2 line guideline Addresses CodeRabbit review nitpick on #10995: shorten the getDeviceId() (target_specific.h), getMacAddrDeviceId() (meshUtils.h), device-id refresh (NodeDB.cpp), and nrf54l15 getDeviceId comments to two lines each, per the "one or two lines maximum" coding guideline. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Fable 5 --- src/mesh/NodeDB.cpp | 60 ++++-------------------- src/meshUtils.cpp | 15 ++++++ src/meshUtils.h | 4 ++ src/platform/esp32/main-esp32.cpp | 22 +++++++++ src/platform/nrf52/main-nrf52.cpp | 11 +++++ src/platform/nrf54l15/main-nrf54l15.cpp | 12 +++++ src/platform/portduino/PortduinoGlue.cpp | 10 ++++ src/platform/rp2xx0/main-rp2xx0.cpp | 10 ++++ src/platform/stm32wl/main-stm32wl.cpp | 9 ++++ src/target_specific.h | 6 ++- 10 files changed, 106 insertions(+), 53 deletions(-) diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index b2804910c..e53ebb699 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -27,6 +27,7 @@ #include "mesh/generated/meshtastic/deviceonly_legacy.pb.h" #include "meshUtils.h" #include "modules/NeighborInfoModule.h" +#include "target_specific.h" #if HAS_VARIABLE_HOPS #include "modules/HopScalingModule.h" #endif @@ -50,11 +51,7 @@ #include "SPILock.h" #include "modules/StoreForwardModule.h" #include -#include -#include #include -#include -#include #endif #ifdef ARCH_PORTDUINO @@ -372,8 +369,7 @@ void NodeDB::disarmNodeDatabaseDecodeTargets() */ uint32_t radioGeneration; -// FIXME - move this somewhere else -extern void getMacAddr(uint8_t *dmac); +// getMacAddr() and getDeviceId() are the per-architecture hooks declared in target_specific.h. /** * @@ -410,53 +406,13 @@ NodeDB::NodeDB() uint32_t channelFileCRC = crc32Buffer(&channelFile, sizeof(channelFile)); int saveWhat = 0; - // Get device unique id -#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C6) - uint32_t unique_id[4]; - // ESP32 factory burns a unique id in efuse for S2+ series and evidently C3+ series - // This is used for HMACs in the esp-rainmaker AIOT platform and seems to be a good choice for us - esp_err_t err = esp_efuse_read_field_blob(ESP_EFUSE_OPTIONAL_UNIQUE_ID, unique_id, sizeof(unique_id) * 8); - if (err == ESP_OK) { - memcpy(myNodeInfo.device_id.bytes, unique_id, sizeof(unique_id)); - myNodeInfo.device_id.size = 16; - } else { - LOG_WARN("Failed to read unique id from efuse"); + // Re-read the device id from silicon each boot via the per-arch getDeviceId(); clear the + // disk-loaded value first so a failed/empty derivation leaves it unset rather than stale. + myNodeInfo.device_id.size = 0; + memset(myNodeInfo.device_id.bytes, 0, sizeof(myNodeInfo.device_id.bytes)); + if (getDeviceId(myNodeInfo.device_id.bytes)) { + myNodeInfo.device_id.size = sizeof(myNodeInfo.device_id.bytes); } -#elif defined(ARCH_NRF54L15) - // nRF54L15: DEVICEID is under FICR->INFO sub-struct (not top-level as on nRF52) - uint64_t device_id_start = ((uint64_t)NRF_FICR->INFO.DEVICEID[1] << 32) | NRF_FICR->INFO.DEVICEID[0]; - uint64_t device_id_end = ((uint64_t)NRF_FICR->DEVICEADDR[1] << 32) | NRF_FICR->DEVICEADDR[0]; - memcpy(myNodeInfo.device_id.bytes, &device_id_start, sizeof(device_id_start)); - memcpy(myNodeInfo.device_id.bytes + sizeof(device_id_start), &device_id_end, sizeof(device_id_end)); - myNodeInfo.device_id.size = 16; -#elif defined(ARCH_NRF52) - // Nordic applies a FIPS compliant Random ID to each chip at the factory - // We concatenate the device address to the Random ID to create a unique ID for now - // This will likely utilize a crypto module in the future - uint64_t device_id_start = ((uint64_t)NRF_FICR->DEVICEID[1] << 32) | NRF_FICR->DEVICEID[0]; - uint64_t device_id_end = ((uint64_t)NRF_FICR->DEVICEADDR[1] << 32) | NRF_FICR->DEVICEADDR[0]; - memcpy(myNodeInfo.device_id.bytes, &device_id_start, sizeof(device_id_start)); - memcpy(myNodeInfo.device_id.bytes + sizeof(device_id_start), &device_id_end, sizeof(device_id_end)); - myNodeInfo.device_id.size = 16; - // Uncomment below to print the device id -#elif ARCH_PORTDUINO - if (portduino_config.has_device_id) { - memcpy(myNodeInfo.device_id.bytes, portduino_config.device_id, 16); - myNodeInfo.device_id.size = 16; - } -#else - // FIXME - implement for other platforms -#endif - - // if (myNodeInfo.device_id.size == 16) { - // std::string deviceIdHex; - // for (size_t i = 0; i < myNodeInfo.device_id.size; ++i) { - // char buf[3]; - // snprintf(buf, sizeof(buf), "%02X", myNodeInfo.device_id.bytes[i]); - // deviceIdHex += buf; - // } - // LOG_DEBUG("Device ID (HEX): %s", deviceIdHex.c_str()); - // } // likewise - we always want the app requirements to come from the running appload myNodeInfo.min_app_version = 30200; // format is Mmmss (where M is 1+the numeric major number. i.e. 30200 means 2.2.00 diff --git a/src/meshUtils.cpp b/src/meshUtils.cpp index 1fd21a244..834cc7276 100644 --- a/src/meshUtils.cpp +++ b/src/meshUtils.cpp @@ -1,4 +1,5 @@ #include "meshUtils.h" +#include "target_specific.h" #include /* @@ -79,6 +80,20 @@ bool memfll(const uint8_t *mem, uint8_t find, size_t numbytes) return true; } +bool getMacAddrDeviceId(uint8_t *deviceId) +{ + // Zero-initialized: getMacAddr() may return without writing (e.g. Portduino with no MAC + // source), and we want that no-write case to hit the all-zeros guard below, not read garbage. + uint8_t mac[6] = {0}; + getMacAddr(mac); + if (memfll(mac, 0, sizeof(mac))) { + LOG_WARN("MAC is all zeros, leaving device_id unset"); + return false; + } + memcpy(deviceId, mac, sizeof(mac)); + return true; +} + bool isOneOf(int item, int count, ...) { va_list args; diff --git a/src/meshUtils.h b/src/meshUtils.h index 0ce01c522..9a80ac51f 100644 --- a/src/meshUtils.h +++ b/src/meshUtils.h @@ -49,6 +49,10 @@ void printBytes(const char *label, const uint8_t *p, size_t numbytes); // is the memory region filled with a single character? bool memfll(const uint8_t *mem, uint8_t find, size_t numbytes); +// getDeviceId() fallback (see target_specific.h): copies the 6-byte factory MAC, or returns false +// on an all-zero MAC so two blank devices don't collide on an all-zero id. +bool getMacAddrDeviceId(uint8_t *deviceId); + bool isOneOf(int item, int count, ...); const std::string vformat(const char *const zcFormat, ...); diff --git a/src/platform/esp32/main-esp32.cpp b/src/platform/esp32/main-esp32.cpp index 25714e543..afbf4482d 100644 --- a/src/platform/esp32/main-esp32.cpp +++ b/src/platform/esp32/main-esp32.cpp @@ -27,6 +27,8 @@ #include "target_specific.h" #include #include +#include +#include #include #include @@ -158,6 +160,26 @@ void getMacAddr(uint8_t *dmac) #endif } +bool getDeviceId(uint8_t *deviceId) +{ +#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32S3) || \ + defined(CONFIG_IDF_TARGET_ESP32C6) + // ESP32 factory burns a 128-bit unique id in efuse for S2+ and C3+ series (used for HMACs + // in the esp-rainmaker AIOT platform); a good stable hardware anchor for us too. + uint32_t unique_id[4]; + esp_err_t err = esp_efuse_read_field_blob(ESP_EFUSE_OPTIONAL_UNIQUE_ID, unique_id, sizeof(unique_id) * 8); + if (err != ESP_OK) { + LOG_WARN("Failed to read unique id from efuse"); + return false; + } + memcpy(deviceId, unique_id, sizeof(unique_id)); + return true; +#else + // Classic ESP32 has no OPTIONAL_UNIQUE_ID efuse: fall back to the factory-burned MAC. + return getMacAddrDeviceId(deviceId); +#endif +} + #if HAS_32768HZ #define CALIBRATE_ONE(cali_clk) calibrate_one(cali_clk, #cali_clk) diff --git a/src/platform/nrf52/main-nrf52.cpp b/src/platform/nrf52/main-nrf52.cpp index f120d387c..87246ba21 100644 --- a/src/platform/nrf52/main-nrf52.cpp +++ b/src/platform/nrf52/main-nrf52.cpp @@ -186,6 +186,17 @@ void getMacAddr(uint8_t *dmac) dmac[0] = src[5] | 0xc0; // MSB high two bits get set elsewhere in the bluetooth stack } +bool getDeviceId(uint8_t *deviceId) +{ + // Nordic burns a FIPS-compliant random id into each chip at the factory. We concatenate + // the device address to that random id to form the 16-byte hardware identifier. + uint64_t device_id_start = ((uint64_t)NRF_FICR->DEVICEID[1] << 32) | NRF_FICR->DEVICEID[0]; + uint64_t device_id_end = ((uint64_t)NRF_FICR->DEVICEADDR[1] << 32) | NRF_FICR->DEVICEADDR[0]; + memcpy(deviceId, &device_id_start, sizeof(device_id_start)); + memcpy(deviceId + sizeof(device_id_start), &device_id_end, sizeof(device_id_end)); + return true; +} + #if !MESHTASTIC_EXCLUDE_BLUETOOTH void setBluetoothEnable(bool enable) { diff --git a/src/platform/nrf54l15/main-nrf54l15.cpp b/src/platform/nrf54l15/main-nrf54l15.cpp index e3e597fc7..b91fe67ee 100644 --- a/src/platform/nrf54l15/main-nrf54l15.cpp +++ b/src/platform/nrf54l15/main-nrf54l15.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include "NodeDB.h" @@ -97,6 +98,17 @@ void getMacAddr(uint8_t *dmac) #endif } +bool getDeviceId(uint8_t *deviceId) +{ + // nRF54L15: DEVICEID under the FICR->INFO sub-struct. Read unconditionally so a future build + // lacking NRF_FICR fails loudly rather than silently sharing getMacAddr()'s placeholder MAC. + uint64_t device_id_start = ((uint64_t)NRF_FICR->INFO.DEVICEID[1] << 32) | NRF_FICR->INFO.DEVICEID[0]; + uint64_t device_id_end = ((uint64_t)NRF_FICR->DEVICEADDR[1] << 32) | NRF_FICR->DEVICEADDR[0]; + memcpy(deviceId, &device_id_start, sizeof(device_id_start)); + memcpy(deviceId + sizeof(device_id_start), &device_id_end, sizeof(device_id_end)); + return true; +} + // ── Bluetooth ───────────────────────────────────────────────────────────────── void setBluetoothEnable(bool enable) diff --git a/src/platform/portduino/PortduinoGlue.cpp b/src/platform/portduino/PortduinoGlue.cpp index 7bb8096ab..3903da2e8 100644 --- a/src/platform/portduino/PortduinoGlue.cpp +++ b/src/platform/portduino/PortduinoGlue.cpp @@ -201,6 +201,16 @@ void getMacAddr(uint8_t *dmac) } } +bool getDeviceId(uint8_t *deviceId) +{ + if (portduino_config.has_device_id) { + memcpy(deviceId, portduino_config.device_id, sizeof(portduino_config.device_id)); + return true; + } + // Config-supplied id stays preferred: host NIC/BT MACs can be unstable (docker, multi-NIC). + return getMacAddrDeviceId(deviceId); +} + std::string cleanupNameForAutoconf(std::string name) { // Convert spaces -> dashes, lowercase diff --git a/src/platform/rp2xx0/main-rp2xx0.cpp b/src/platform/rp2xx0/main-rp2xx0.cpp index ee50f4fb1..cb95f8e84 100644 --- a/src/platform/rp2xx0/main-rp2xx0.cpp +++ b/src/platform/rp2xx0/main-rp2xx0.cpp @@ -1,6 +1,7 @@ #include "HardwareRNG.h" #include "configuration.h" #include "hardware/xosc.h" +#include #include #include #include @@ -98,6 +99,15 @@ void getMacAddr(uint8_t *dmac) dmac[0] = src.id[2]; } +bool getDeviceId(uint8_t *deviceId) +{ + // RP2040/RP2350: 64-bit unique board id (flash serial / OTP) in bytes 0-7 (rest stay zero). + pico_unique_board_id_t board_id; + pico_get_unique_board_id(&board_id); + memcpy(deviceId, board_id.id, sizeof(board_id.id)); + return true; +} + void rp2040Setup() { if (watchdog_caused_reboot()) { diff --git a/src/platform/stm32wl/main-stm32wl.cpp b/src/platform/stm32wl/main-stm32wl.cpp index 86ef47fea..8ec87092b 100644 --- a/src/platform/stm32wl/main-stm32wl.cpp +++ b/src/platform/stm32wl/main-stm32wl.cpp @@ -1,5 +1,6 @@ #include "RTC.h" #include "configuration.h" +#include #include #include #include @@ -77,6 +78,14 @@ void getMacAddr(uint8_t *dmac) dmac[0] = (uint8_t)(uid2 >> 8); } +bool getDeviceId(uint8_t *deviceId) +{ + // STM32WL: 96-bit factory silicon UID (words w0..w2, little-endian) in bytes 0-11 (rest stay zero). + uint32_t uid[3] = {HAL_GetUIDw0(), HAL_GetUIDw1(), HAL_GetUIDw2()}; + memcpy(deviceId, uid, sizeof(uid)); + return true; +} + void cpuDeepSleep(uint32_t msecToWake) {} // Hacks to force more code and data out. diff --git a/src/target_specific.h b/src/target_specific.h index 7404a3720..d7aba018b 100644 --- a/src/target_specific.h +++ b/src/target_specific.h @@ -7,4 +7,8 @@ // Enable/disable bluetooth. void setBluetoothEnable(bool enable); -void getMacAddr(uint8_t *dmac); \ No newline at end of file +void getMacAddr(uint8_t *dmac); + +// Fill deviceId (a caller-zeroed 16-byte buffer) with a stable silicon/factory id; return true, or +// false (buffer untouched) if none exists here. Writes only leading bytes. Per-arch: src/platform//. +bool getDeviceId(uint8_t *deviceId);