Extra repeats#10962
Open
NomDeTom wants to merge 9 commits into
Open
Conversation
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (29)
Build artifacts expire on 2026-08-17. Updated for |
Contributor
📝 WalkthroughWalkthroughIntroduces ChangesRepeat Scaling Module
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NextHopRouter
participant RepeatScalingModule
participant FloodingRouter
participant RadioLibInterface
NextHopRouter->>RepeatScalingModule: noteScheduled(sender, id, portnum)
FloodingRouter->>RepeatScalingModule: shouldCancelDupe(packet)
RepeatScalingModule-->>FloodingRouter: cancellation decision
RadioLibInterface->>RepeatScalingModule: getToleratedDupeCount(sender, id)
RepeatScalingModule-->>RadioLibInterface: tolerated duplicate count
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/test_repeat_scaling_module/test_main.cpp (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment blocks exceed the one-to-two-line guideline.
Several explanatory blocks in this file (file header,
ScopedChannelUtil/ScopedAirUtilTxhelper intro, Group 2b header, Group 5 header) run 4–11 lines each. The project's C++ style guideline restricts comments to one or two lines except when the reason truly isn't obvious.As per coding guidelines: "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
Also applies to: 55-60, 241-246, 436-446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_repeat_scaling_module/test_main.cpp` around lines 1 - 10, Shorten the oversized comment blocks in test_main.cpp to meet the one- or two-line guideline by trimming the file header and the explanatory headers around ScopedChannelUtil, ScopedAirUtilTx, Group 2b, and Group 5. Keep only the minimal non-obvious context, remove restatements of the code/tests that follow, and preserve the intent in concise comments near the relevant symbols.Source: Coding guidelines
src/modules/RepeatScalingModule.h (1)
6-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comments exceed the repo's comment-length guideline.
The class-level comment (Lines 6-16) and several method comments (
shouldCancelDupeLines 23-30,noteScheduledLines 33-38,getToleratedDupeCountLines 41-44,getDupeCancelThresholdLines 48-51,registerDupeHeardLines 54-57) run well past the repo's cap. As per coding guidelines,**/*.{c,cc,cpp,cxx,h,hpp,hh}: "Keep code comments minimal: one or two lines... only when the reason is not obvious, and do not restate the next line."✂️ Example trim (class-level comment)
-/** - * RepeatScalingModule owns the "how many duplicate rebroadcasts of a packet we ourselves have - * queued to rebroadcast should we tolerate before giving up" decision (see - * FloodingRouter::perhapsCancelDupe, its sole caller). - * - * The historical behavior is to cancel our own queued rebroadcast as soon as we hear the very - * first duplicate from another node. This module allows some packet types (see - * getDupeCancelThreshold in RepeatScalingModule.cpp) to instead tolerate a configurable number of - * heard duplicates first, trading a little extra airtime for better delivery odds - unless the - * mesh is already busy/dense, in which case it always falls back to the historical behavior. - */ +// Decides how many heard duplicate rebroadcasts to tolerate before cancelling our own queued +// rebroadcast (see FloodingRouter::perhapsCancelDupe and getDupeCancelThreshold).The same trimming applies to the other flagged comments in this range.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/RepeatScalingModule.h` around lines 6 - 66, The doc comments in RepeatScalingModule are too verbose and exceed the repo’s comment-length guideline. Trim the class comment and the method comments for shouldCancelDupe, noteScheduled, getToleratedDupeCount, getDupeCancelThreshold, and registerDupeHeard so each is only one or two short lines, keeping only the essential intent and removing any restatement of the obvious behavior.Source: Coding guidelines
src/modules/RepeatScalingModule.cpp (2)
9-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerbose comments here also exceed the repo's comment-length guideline.
The
meshTooBusyForExtraRepeatsdoc (Lines 19-21) and especially the block comment abovegetDupeCancelThreshold(Lines 48-75, ~28 lines) go well past the cap. As per coding guidelines,**/*.{c,cc,cpp,cxx,h,hpp,hh}: "Keep code comments minimal: one or two lines... only when the reason is not obvious, and do not restate the next line." Consider moving the detailed design rationale into a design doc/ADR and leaving only a terse pointer comment in-line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/RepeatScalingModule.cpp` around lines 9 - 75, The comments in RepeatScalingModule are too verbose and exceed the repository’s comment-length guideline. Trim the doc block around meshTooBusyForExtraRepeats and the explanatory comment near getDupeCancelThreshold to one or two short lines that only state the essential intent, and remove the repeated design rationale. If the detailed threshold behavior matters, move that explanation into a design doc or ADR and leave a brief inline pointer comment in RepeatScalingModule.cpp.Source: Coding guidelines
78-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate portnum-resolution logic.
getDupeCancelThreshold(Lines 78-90) andshouldCancelDupe(Lines 206-207) both independently implementdecoded ? p->decoded.portnum : lookupNotedPortnum(p->from, p->id). Extracting a small private helper avoids the two copies drifting apart later.♻️ Suggested extraction
+int32_t RepeatScalingModule::resolvePortnum(const meshtastic_MeshPacket *p) const +{ + return (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum + : lookupNotedPortnum(p->from, p->id); +} + uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket *p) { - int32_t portnum; - if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { - portnum = p->decoded.portnum; - } else { - portnum = lookupNotedPortnum(p->from, p->id); - } + int32_t portnum = resolvePortnum(p);And in
shouldCancelDupe:- const int32_t portnum = - (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum : lookupNotedPortnum(p->from, p->id); + const int32_t portnum = resolvePortnum(p);Also applies to: 202-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/RepeatScalingModule.cpp` around lines 78 - 90, The portnum resolution logic is duplicated between getDupeCancelThreshold and shouldCancelDupe, so extract the shared decoded-or-lookup behavior into a small private helper in RepeatScalingModule and have both call it. Use the helper to centralize the meshtastic_MeshPacket_decoded_tag check and lookupNotedPortnum(p->from, p->id) fallback so the two paths stay consistent and don’t drift apart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/modules/RepeatScalingModule.cpp`:
- Around line 9-75: The comments in RepeatScalingModule are too verbose and
exceed the repository’s comment-length guideline. Trim the doc block around
meshTooBusyForExtraRepeats and the explanatory comment near
getDupeCancelThreshold to one or two short lines that only state the essential
intent, and remove the repeated design rationale. If the detailed threshold
behavior matters, move that explanation into a design doc or ADR and leave a
brief inline pointer comment in RepeatScalingModule.cpp.
- Around line 78-90: The portnum resolution logic is duplicated between
getDupeCancelThreshold and shouldCancelDupe, so extract the shared
decoded-or-lookup behavior into a small private helper in RepeatScalingModule
and have both call it. Use the helper to centralize the
meshtastic_MeshPacket_decoded_tag check and lookupNotedPortnum(p->from, p->id)
fallback so the two paths stay consistent and don’t drift apart.
In `@src/modules/RepeatScalingModule.h`:
- Around line 6-66: The doc comments in RepeatScalingModule are too verbose and
exceed the repo’s comment-length guideline. Trim the class comment and the
method comments for shouldCancelDupe, noteScheduled, getToleratedDupeCount,
getDupeCancelThreshold, and registerDupeHeard so each is only one or two short
lines, keeping only the essential intent and removing any restatement of the
obvious behavior.
In `@test/test_repeat_scaling_module/test_main.cpp`:
- Around line 1-10: Shorten the oversized comment blocks in test_main.cpp to
meet the one- or two-line guideline by trimming the file header and the
explanatory headers around ScopedChannelUtil, ScopedAirUtilTx, Group 2b, and
Group 5. Keep only the minimal non-obvious context, remove restatements of the
code/tests that follow, and preserve the intent in concise comments near the
relevant symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d84ed1d-f11d-416f-b2cd-9f712644e09a
📒 Files selected for processing (11)
src/mesh/FloodingRouter.cppsrc/mesh/FloodingRouter.hsrc/mesh/NextHopRouter.cppsrc/mesh/RadioLibInterface.cppsrc/modules/HopScalingModule.hsrc/modules/Modules.cppsrc/modules/RepeatScalingModule.cppsrc/modules/RepeatScalingModule.htest/native-suite-counttest/test_flooding_router/test_main.cpptest/test_repeat_scaling_module/test_main.cpp
NomDeTom
added
module
Contributor
|
I wonder if there would be value in using RSSI of a packet in making retransmission decision. For example allow duplicate if heard retransmission was from a node relatively far away (using threshold calculated from RSSI of nearest neighbours). |
Collaborator
Author
I think we tried something like that before. @RCGV1 didn't you have something? |
… own rebroadcast FloodingRouter previously cancelled its own scheduled rebroadcast the instant it heard any duplicate of a packet from another node. Add a small per-(sender, id) ring buffer that counts duplicates heard so far, and a compile-time per-portnum threshold (getDupeCancelThreshold) controlling how many repeats to tolerate before giving up - default of 1 preserves existing behavior everywhere. Enable it for TEXT_MESSAGE_APP/TEXT_MESSAGE_COMPRESSED_APP (threshold 2): no ACK/retry safety net for broadcast chat, and this applies to DM text messages too since NextHopRouter::shouldFilterReceived calls the same inherited perhapsCancelDupe/getDupeCancelThreshold with no addressing-based branching. Adds test/test_flooding_router covering the ring buffer, the default/text- message thresholds, and the perhapsCancelDupe threshold-gating integration. clod helped too
Add meshTooBusyForExtraRepeats() to FloodingRouter: forces getDupeCancelThreshold() back down to 1 (cancel on first duplicate heard) whenever channel utilization is above 10%, TX air utilization is above 4%, or HopScalingModule estimates more than 10 active direct neighbors - regardless of what the per-portnum table says. Spending extra airtime on repeat-tolerant rebroadcasts isn't worth it once the mesh is already congested or dense enough that the packet is probably getting through some other way. Adds a test-only HopScalingModule::setLastPerHopCountsForTest() setter (mirrors the existing PIO_UNIT_TESTING accessors) so tests can inject a specific direct-neighbor count without driving the full histogram/rollover machinery. Extends test/test_flooding_router with the busy-channel-util, busy-air-util-tx, and busy-direct-node-count gate cases (including at-threshold boundary checks), plus a regression test confirming the gate has no effect on portnums that were never extended past 1. clod helped too
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/FloodingRouter.cpp (1)
142-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense overly long comments.
As per coding guidelines, code comments should be kept minimal with a strict limit of one or two lines maximum.
src/mesh/FloodingRouter.cpp#L142-L145: Reduce the 4-line explanatory block to a brief summary (e.g.,// Fall back to cancelling on the first duplicate if RepeatScalingModule is excluded.).src/modules/HopScalingModule.h#L207-L209: Condense the 3-line comment to two lines (e.g.,/// Directly inject the last-rollover per-hop counts for tests, bypassing the histogram/rollHour() machinery.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/FloodingRouter.cpp` around lines 142 - 145, The comments at src/mesh/FloodingRouter.cpp lines 142-145 and src/modules/HopScalingModule.h lines 207-209 are overly long. Condense the FloodingRouter comment to one brief line describing first-duplicate cancellation when RepeatScalingModule is excluded, and reduce the HopScalingModule comment to no more than two lines while preserving its test-injection and bypass behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/FloodingRouter.cpp`:
- Around line 142-145: The comments at src/mesh/FloodingRouter.cpp lines 142-145
and src/modules/HopScalingModule.h lines 207-209 are overly long. Condense the
FloodingRouter comment to one brief line describing first-duplicate cancellation
when RepeatScalingModule is excluded, and reduce the HopScalingModule comment to
no more than two lines while preserving its test-injection and bypass behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e43afe8c-a810-4bcd-a7bf-a85131ce2f0e
📒 Files selected for processing (11)
src/mesh/FloodingRouter.cppsrc/mesh/FloodingRouter.hsrc/mesh/NextHopRouter.cppsrc/mesh/RadioLibInterface.cppsrc/modules/HopScalingModule.hsrc/modules/Modules.cppsrc/modules/RepeatScalingModule.cppsrc/modules/RepeatScalingModule.htest/native-suite-counttest/test_flooding_router/test_main.cpptest/test_repeat_scaling_module/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- test/native-suite-count
- src/modules/Modules.cpp
- src/mesh/NextHopRouter.cpp
- src/modules/RepeatScalingModule.h
- src/mesh/FloodingRouter.h
- test/test_flooding_router/test_main.cpp
- src/modules/RepeatScalingModule.cpp
- test/test_repeat_scaling_module/test_main.cpp
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request introduces the new
RepeatScalingModule, which refines how the mesh network decides when to cancel scheduled rebroadcasts of duplicate packets. Instead of always cancelling on the first duplicate (the historical behavior), the module allows for more nuanced, per-packet-type tolerance, improving delivery odds without wasting airtime on busy networks. The change is integrated across the codebase, with supporting logic and hooks in several routers and modules.Repeat scaling logic and integration:
RepeatScalingModule(src/modules/RepeatScalingModule.cpp,src/modules/RepeatScalingModule.h) to track and decide how many duplicate rebroadcasts to tolerate before cancelling, with per-portnum thresholds and busy-mesh detection. [1] [2]RepeatScalingModuleintoFloodingRouter,NextHopRouter, andRadioLibInterfaceto delegate dupe-cancel logic and to log rebroadcast decisions. [1] [2] [3] [4] [5] [6]RepeatScalingModulein the module setup routine, with proper conditional compilation support. [1] [2]Testing and developer support:
HopScalingModuleto allow direct injection of per-hop counts, supporting tests for the new repeat scaling logic.These changes make rebroadcast cancellation more adaptive and robust, especially in varying mesh network conditions.
🤝 Attestations
Pro-micro
Summary by CodeRabbit