Improve ACK logic for responses and repeated packets #5232
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thebentern
approved these changes
Nov 3, 2024
caveman99
pushed a commit
that referenced
this pull request
Nov 3, 2024
* Don't send ACKs to responses over multiple hops * Move repeated sending logic to `wasSeenRecently()` * Add exception for simulator for duplicate packets from PhoneAPI * Add short debug message
This was referenced Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Responses to packets sent with
want_ackwill also usewant_ack:firmware/src/mesh/MeshModule.cpp
Line 228 in bee474e
A consequence of this is that we were also sending a real ACK (over multiple hops) again when receiving a response, which is unnecessary as the responder doesn't do anything with it. It only needs to stop retransmissions, which it does when receiving an implicit ACK already. Only if receiving it directly (so nobody created an implicit ACK yet), we have to send an ACK with hop limit of 0. I had to rewrite the hop limit logic for
sendAckNak()to do this.Next, there was logic to rebroadcast again in case a node was retransmitting a reliable packet, e.g., because the ACK got lost, but I found it would immediately cancel it again when it was checking for
wasSeenRecently()afterwards. Now with 0c763af, we do as if the packet was not yet received if we hear the original transmitter repeating it.Lastly, I added an exception to ignoring duplicate packets from the PhoneAPI when using the simulator.