Skip to content

fix(mqtt): publish one HA device per node id (refs #872)#899

Open
mschabhuettl wants to merge 4 commits into
ruvnet:mainfrom
mschabhuettl:fix/per-node-mqtt-discovery
Open

fix(mqtt): publish one HA device per node id (refs #872)#899
mschabhuettl wants to merge 4 commits into
ruvnet:mainfrom
mschabhuettl:fix/per-node-mqtt-discovery

Conversation

@mschabhuettl
Copy link
Copy Markdown

@mschabhuettl mschabhuettl commented Jun 1, 2026

Problem

With multiple ESP32 nodes feeding a single sensing-server, the MQTT
integration publishes one shared Home-Assistant device
(wifi_densepose_wifi-densepose-1) for all of them — contradicting
docs/integrations/home-assistant.md, which states the integration creates one
device per node.

Verified on ghcr.io/ruvnet/wifi-densepose:latest (post-#872, image created
2026-05-31T15:44:26Z): with two distinct nodes powered, only
wifi-densepose-1 ever appears in discovery, and the broker shows a single
client wifi-densepose-1 with entities=20.

Root cause

The per-node data already exists internally — VitalsSnapshot carries
node_id, and the REST /api/v1/nodes endpoint separates nodes correctly.
The MQTT path is what collapses them:

  • main.rs builds one OwnedDiscoveryBuilder with a fixed
    node_id = client_id, bridges the aggregate sensing broadcast, and
    stamps that same id onto every snapshot.
  • publisher.rs::run holds that single builder for its whole lifetime and
    never reads snap.node_id.

Result: N physical nodes → 1 HA device.

The discovery/topic layer is already fully node-parametric
(DiscoveryBuilder formats every topic as wifi_densepose_{node_id}/...), so
the fix is confined to the publisher loop and the bridge that feeds it.

Fix

  • src/mqtt/publisher.rs: maintain a HashMap<node_id, NodeEntry>.
    Register a new HA device lazily (retained discovery + availability) the first
    time a node id is seen on the stream; route each snapshot to its own node's
    builder; per-node rate limiters; re-publish discovery per node on
    refresh/reconnect; publish offline for every known node on shutdown.
  • src/main.rs: fan out the sensing broadcast per node — emit one
    VitalsSnapshot per entry of the nodes[] array, preserving real ids,
    instead of stamping a single fixed id. Presence is mapped as
    motion_level != absent && status == active.

Result

N nodes surface as N HA devices (wifi_densepose_1wifi_densepose_N),
each with its own presence / person-count / motion entities. New nodes are
auto-registered the first time they appear on the stream. Without a friendly
name they show as "RuView node "; a future --mqtt-zone-map flag can map
ids to room names.

Testing

  • A CI workflow (.github/workflows/pr-check.yml) builds the crate with
    --features mqtt on every push to verify compilation. (Added for
    verification; happy to drop it before merge if you prefer.)
  • Live: two nodes powered → two distinct discovery devices.

Open question for review

The per-node fan-out in main.rs reads v["nodes"] from the sensing
broadcast, assuming the same shape as REST /api/v1/nodes
(node_id / status / motion_level / person_count / rssi_dbm).
If that broadcast does not carry the per-node array, the bridge should instead
be fed from the node_states map the REST handler already uses. Happy to
adjust based on review.

Refs #872. Fixes #898

Copilot AI review requested due to automatic review settings June 1, 2026 14:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds per-node Home Assistant (HA) discovery/publishing by deriving MQTT discovery identity and topics from each inbound VitalsSnapshot.node_id, allowing multiple physical nodes (rooms) to appear as separate HA devices.

Changes:

  • Add per-node publisher state (HashMap<node_id, NodeEntry>) with independent availability + rate limiting and lazy discovery/availability registration on first sight.
  • Update reconnect/refresh/heartbeat flows to operate across all known nodes.
  • Change main MQTT snapshot fan-out to emit one VitalsSnapshot per element in the inbound nodes array, preserving per-node ids.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
v2/crates/wifi-densepose-sensing-server/src/mqtt/publisher.rs Introduces per-node discovery identities + per-node availability/rate limiting and updates run loop logic accordingly.
v2/crates/wifi-densepose-sensing-server/src/main.rs Switches MQTT publisher builder to a template and fans out inbound JSON into per-node VitalsSnapshots.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread v2/crates/wifi-densepose-sensing-server/src/main.rs Outdated
Comment thread v2/crates/wifi-densepose-sensing-server/src/main.rs
Comment thread v2/crates/wifi-densepose-sensing-server/src/mqtt/publisher.rs
Comment thread v2/crates/wifi-densepose-sensing-server/src/mqtt/publisher.rs
- main.rs: parse node_id as number-or-string; clamp person_count to u32
- publisher.rs: document unbounded node-map (no eviction); tidy lazy register
@mschabhuettl mschabhuettl force-pushed the fix/per-node-mqtt-discovery branch from 28d5d4c to 0de296f Compare June 1, 2026 14:28
ruvnet added a commit that referenced this pull request Jun 2, 2026
) (#918)

The MQTT bridge fanned out one Home-Assistant device per node (#898) but
applied the *room-level aggregate* classification to every node — so in a
multi-node setup a node in an empty corner inherited another node's
"present", and `motion_level: "absent"` was mis-mapped to full motion
(the aggregate match fell through `Some(_) => 1.0`).

Each node in the sensing broadcast's `nodes` array already carries its own
`classification` (`motion_level`/`presence`/`confidence`, see
PerNodeFeatureInfo) and RSSI. Now each per-node snapshot reads that node's
own classification, deferring to the room aggregate only for fields a node
omits. Vitals (breathing/heart rate) and person count stay room-level.

Extracted the JSON→VitalsSnapshot mapping into a pure, testable function
(`vitals_snapshots_from_sensing_json`) and added 4 unit tests covering
per-node divergence, partial-field fallback, the no-nodes aggregate path,
and the absent→zero-motion fix.

Supersedes #899, which targeted the right bug but read non-existent fields
(`node["motion_level"]` / `node["status"]` instead of the nested
`node["classification"]` + `stale`).

Verified: builds with `--features mqtt`; new tests pass; full crate unit
suite 432 + 114 passed, 0 failed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants