MAVLink: refine reconnect STATUSTEXT (per-peer tracking, snapshot rate limit) - #11731
MAVLink: refine reconnect STATUSTEXT (per-peer tracking, snapshot rate limit)#11731xznhj8129 wants to merge 2 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
76e8e3e to
9a4d419
Compare
|
Went through this one closely since it's specifically about reliability of the arming-state notification — a couple of things I noticed that both seem to reproduce the same class of issue this PR is fixing, wanted to check my understanding: Route-table-full peers get no notification at all, not "broadcast-only" fallback The new per-port snapshot floor can suppress a second peer's first-ever snapshot Removing the enable-transition Not trying to block on all of these — mostly want to understand whether the route-table-full and shared-floor cases are known/acceptable tradeoffs before flagging them as gaps. The per-peer keying itself (the core fix for the steady-peer-masking-a-new-one bug) looks correct and clean, and the millis()-wraparound-safe comparisons and the |
Review feedback on the reconnect STATUSTEXT commit: - Track heartbeat presence per peer in the route table instead of one timestamp per port, so a steady peer cannot mask a newly joining one behind the same port, and a peer failing over to another port registers as a reconnect there. If the route table is full the new peer degrades to broadcast-only behavior. - Rate-limit arming snapshots per port so a peer heartbeating slower than the gap threshold cannot elicit a resend on every beat. - Drop the enable-transition snapshot: it fired into a port nobody had connected to yet (boot) and duplicated the first-heartbeat snapshot on shared-port enable. Reconnect is now driven purely by the heartbeat signal; the enable transition still resets per-port one-shot state via the port reset path.
…rdown - Scope the arming-snapshot floor to the peer (route entry) instead of the port. Two peers sharing a port each get their own allowance, so a second peer's first-ever snapshot is no longer swallowed by the first peer's window. mavlinkPortReconnected() becomes mavlinkPeerReconnected() and takes the route it fired for. - Clear heartbeat and snapshot state for peers last heard on a port when that port is torn down. The port's one-shot state is reset there, so a peer resuming inside the gap window must not skip its fresh snapshot. - Say plainly what happens when the route table is full: that peer gets no snapshot at all. The broadcast path is edge-triggered on the arming flags changing, so calling it a "broadcast-only" fallback was wrong.
9a4d419 to
179b548
Compare
|
All three are fair. Two were real, one was a bad comment on my part. Shared floor — you're right, and right that it's the same bug at the delivery stage. Moved lastArmingSnapshotMs into mavlinkRouteEntry_t; mavlinkPortReconnected() is now mavlinkPeerReconnected(portIndex, route). Caught live on SITL, same rig and eeprom, only the firmware differing: peer A connects, peer B heartbeats 4s later on the same port. Pre-fix, B gets nothing (after B joined=0); post-fix it gets its snapshot. The existing rig had B joining at t=12, deliberately past the floor — which is why it passed on the broken code. Enable-transition removal — agreed, nothing invalidated route state on teardown. Added mavlinkForgetHeartbeatsForPort(), called from freeMAVLinkTelemetryPortByIndex(), clearing heartbeat and snapshot state for peers last heard on that port. Worth noting the reachable scope is narrow: determinePortSharing() returns NOT_SHARED for a MAVLink-only UART, so enabled is always true and teardown never runs. On a shared port with telemetry_switch = OFF the gate is ARMING_FLAG(ARMED), and while armed disableFlags == 0, so there's nothing to snapshot. That leaves shared port + telemetry_switch = ON as the only configuration this affects. Fixed anyway — it costs nothing — but I didn't build a rig for it. The path is straight-line and single-threaded, and the SITL/UART difference (TCP drops the client, a real radio keeps transmitting into a dead port) is a hardware question a rig wouldn't settle. Route table full — comment was simply wrong; corrected to say plainly that such a peer gets no snapshot at all, since the broadcast path is edge-triggered on the flags changing. Not adding eviction. LRU by last heartbeat is the obvious key and it's wrong here: routes are learned from any message, so a peer that sends data but never heartbeats sits at lastHeartbeatMs == 0 and looks stalest — eviction would drop an actively-routing peer. Doing it right needs a separate last-seen timestamp on all 32 entries, and I can't construct 32 distinct (sysid,compid) pairs on real hardware. |
|
Test firmware build ready — commit Download firmware for PR #11731 244 targets built. Find your board's
|
|
RAM / Flash usage vs. base branch — commit
See RAM/flash optimization guide for techniques to reduce usage. |
2 participants
Follow-up to the reconnect STATUSTEXT handling merged in #11715, addressing review feedback on the reconnect-detection design.
The original commit tracked heartbeat presence with a single timestamp per physical port and sent an arming-reason snapshot on every judged reconnect. Review raised two soundness issues with that:
Changes
sysid/compid), not per port. A steady peer no longer masks a new one; a peer moving to another port registers as a reconnect there. If the route table is full the new peer degrades gracefully to the pre-existing broadcast-only behaviour.Testing
mavlink_unittest38/38 at this level (no new unit tests — behaviour is timing/multi-peer and covered by the live rig below).Built on the
mav/04-streams-protocolcontent already inmaintenance-10.xvia #11715, so this shows a single commit.