Add list of text message packet IDs, and check for dupes#9180
Conversation
|
So we are currently seeing duplicates of text messages that are not in the PacketHistory anymore? |
|
@GUVWAF Yes. Both through higher hop counts, and in the new store and forward++ tests. It seems to only be text messages that are causing the problem, so I went for the simple solution. |
|
I'm wondering whether it's really only text messages that cause this, or whether it's just only noticeable for text messages. Not so long time ago we moved to a static memory allocation for the PacketHistory and it might be we just need to increase its size. PacketHistory is now also used for routing decisions (using the next_hop and relay_node bytes). |
Store and Forward ++ is limited to text messages, so half of the cause is only texts.
Something I noticed about PacketHistory is that we seem to have 4 PacketHistory pools: one for each class that inherits from Router, unless I'm misunderstanding how the class inheritance works. And at that, it looks like only flooding router and next hop router actually write to their pools. My original idea was to make this a lighter message store (only 4 bytes per message) that would grab text messages when they fell off the end of the PacketHistory. But with the split between the two router types, that became a pretty complicated task. |
The better approach perhaps would be to ensure that we have a global singleton packethistory to consolidate these rather than add the complexity of another one? |
Hmm, I see. However, considering the existing issue #8790, which seems to be caused by
No, we only have one router object, which is the most-derived class, namely ReliableRouter: Line 805 in 9f5170a |
Ah-hah! That was the detail I was missing. Regardless, SF++ will deliver missed text messages up to an hour later, so some sort of dedicated text message pool seems reasonable. |
Yes, fair enough. Then in general this looks good to me. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to add duplicate detection for text message packets by maintaining a separate list of recently seen packet IDs in the TextMessageModule and checking this list in FloodingRouter to avoid retransmitting duplicate text packets.
- Adds a rolling buffer of 50 packet IDs to track recently seen text messages
- Implements a
recentlySeen()method to check if a packet ID exists in the tracking list - Integrates the duplicate check into FloodingRouter's packet filtering logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/modules/TextMessageModule.h | Adds constant for list size, public recentlySeen() method, and private members for tracking packet IDs |
| src/modules/TextMessageModule.cpp | Implements packet ID tracking in handleReceived() and duplicate detection in recentlySeen() |
| src/mesh/FloodingRouter.cpp | Integrates text message duplicate check into the existing packet filtering logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!seenRecently && !wasUpgraded && textMessageModule) { | ||
| seenRecently = textMessageModule->recentlySeen(p->id); | ||
| } | ||
|
|
There was a problem hiding this comment.
This duplicate detection logic has a critical order-of-operations bug. The check for textMessageModule->recentlySeen(p->id) happens in shouldFilterReceived, which is called BEFORE modules receive the packet via handleReceived. This means the packet ID is checked against the list before it's added to the list in TextMessageModule::handleReceived, so this will never detect duplicates correctly.
The packet ID needs to be added to the tracking list before the duplicate check occurs, or the duplicate check needs to happen after the packet has been processed by the module.
| if (!seenRecently && !wasUpgraded && textMessageModule) { | |
| seenRecently = textMessageModule->recentlySeen(p->id); | |
| } |
| bool TextMessageModule::recentlySeen(uint32_t id) | ||
| { | ||
| for (size_t i = 0; i < TEXT_PACKET_LIST_SIZE; i++) { | ||
| if (textPacketList[i] != 0 && textPacketList[i] == id) { |
There was a problem hiding this comment.
The recentlySeen check skips packet ID 0, but the array is initialized with zeros. This means the first packet with ID 0 will never be detected as a duplicate, and empty slots in the array will incorrectly fail to match against actual packet ID 0. While packet ID 0 may be uncommon, if it's a valid packet ID, this logic should handle it correctly. Consider using a sentinel value like UINT32_MAX to indicate empty slots, or tracking the number of valid entries separately.
| uint32_t textPacketList[TEXT_PACKET_LIST_SIZE] = {0}; | ||
| size_t textPacketListIndex = 0; |
There was a problem hiding this comment.
This implementation duplicates the packet history tracking logic that already exists in PacketHistory class. The codebase already has a comprehensive packet history system with wasSeenRecently that tracks sender + packet ID combinations. Consider leveraging the existing infrastructure instead of maintaining a separate tracking list specifically for text messages, which could lead to inconsistencies and maintenance burden.
| bool TextMessageModule::recentlySeen(uint32_t id) | ||
| { | ||
| for (size_t i = 0; i < TEXT_PACKET_LIST_SIZE; i++) { | ||
| if (textPacketList[i] != 0 && textPacketList[i] == id) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate check only considers packet ID without considering the sender. This means if two different nodes happen to generate the same packet ID, one will be incorrectly filtered as a duplicate. The existing PacketHistory system correctly tracks both sender and packet ID to avoid this issue. Consider checking both p->from and p->id to ensure you're detecting true duplicates.
|
@GUVWAF Had a thought about this last night: Should we check the new message list first, so as not to prevent these rebroadcasts from poisoning the PacketHistory? |
|
I think it's better to have them in the PacketHistory first, as we now store things like |
Yes, that's what I mean. If these are strangely routed packets coming from the wrong direction, we really don't want them influencing next-hop routing decisions. |
…9180) Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
I think the Copilot review was only requested after this PR was merged. From what I understand with my limited view, it seems the copilot comments above, one of you should have a look again. What do you think of the comments, especially the one on order-of-operations? |
…9180) Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Adds a trivial list of seen packet IDs in the text message module, and then checks for a matching packet ID in FloodingRouter, to avoid retransmitting old text packets.