fix(esp32): use runtime node_id from NVS in outgoing packets#232
fix(esp32): use runtime node_id from NVS in outgoing packets#232melodykke wants to merge 2 commits intoruvnet:mainfrom
Conversation
- 为 Rust sensing server 增加空间布局配置入口,支持加载节点位置与语义区域定义,并扩展空间融合解释输出。 - 在 Docker 镜像中打包 config 目录,确保部署后可直接读取 spatial-layout.json。 - 修正 ESP32 显示界面中的节点编号来源,改为使用运行时 NVS 配置而非编译期常量。
ruvnet
left a comment
There was a problem hiding this comment.
Code Review — PR #232
Recommendation: HOLD (partially superseded, scope creep)
Summary
This PR from @melodykke has two distinct parts:
Part 1 — Firmware: NVS runtime node_id (the stated purpose)
Replaces CONFIG_CSI_NODE_ID with g_nvs_config.node_id across 4 firmware files:
csi_collector.c— packet serializationdisplay_ui.c— UI node labeledge_processing.c— compressed frame and vitals packetswasm_runtime.c— WASM output packets
Part 2 — Sensing server: Multi-node spatial fusion (~800 lines, unstated)
Adds an entire spatial fusion system to the sensing server:
SpatialLayoutConfigwith JSON node positions and zone definitions- Per-node
Esp32NodeStatewith independent signal processing pipelines - Zone-based presence/motion/vital scoring with cross-node consensus
FusionConsensusandFusionExplanationstructsconfig/spatial-layout.jsonspatial layout file- Docker config update
Security Assessment
- No security vulnerabilities found. The
extern nvs_config_t g_nvs_configpattern is safe — it is initialized before use inapp_main. - Path concern:
spatial-layout.jsonis loaded from disk via--spatial-configflag — no user-facing path traversal risk since it is a CLI argument, not a web endpoint. - The
adaptive_overriderefactor correctly decouples fromAppStateInner, improving testability.
Conflict Assessment (ADR-069 through ADR-078)
- Part 1 (NVS node_id) is already on main. Commit
8a84748a8("fix(firmware): use NVS node_id instead of Kconfig constant") already made these exact same changes. Merging would conflict. - Part 2 (spatial fusion) will conflict heavily with
sensing-server/src/main.rson main, which has had significant changes through ADR-069 (Cognitum pipeline), ADR-070+, and the recentv0.5.3cross-node fusion work.
Code Quality Notes (Part 2)
- Good: Per-node state isolation prevents cross-contamination of temporal features between ESP32 nodes.
- Good: Zone scoring with geometric weighting is a sound approach.
- Concern: ~800 lines of new code added to an already large
main.rs(3000+ lines). This should be extracted into separate modules (spatial.rs,fusion.rs). - Concern: The
fuse_esp32_updatefunction is doing too much — it combines filtering, scoring, consensus, and explanation generation. Should be decomposed.
Recommendation
Part 1 is already merged. Part 2 introduces valuable multi-node fusion but:
- Conflicts extensively with current main
- Needs to be rebased onto current main
- Should be split into a separate PR with proper module decomposition
- Should be reviewed against our existing cross-node fusion work (v0.5.3, ADR-069)
Verdict: HOLD — Ask contributor to rebase and split the sensing-server changes into a separate PR.
|
Thanks @melodykke — the NVS node_id fix is already on main (8a84748). The multi-node spatial fusion concept is interesting but conflicts extensively with v0.5.3-v0.5.5 changes (cross-node fusion, ADR-069 Seed pipeline, ADR-075 MinCut person counting). Could you rebase and split the spatial fusion into a separate PR targeting the sensing-server only? Happy to review a rebased version. |
Summary
Fix an ESP32 firmware issue where outgoing packets could use the compile-time node ID instead of the runtime node ID loaded from NVS.
In multi-node setups, this could cause a provisioned board to still appear with the wrong
node_idon the receiver side.Root Cause
Some packet serialization paths were still using
CONFIG_CSI_NODE_IDinstead ofg_nvs_config.node_id.That meant NVS provisioning could succeed while transmitted packets still carried the compile-time default.
Changes
Use
g_nvs_config.node_idconsistently in outgoing packet paths.Validation
Tested with a real ESP32-S3 setup.
node_id=2via NVSnode_id=2correctlyImpact
This makes runtime node provisioning behave correctly for multi-node deployments.