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

Fix sending duplicate packets to PhoneAPI/MQTT #5315

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

GUVWAF
Copy link
Member

@GUVWAF GUVWAF commented Nov 11, 2024

Accidentally introduced with #5232.

When receiving repeated packets due to dropped ACKs/replies, we should re-do any routing or sending ACK/replies, but we shouldn’t resend it to the PhoneAPI or MQTT.

To check this, I moved the PacketHistory to the Router class (which is needed for #2856 anyway).

@GUVWAF GUVWAF requested a review from thebentern November 11, 2024 18:48
@fifieldt fifieldt merged commit 40bc04b into meshtastic:master Nov 11, 2024
47 checks passed
@GUVWAF
Copy link
Member Author

GUVWAF commented Nov 12, 2024

@thebentern Just found that in some cases this actually causes it to not send to the PhoneAPI when it actually should. Will try to find a better fix...

GUVWAF added a commit to GUVWAF/Meshtastic-device that referenced this pull request Nov 12, 2024
thebentern pushed a commit that referenced this pull request Nov 12, 2024
* Revert "Fix sending duplicate packets to PhoneAPI/MQTT (#5315)"

This reverts commit 40bc04b.

* Handle repeated packet after potentially canceling previous Tx
@VigibotDev
Copy link

Thanks for solving. Going test :)

@VigibotDev
Copy link

Testing 2.5.13 for duplicate message it's not ok for now. Easy to reproduce for me feel free ask more testing.

Screenshot_20241114_180949_Meshtastic
Screenshot_20241114_180838_Meshtastic

@VigibotDev
Copy link

OK I must update the other side also because it can receive twice also

@VigibotDev
Copy link

Solved for me :)
Screenshot_20241114_183511_Meshtastic
Screenshot_20241114_183834_Meshtastic

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.

4 participants