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

Set high priority for text messages #4592

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

GUVWAF
Copy link
Member

@GUVWAF GUVWAF commented Aug 30, 2024

Use the new HIGH priority for text messages in the Tx queue. Fixes #4576.

When working on this, I saw that fixPriority() was called in MeshPacketQueue::enqueue(), but that is always after encryption, while it was checking p->decoded without checking the payload variant. Not sure why this never failed, but I think it wouldn't set the ACK priority correctly before. I therefore moved it to before encryption (confirmed there's no other MeshPacketQueue than the txQueue).

Also move `fixPriority()` to before encryption
@garthvh
Copy link
Member

garthvh commented Aug 30, 2024

Closes #4576

@thebentern thebentern merged commit eb071ec into meshtastic:master Aug 30, 2024
104 checks passed
@GUVWAF GUVWAF deleted the textPrio branch August 31, 2024 13:27
@gitbisector
Copy link
Contributor

Happy to hear you are taking another look at this otherwise boring piece of code too.

Been profiling TX queue occupancy and how long messages sit in there. Upgraded to master this morning including this change. Can see priority 10, 64, 70, 100 and 120 in use now. Still, some messages get stuck for more than 15 minutes while others of same priority bypass it. Started with measuring through msg counts rather than time in my graphs as GUVWAF noted time might stay at 0.
image

I do have working code that uses stable same-priority order without adding any storage but can't use the heap implementation with that. As station under my control does have rx_time I'll use that to do before/after measurements.

@gitbisector
Copy link
Contributor

BTW, expect your commit that gives proper priority to ack and raises text message above telemetry is going to improve user experience a bunch.

@GUVWAF
Copy link
Member Author

GUVWAF commented Sep 1, 2024

Thanks for the analysis, that's interesting. I might have underestimated how big the influence of this is in really busy meshes. Would be nice to have a cost-effective solution, and I'm wondering if we should make more priority levels for certain messages, e.g. responses to requests.

@gitbisector
Copy link
Contributor

Have some log proof now this is backwards. Even says: return 'true' if pi1 is ordered before p2.

`/// @return "true" if "p1" is ordered before "p2"
bool CompareMeshPacketFunc(const meshtastic_MeshPacket *p1, const meshtastic_MeshPacket *p2)
{
assert(p1 && p2);
auto p1p = getPriority(p1), p2p = getPriority(p2);

// If priorities differ, use that
// for equal priorities, order by id (older packets have higher priority - this will briefly be wrong when IDs roll over but
// no big deal)
return (p1p != p2p)
           ? (p1p < p2p)                                                 // prefer bigger priorities
           : ((p1->id & ID_COUNTER_MASK) >= (p2->id & ID_COUNTER_MASK)); // Mask to counter portion, prefer smaller packet ids

}`

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.

[Feature Request]: Always Prioritize Text Messages
4 participants