Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACK retried want_ack packets #919 #924

Merged
merged 6 commits into from
Nov 29, 2021
Merged

ACK retried want_ack packets #919 #924

merged 6 commits into from
Nov 29, 2021

Conversation

rxt1077
Copy link
Contributor

@rxt1077 rxt1077 commented Nov 23, 2021

This PR addresses Issue #919 by adding some code to ReliableRouter::shouldFilterReceived that ACKs retried packets that would normally just be filtered with the call to FloodingRouter::shouldFilterReceived at the end.

I'll be the first to point out that this isn't great in that it requires decoding the packet to have a channel to respond on meaning the shouldFilterRecieved inherited functions all go from cost MeshPacket * to MeshPacket *. I'd prefer that a filter function doesn't modify the thing passed to it, but I didn't think it warranted making a copy of the packet and there's already some interesting ACK logic in the function anyway.

For what it's worth, I wholeheartedly agree with this comment.

@mc-hamster
Copy link
Member

Approved to run the CI

@mc-hamster
Copy link
Member

Thanks for the PR!

@mc-hamster mc-hamster merged commit 5199b4d into meshtastic:master Nov 29, 2021
@geeksville
Copy link
Member

This pull request has been mentioned on Meshtastic. There might be relevant details there:

https://meshtastic.discourse.group/t/meshtastic-device-1-2-48-alpha/4359/1

@geeksville
Copy link
Member

This pull request has been mentioned on Meshtastic. There might be relevant details there:

https://meshtastic.discourse.group/t/meshtastic-device-1-2-49-5354c49-alpha/4502/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants