Provide relay node info for outbound packets - #10767
Conversation
@alecperkins, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
github-actions
Bot
added
first-contribution
needs-review
There was a problem hiding this comment.
Pull request overview
This PR extends the routing/phone notification path so clients can learn which nodes relayed a packet (including subsequent relays), by attaching relay identity + link metrics to implicit ACKs for outbound packets and by forwarding relay-info-only duplicates to the phone.
Changes:
- Extend ACK/NAK allocation/sending APIs to optionally carry relay metadata from an observed packet.
- Generate implicit ACK notifications for each overheard rebroadcast of our outbound packets, carrying the relayer + RSSI/SNR + hop fields.
- When a duplicate incoming packet is ignored (not rebroadcast), forward a decoded copy to the phone so it can learn additional relayers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/modules/RoutingModule.h | Extends sendAckNak/allocAckNak to accept an optional relay-source packet. |
| src/modules/RoutingModule.cpp | Threads the new relaySource parameter through to MeshModule::allocAckNak(). |
| src/mesh/Router.h | Extends sendAckNak signature and adds notifyPhoneOfRelayDupe() API for relay-only dupe forwarding. |
| src/mesh/Router.cpp | Implements relay-dupe forwarding by copying, decoding, and sending the packet to the phone. |
| src/mesh/ReliableRouter.cpp | Emits implicit-ack notifications on every overheard rebroadcast of our outbound packets (including later relays). |
| src/mesh/NextHopRouter.cpp | Calls notifyPhoneOfRelayDupe() on duplicate packets so the phone can learn additional relayers. |
| src/mesh/MeshModule.h | Extends allocAckNak() to accept an optional relay-source packet. |
| src/mesh/MeshModule.cpp | Copies relay identity + RSSI/SNR + hop fields into the locally-delivered implicit ACK when relaySource is provided. |
| src/mesh/FloodingRouter.cpp | Calls notifyPhoneOfRelayDupe() on duplicate packets so the phone can learn additional relayers. |
| // Use the channel of our original pending packet when we still have it. Otherwise (a plain | ||
| // broadcast with no pending retransmission, or a subsequent relayer after we already cleared it) | ||
| // the overheard copy only carries the channel hash, not a usable index, so fall back to primary. | ||
| ChannelIndex ackChannel = old ? old->packet->channel : channels.getPrimaryIndex(); |
|
Not sure I love this approach, sending the same packet over the phone api multiple times seems like a problematic pattern, especially for diagnostic only data. |
|
Is there a better way to provide it to the phone? It's info the node already has, that clients may find useful or can ignore if they don't. I wouldn't call it diagnostic exactly, more a revelation of how the mesh functions at a fundamental level. "Who is hearing me?" and "How am I hearing this?" are useful information for users as they move about, or as the mesh changes around them. And it's a FAQ from those getting set up that's often hard to answer. The apps even already seem to expect the outbound relay, it just wasn't working before. Edit: and traceroutes are a special case where the packets are not duplicates. Each transmission carries different and useful information. Path diversity is a critical concept for making a well-functioning mesh, so it's helpful to illustrate it rather than obscure it. |
garthvh
left a comment
garthvh
left a comment
There was a problem hiding this comment.
The need to immediately patch every client to prevent duplicate messages is a no go, this needs to be rethought. I would suggest that this is maybe a routing packet.
@garthvh without the dupe notifications, is the addition of relay characteristics to the ack message acceptable? Being able to known outbound relays is the more important and more frequent feature request. |
|
We can't send duplicate messages, that approach is not going to work. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds an optional ChangesRelay source in ACK/NAK flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ReliableRouter
participant Router
participant RoutingModule
participant MeshModule
ReliableRouter->>Router: sendAckNak(..., relaySource)
Router->>RoutingModule: sendAckNak(..., relaySource)
RoutingModule->>MeshModule: allocAckNak(..., relaySource)
MeshModule-->>RoutingModule: ACK/NAK with relay metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alecperkins
changed the title
Right, I mean the addition of the relay info to the existing implicit ack notice, so the clients can know who gave them the implicit ack. I've stripped out the duplicate portion so this change is only that addition: 6555721. Though I will note clients are already expecting to be notified about duplicates, because it’s a reasonable assumption, and already dedupe by mesh packet ID. Some of them even are relying on this in their UI, so arguably the current firmware behavior is defective from their perspective. For example, the stock Android and Apple apps connected to a node running the now-removed changes: 
Versus the stock Apple and 2.7.15 fw: 
Even though that packet was definitely RX'd at this location multiple times: 
The only change the Apple app needed for the multiple notices was a fix to prevent duplicate push notifications, it otherwise already tolerated and assumed it might hear the same packet repeatedly. |
GUVWAF
left a comment
GUVWAF
left a comment
There was a problem hiding this comment.
The current implementation without duplicated packets looks fine to me.
Some of them even are relying on this in their UI, so arguably the current firmware behavior is defective from their perspective.
This is true, currently you only get the implicit ACK when it's still in the list of "pending packets" (i.e. retransmissions), where it will be deleted after the first rebroadcast is overheard:
firmware/src/mesh/ReliableRouter.cpp
Lines 52 to 54 in 1e7cf4e
Firmware Size Report42 targets | vs
Show 37 more target(s)
Size budgets
Budgets live in Updated for 2b09d81 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/Router.cpp (1)
883-886: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the explanatory comment.
This four-line comment exceeds the repository’s one- or two-line maximum; condense it to the essential rationale or remove details that are apparent from the code. As per coding guidelines: “Keep code comments minimal: one or two lines maximum, only when the reason is not obvious.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/Router.cpp` around lines 883 - 886, Condense the explanatory comment above the post-decode hop_start check to one or two lines, retaining only the essential rationale that unknown hop_start identifies pre-hop firmware packets and local-origin packets are exempt; remove the detailed processing-destination list.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/Router.cpp`:
- Around line 883-886: Condense the explanatory comment above the post-decode
hop_start check to one or two lines, retaining only the essential rationale that
unknown hop_start identifies pre-hop firmware packets and local-origin packets
are exempt; remove the detailed processing-destination list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f56ad487-f4b1-4381-8ae6-3720ea4814ed
📒 Files selected for processing (2)
src/mesh/ReliableRouter.cppsrc/mesh/Router.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mesh/ReliableRouter.cpp
When we overhear another node rebroadcast one of our own packets, we generate an implicit-ack ROUTING packet for the local sending process. That ack is delivered locally to the phone, so pass the overheard rebroadcast as a relay source and copy its relay_node and the rx_rssi / rx_snr we heard it at onto the ack. This lets the connected client see which node relayed our packet and the link quality, instead of only learning that the packet was repeated. allocAckNak / sendAckNak gain an optional relaySource parameter. The ack is sent to ourselves (to == us), so Router::send() is bypassed and does not overwrite these fields with our own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
74a4276 to
6ae0d51
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This provides the relay info in the implicit ack response to phones so the phone can know who relayed the node's own outbound packets.
Apps can then use this to show more details about which infrastructure is servicing the user’s node.
The stock 2.7.15 (2074) meshtastic/Meshtastic-Apple app, for instance:
Client compatibility
🤝 Attestations
Summary by CodeRabbit
New Features
Bug Fixes