feat: Refactor roles to helper functions where possible. Fixed issues where found with reasioning. by h3lix1 · Pull Request #9804 · meshtastic/firmware

h3lix1 · 2026-03-04T05:16:52Z

Summary

Centralizes scattered device role comparisons into reusable inline helpers in meshUtils.h, replacing duplicated role-check patterns across multiple files. Fixes one magic number, multiple missing role variants, broadens several ROUTER-only checks to cover ROUTER_LATE (and in some cases CLIENT_BASE) where the original restriction was an oversight, adds semantic grouping helpers for sensor/tracker/TAK roles, and adds single-value helpers for complete coverage. Also fixes a bug where ROUTER_LATE was missing from early rebroadcast logic, and aligns Canned Message auto-favoriting with router-like role semantics.

NOTE: This was assisted with the use of both Claude Opus 4.6 and GPT 5.3-Codex. The logic changes are all mine.

Helpers in src/meshUtils.h:

Helper	Roles matched	Call sites
`isRouterRole(role)`	`ROUTER`, `ROUTER_LATE`	12 across 9 files
`isRouterLikeRole(role)`	`ROUTER`, `ROUTER_LATE`, `CLIENT_BASE`	5 across 5 files
`isTrackerRole(role)`	`TRACKER`, `TAK_TRACKER`	6 across 1 file
`isSensorRole(role)`	`SENSOR`	14 across 5 files
`isTrackerOrSensorRole(role)`	`TRACKER`, `TAK_TRACKER`, `SENSOR`	5 across 5 files
`isTakLikeRole(role)`	`TAK`, `TAK_TRACKER`	1 across 1 file
`isSensorOrRouterRole(role)`	`SENSOR`, `ROUTER`, `ROUTER_LATE`	7 across 6 files (via `isSensorOrRouter()` class wrapper)
`isClientMuteRole(role)`	`CLIENT_MUTE`	1 across 1 file
`isClientHiddenRole(role)`	`CLIENT_HIDDEN`	2 across 2 files
`isClientBaseRole(role)`	`CLIENT_BASE`	4 across 3 files
`isLostAndFoundRole(role)`	`LOST_AND_FOUND`	3 across 2 files
`isClientRole(role)`	`CLIENT`, `CLIENT_MUTE`, `CLIENT_HIDDEN`, `CLIENT_BASE`	1 across 1 file

Changes

#	File	What changed	Helper used	Functional change?
1	`src/meshUtils.h`	Added `config.pb.h` include and 3 inline helper functions	—	No
2	`src/mesh/Router.cpp:88`	`IS_ONE_OF(role, ROUTER, ROUTER_LATE, CLIENT_BASE)` → `isRouterLikeRole(role)`	`isRouterLikeRole`	No
3	`src/mesh/Router.cpp:112`	`!IS_ONE_OF(node->user.role, ROUTER, ROUTER_LATE, CLIENT_BASE)` → `!isRouterLikeRole(static_cast<>(node->user.role))`	`isRouterLikeRole`	No
4	`src/mesh/FloodingRouter.cpp:103`	`role == ROUTER		role == ROUTER_LATE`→`isRouterRole(role)`
5	`src/modules/AdminModule.cpp:315`	`isOneOf(role, CLIENT_BASE, ROUTER, ROUTER_LATE)` → `isRouterLikeRole(role)`	`isRouterLikeRole`	No
6	`src/modules/AdminModule.cpp:680`	`role == ROUTER		role == ROUTER_LATE`→`isRouterRole(role)`
7	`src/modules/PositionModule.cpp:40`	`role != TRACKER && role != TAK_TRACKER` → `!isTrackerRole(role)`	`isTrackerRole`	No
8	`src/modules/PositionModule.cpp:46`	`role == TRACKER		role == TAK_TRACKER`→`isTrackerRole(role)`
9	`src/modules/PositionModule.cpp:381`	`role == TRACKER		role == TAK_TRACKER`→`isTrackerRole(role)`
10	`src/PowerFSM.cpp:374`	`isTrackerRole(role)		role == SENSOR`→`isTrackerOrSensorRole(role)`
11	`src/main.cpp:720`	`isTrackerRole(role)		role == SENSOR`→`isTrackerOrSensorRole(role)`
12	`src/modules/CannedMessageModule.cpp:1076`	Magic number `12` → `meshtastic_Config_DeviceConfig_Role_CLIENT_BASE`	—	No (value 12 = CLIENT_BASE)
13	`src/modules/NodeInfoModule.cpp:99`	`role != TRACKER && role != SENSOR` → `!isTrackerOrSensorRole(role)`	`isTrackerOrSensorRole`	Yes
14	`src/PowerFSM.cpp:41,265`	`role == ROUTER` → `isRouterRole(role)`	`isRouterRole`	Yes
15	`src/mesh/Default.h:44`	`IF_ROUTER` macro: `role == ROUTER` → `isRouterRole(role)`	`isRouterRole`	Yes
16	`src/mesh/Default.cpp:41,45`	`role == ROUTER` → `isRouterRole(role)`; `TRACKER, SENSOR` → `isTrackerOrSensorRole(role)`	`isRouterRole`, `isTrackerOrSensorRole`	Yes
17	`src/mesh/MeshService.cpp:299`	`role == ROUTER` → `isRouterLikeRole(role)`	`isRouterLikeRole`	Yes
18	`src/modules/AdminModule.cpp:653`	`role == ROUTER` → `isRouterRole(role)`	`isRouterRole`	Yes
19	`src/sleep.cpp:549`	`role == ROUTER` → `isRouterRole(role)`	`isRouterRole`	Yes
20	`src/modules/PositionModule.cpp:380`	`role == TRACKER` → `isTrackerRole(role)`	`isTrackerRole`	Yes
21	`src/meshUtils.h`	Added `isSensorRole`, `isTrackerOrSensorRole`, `isTakLikeRole`, `isSensorOrRouterRole` helpers	—	No
22	`src/modules/Telemetry/BaseTelemetryModule.h`	Replaced class method `isSensorOrRouterRole()` with `isSensorOrRouter()` delegating to free function in meshUtils.h	`isSensorOrRouterRole`	Yes (now includes `ROUTER_LATE`)
23	`src/modules/Telemetry/DeviceTelemetry.cpp`	`isSensorOrRouterRole()` → `isSensorOrRouter()` (5 telemetry files)	`isSensorOrRouterRole`	Yes (now includes `ROUTER_LATE`)
24	`src/modules/Telemetry/{Health,Environment,Power,AirQuality}Telemetry.cpp`	`role == SENSOR` / `role != SENSOR` → `isSensorRole(role)` / `!isSensorRole(role)`	`isSensorRole`	No
25	`src/modules/SerialModule.cpp:317`	`role == SENSOR` → `isSensorRole(role)`	`isSensorRole`	No
26	`src/modules/Modules.cpp:152`	`role == TAK		role == TAK_TRACKER`→`isTakLikeRole(role)`
27	`src/modules/PositionModule.cpp:429`	`role != TRACKER && role != TAK_TRACKER` → `!isTrackerRole(role)`	`isTrackerRole`	No
28	`src/platform/nrf52/main-nrf52.cpp:450`	`IS_ONE_OF(TRACKER, TAK_TRACKER, SENSOR)` → `isTrackerOrSensorRole(role)`	`isTrackerOrSensorRole`	No
29	`src/mesh/NodeDB.cpp:670`	`role != ROUTER` → `!isRouterRole(role)`	`isRouterRole`	Yes
30	`src/modules/StoreForwardModule.cpp:591`	`role == ROUTER` → `isRouterRole(role)`	`isRouterRole`	Yes
31	`src/mesh/RadioInterface.cpp:612`	Reverted — kept as `role == ROUTER` (ROUTER_LATE intentionally excluded)	—	No
32	`src/meshUtils.h`	Added `isClientMuteRole`, `isClientHiddenRole`, `isClientBaseRole`, `isLostAndFoundRole`, `isClientRole` helpers	—	No
33	`src/mesh/FloodingRouter.cpp:139`	`role != CLIENT_MUTE` → `!isClientMuteRole(role)`	`isClientMuteRole`	No
34	`src/mesh/FloodingRouter.cpp:109`	`role == CLIENT_BASE` → `isClientBaseRole(role)`	`isClientBaseRole`	No
35	`src/mesh/FloodingRouter.cpp:131`	`role == CLIENT_BASE` → `isClientBaseRole(role)`	`isClientBaseRole`	No
36	`src/modules/NodeInfoModule.cpp:207`	`role != CLIENT_HIDDEN` → `!isClientHiddenRole(role)`	`isClientHiddenRole`	No
37	`src/modules/CannedMessageModule.cpp:1077`	`role != CLIENT_BASE` → `!isRouterLikeRole(role)`	`isRouterLikeRole`	Yes
38	`src/modules/AdminModule.cpp:108`	`role == CLIENT_BASE` → `isClientBaseRole(role)`	`isClientBaseRole`	No
39	`src/modules/Telemetry/DeviceTelemetry.cpp:31`	`role != CLIENT_HIDDEN` → `!isClientHiddenRole(role)`	`isClientHiddenRole`	No
40	`src/modules/PositionModule.cpp:291`	`role != LOST_AND_FOUND` → `!isLostAndFoundRole(role)`	`isLostAndFoundRole`	No
41	`src/modules/PositionModule.cpp:449`	`role == LOST_AND_FOUND` → `isLostAndFoundRole(role)`	`isLostAndFoundRole`	No
42	`src/graphics/Screen.cpp:1854`	`IS_ONE_OF(CLIENT, CLIENT_MUTE, CLIENT_HIDDEN, CLIENT_BASE)` → `isClientRole(role)`	`isClientRole`	No
43	`src/modules/PositionModule.cpp:392`	`IS_ONE_OF(TRACKER, TAK_TRACKER)` → `isTrackerRole(role)`	`isTrackerRole`	No
44	`src/mesh/NodeDB.cpp:582`	`IS_ONE_OF(ROUTER, ROUTER_LATE, LOST_AND_FOUND)` → `isRouterRole() \|\| isLostAndFoundRole()`	`isRouterRole`, `isLostAndFoundRole`	No
45	`src/mesh/NodeDB.cpp:1785`	`role == CLIENT_BASE` → `isClientBaseRole(role)`	`isClientBaseRole`	No

Functional change details

#13 — NodeInfoModule.cpp `requestWantResponse` (line 99)

	Before	After
Code	`role != TRACKER && role != SENSOR`	`!isTrackerOrSensorRole(role)`
Excluded roles	`TRACKER`, `SENSOR`	`TRACKER`, `TAK_TRACKER`, `SENSOR`
Effect	`TAK_TRACKER` nodes would set `want_response = true`, soliciting NodeInfo replies	`TAK_TRACKER` nodes now skip `want_response`, matching `TRACKER` behavior

Rationale: TAK_TRACKER is a tracker variant (enum value 10) and should behave like TRACKER (enum value 5) for NodeInfo response solicitation. The original code predates the TAK_TRACKER role and was not updated when it was added. This is a bug fix — tracker-class devices are low-power and should not solicit unnecessary responses.

#14 — PowerFSM.cpp `isPowered()` (line 41) and `PowerFSM_setup()` (line 265)

	Before	After
Code	`role == ROUTER`	`isRouterRole(role)`
Affected roles	`ROUTER` only	`ROUTER`, `ROUTER_LATE`
Effect	`ROUTER_LATE` was not treated as power-saving and would not wake to NB (no-bluetooth) state	`ROUTER_LATE` now assumed power-saving and wakes to NB like `ROUTER`

Rationale: ROUTER_LATE is an infrastructure relay role with the same deployment profile as ROUTER — typically on solar or always-on power sources. installRoleDefaults already sets is_unmessagable = true and telemetry.device_update_interval = ONE_DAY for ROUTER_LATE, confirming it is an infrastructure node. There is no reason for it to behave differently for power-state assumptions or bluetooth wake transitions. The only intentional difference between ROUTER and ROUTER_LATE is rebroadcast timing, not power management.

#15 — Default.h `IF_ROUTER` macro (line 44)

	Before	After
Code	`role == ROUTER`	`isRouterRole(role)`
Affected roles	`ROUTER` only	`ROUTER`, `ROUTER_LATE`
Effect	`ROUTER_LATE` used CLIENT-class defaults: GPS every 2 min, screen-on 10 min, bluetooth wait 60s, etc.	`ROUTER_LATE` now gets infrastructure defaults: GPS once/day, screen-on 1s, bluetooth wait 1s, etc.

Rationale: The IF_ROUTER macro controls interval defaults for GPS updates, telemetry broadcast, bluetooth timeout, sleep durations, and screen-on time. ROUTER_LATE is an infrastructure node that should not be broadcasting GPS every 2 minutes, keeping its screen on for 10 minutes, or waiting 60 seconds for bluetooth connections. Its installRoleDefaults already sets device_update_interval = ONE_DAY — the IF_ROUTER defaults are consistent with that infrastructure profile. Without this change, ROUTER_LATE devices waste battery and airtime on intervals designed for handheld client devices.

#16 — Default.cpp `getConfiguredOrDefaultMsScaled()` (lines 41, 45)

	Before	After
Line 41	`role == ROUTER` skips congestion scaling	`isRouterRole(role)` — both `ROUTER` and `ROUTER_LATE` skip
Line 45	`TRACKER, SENSOR` skip congestion scaling	`isTrackerOrSensorRole(role)` — adds `TAK_TRACKER`
Effect	`ROUTER_LATE` and `TAK_TRACKER` had their intervals congestion-scaled unnecessarily	Both now skip congestion scaling, matching their role class behavior

Rationale: Congestion scaling multiplies broadcast intervals based on online node count. Infrastructure roles (ROUTER, ROUTER_LATE) already have significantly higher base intervals and should not be further scaled — they need to maintain predictable relay timing. Similarly, TAK_TRACKER is a tracker variant with priority position sends; its intervals should not be dilated by congestion scaling any more than TRACKER's are.

#17 — MeshService.cpp `isPreferredRebroadcaster` (line 299)

	Before	After
Code	`role == ROUTER`	`isRouterLikeRole(role)`
Affected roles	`ROUTER` only	`ROUTER`, `ROUTER_LATE`, `CLIENT_BASE`
Effect	`ROUTER_LATE` and `CLIENT_BASE` would send unsolicited NodeInfo when hearing unknown nodes	All infrastructure/relay roles now skip unsolicited NodeInfo to reduce channel noise

Rationale: When isPreferredRebroadcaster is true, the node skips sending its own NodeInfo in response to hearing a new unknown node. The intent is that high-traffic infrastructure nodes should not add NodeInfo chatter for every new node they overhear. ROUTER_LATE is equally high-traffic infrastructure — it is is_unmessagable = true and has no reason to solicit NodeInfo exchanges. CLIENT_BASE acts as a relay for favorited nodes and also benefits from reduced unsolicited chatter on the channel. All three roles prioritize forwarding over chatting.

#18 — AdminModule.cpp rebroadcast mode restriction (line 653)

	Before	After
Code	`role == ROUTER`	`isRouterRole(role)`
Affected roles	`ROUTER` only	`ROUTER`, `ROUTER_LATE`
Effect	`ROUTER_LATE` could be configured with `rebroadcast_mode = NONE`	`ROUTER_LATE` now blocked from `NONE` rebroadcast mode, matching `ROUTER`

Rationale: A router-class node with rebroadcast mode set to NONE is a contradiction — it exists to relay packets. The validation guard preventing this misconfiguration should apply equally to ROUTER_LATE. Without this check, a user could accidentally disable relaying on a ROUTER_LATE node with no warning, silently breaking mesh connectivity through that node.

#19 — sleep.cpp `shouldLoraWake()` (line 549)

	Before	After
Code	`role == ROUTER`	`isRouterRole(role)`
Affected roles	`ROUTER` only	`ROUTER`, `ROUTER_LATE`
Effect	`ROUTER_LATE` would not wake on LoRa interrupt from deep sleep	`ROUTER_LATE` now wakes on LoRa activity like `ROUTER`

Rationale: shouldLoraWake determines whether LoRa radio activity should wake the device from deep sleep. A ROUTER_LATE that sleeps through incoming packets cannot fulfill its relay purpose — it would miss packets entirely and create a dead zone in the mesh. This is arguably the most impactful fix: without LoRa wake, a sleeping ROUTER_LATE is a broken relay node. The only intentional difference between the two roles is rebroadcast timing (early vs late), not whether they should wake up to relay at all.

#20 — PositionModule.cpp `want_response` on position packets (line 380)

	Before	After
Code	`role == TRACKER` forces `want_response = false`	`isTrackerRole(role)` forces `want_response = false`
Affected roles	`TRACKER` only	`TRACKER`, `TAK_TRACKER`
Effect	`TAK_TRACKER` would set `want_response = true` on position packets, soliciting replies	`TAK_TRACKER` now suppresses `want_response`, matching `TRACKER` behavior

Rationale: TAK_TRACKER is a low-power tracker variant. Setting want_response = true means it solicits position replies from other nodes, which costs airtime and battery — exactly what tracker-class devices should avoid. The very next line (381) already uses isTrackerRole() for setting priority to RELIABLE for both tracker types. This inconsistency (line 380 TRACKER-only, line 381 both) indicates the want_response line was not updated when TAK_TRACKER was introduced. Same class of bug as the NodeInfoModule fix (#13).

#22–23 — BaseTelemetryModule.h `isSensorOrRouterRole()` → `isSensorOrRouterRole()` free function

	Before	After
Code	Class method: `role == SENSOR
Affected roles	`SENSOR`, `ROUTER` only	`SENSOR`, `ROUTER`, `ROUTER_LATE`
Effect	`ROUTER_LATE` was not treated as an "impolite" telemetry role in 5 telemetry modules	`ROUTER_LATE` now skips multi-hop broadcast request filtering and uses impolite send timing, matching `ROUTER`

Rationale: The isSensorOrRouterRole() check controls two behaviors in telemetry modules: (1) whether to ignore multi-hop broadcast telemetry requests (impolite roles respond regardless), and (2) whether to use more aggressive send timing without channel utilization checks. ROUTER_LATE is an infrastructure node with the same telemetry profile as ROUTER — installRoleDefaults sets device_update_interval = ONE_DAY for both. It should behave identically for telemetry sending policy. The method was also moved from a class method to a free function in meshUtils.h for consistency with the other role helpers.

#29 — NodeDB.cpp `node_info_broadcast_secs` default (line 670)

	Before	After
Code	`role != ROUTER`	`!isRouterRole(role)`
Affected roles	`ROUTER` only kept its configured value	`ROUTER` and `ROUTER_LATE` both keep their configured value
Effect	`ROUTER_LATE` had `node_info_broadcast_secs` overwritten to default during factory reset	`ROUTER_LATE` now preserves its configured `node_info_broadcast_secs` like `ROUTER`

Rationale: During installDefaultConfig() (factory reset), node_info_broadcast_secs is overwritten to the default value for all roles except ROUTER. ROUTER_LATE is an infrastructure role that sets device_update_interval = ONE_DAY in installRoleDefaults — it should also preserve its node info broadcast configuration. Without this, a factory reset on a ROUTER_LATE device would silently revert to client-class NodeInfo broadcast frequency, increasing unnecessary airtime.

#30 — StoreForwardModule.cpp auto-enable S&F server (line 591)

	Before	After
Code	`role == ROUTER`	`isRouterRole(role)`
Affected roles	`ROUTER` only auto-enables S&F server	`ROUTER` and `ROUTER_LATE` both auto-enable S&F server
Effect	`ROUTER_LATE` required explicit `is_server = true` to act as S&F server	`ROUTER_LATE` now auto-enables S&F server mode when S&F is enabled

Rationale: ROUTER_LATE is an infrastructure relay with the same deployment profile as ROUTER — always-on, with PSRAM available for S&F history storage. If Store & Forward is enabled on a ROUTER_LATE node, it should auto-enter server mode just like ROUTER does. The || moduleConfig.store_forward.is_server fallback remains, so any other role can still explicitly opt in. This removes a confusing configuration asymmetry between two roles that differ only in rebroadcast timing.

#37 — CannedMessageModule.cpp DM auto-favorite gate (line 1077)

	Before	After
Code	`!isClientBaseRole(role)`	`!isRouterLikeRole(role)`
Affected roles	Routers (`ROUTER`, `ROUTER_LATE`) still auto-favorited DM peers	Router-like roles no longer auto-favorite DM peers
Effect	Router-class nodes could accumulate favorites just by sending DMs	Only non-router-like roles auto-favorite DM peers

Rationale: Auto-favoriting on DM send is useful for client-style roles, but for infrastructure/relay roles it can create noisy and unintended favorite state. Using !isRouterLikeRole(role) keeps this convenience for client-style devices while avoiding automatic favorite churn on ROUTER, ROUTER_LATE, and CLIENT_BASE.

Intentionally unchanged

These checks have specific reasons for their current role restrictions and should not be broadened or replaced with helpers:

File	Line	Why kept as-is
`RadioInterface.cpp`	612	`ROUTER`-only: `shouldRebroadcastEarlyLikeRouter()` must exclude `ROUTER_LATE` — the entire purpose of `ROUTER_LATE` is to not rebroadcast early. Late rebroadcast timing is handled separately in `FloodingRouter::perhapsCancelDupe()`.
`FloodingRouter.cpp`	128	`ROUTER_LATE`-only: always clamps to late rebroadcast window. This is the defining single-role behavior for `ROUTER_LATE` — no other role shares it. No single-value helper exists for `ROUTER_LATE` alone.
`PositionModule.cpp`	282	`TAK_TRACKER`-only: sends position as TAK packet over ATAK port. This is TAK wire-format specific — no other role sends ATAK PLI. No single-value helper exists for `TAK_TRACKER` alone.
`NodeDB.cpp`	923–979	`installRoleDefaults()` — per-role switch statement setting unique defaults (different intervals, flags, position settings) for each role. Not groupable — each case is role-specific.
`DisplayFormatters.cpp`	various	Switch statement for role display names — one case per role, not groupable.
`AdminModule.cpp`	669	Deprecated role migration check on incoming config payload (`c.payload_variant.device.role`), not `config.device.role`. Checks for `ROUTER_CLIENT` and `REPEATER`, which are deprecated roles with no helpers.

🤝 Attestations

I have tested that my proposed changes behave as described.
I have tested that my proposed changes do not cause any obvious regressions on the following devices:
- Heltec (Lora32) V3
- LilyGo T-Deck
- LilyGo T-Beam
- RAK WisBlock 4631
- Seeed Studio T-1000E tracker card
- Other (please specify below)
  Heltec V4

Rebase with upstream

…evelop

Documents our experimentation with ESP-IDF DFS and why it doesn't work well for Meshtastic (RTOS locks, BLE locks, USB issues). Proposes simpler alternative: manual setCpuFrequencyMhz() control with explicit triggers for when to go fast vs slow.

…evelop

Add isRouterRole(), isRouterLikeRole(), and isTrackerRole() inline helpers to eliminate scattered multi-role comparisons across the codebase. Also fixes magic number 12 in CannedMessageModule and adds missing TAK_TRACKER exclusion in NodeInfoModule requestWantResponse. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…s skipped.

Documents our experimentation with ESP-IDF DFS and why it doesn't work well for Meshtastic (RTOS locks, BLE locks, USB issues). Proposes simpler alternative: manual setCpuFrequencyMhz() control with explicit triggers for when to go fast vs slow.

Add isRouterRole(), isRouterLikeRole(), and isTrackerRole() inline helpers to eliminate scattered multi-role comparisons across the codebase. Also fixes magic number 12 in CannedMessageModule and adds missing TAK_TRACKER exclusion in NodeInfoModule requestWantResponse. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…s skipped.

…ined.

…re-sunl into refactor/role-helpers # Conflicts: # src/PowerFSM.cpp # src/main.cpp # src/mesh/Default.cpp # src/meshUtils.h # src/modules/CannedMessageModule.cpp # src/modules/NodeInfoModule.cpp

caveman99

My thoughts on this:

i spent 20 minutes checking your code for logic errors. That's probably more than it took you to churn it out.
We are replacing comparison operators with nested function calls that use stack space. Most of these simple things could have been macros to inline code.

Apart from the inconsistencies i found, this role management in the code is really fine grained. Replacing them with the helpers creates uncertainty what each helper includes or excludes. I would have rather replaced more of the calls with the IS_ONE_OF macro, so you still see what exactly is permitted and not permitted in place.

caveman99 · 2026-03-04T07:47:26Z

src/mesh/MeshService.cpp

    nodeDB->updateFrom(*mp); // update our DB state based off sniffing every RX packet from the radio
-    bool isPreferredRebroadcaster = config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER;
+    bool isPreferredRebroadcaster = isRouterLikeRole(config.device.role);
    if (mp->which_payload_variant == meshtastic_MeshPacket_decoded_tag &&


The same macro is replaced with isRouterLikeRole() when Default.h uses isRouterRole()

Because this code should also include ROUTER_LATE, but it doesn't. Why should ROUTER_LATE be treated any differently than ROUTER here?

caveman99 · 2026-03-04T07:50:57Z

src/modules/Telemetry/BaseTelemetryModule.h

-               config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER;
-    }
+    bool isSensorOrRouter() const { return isSensorOrRouterRole(config.device.role); }
 };


Wouldn't that move into the helpers as well?

isSensorOrRouter? I should replace its instances with isSensorOrRouterRole, correct.

caveman99 · 2026-03-04T07:52:34Z

src/modules/AdminModule.cpp

+            isRouterRole(config.device.role)) {
            config.device.rebroadcast_mode = meshtastic_Config_DeviceConfig_RebroadcastMode_ALL;
            const char *warning = "Rebroadcast mode can't be set to NONE for a router";
            LOG_WARN(warning);


Well, what is it now? just router or router_late as well?

isRouterRole includes router_late as well, since there is only one or two lines of code that shouldn't include both, and those are very specific pieces of code.

caveman99 · 2026-03-04T07:52:42Z

src/modules/AdminModule.cpp

        // If we're in event mode, nobody is a Router or Router Late
-        if (config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER ||
-            config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_LATE) {
+        if (isRouterRole(config.device.role)) {


see above comment

Saw above, and commented

caveman99 · 2026-03-04T07:53:39Z

src/modules/CannedMessageModule.cpp

-        if (config.device.role != 12) {
+        // Only add as favorite if our role is not router-like.
+        if (!isRouterLikeRole(config.device.role)) {
            LOG_INFO("Proactively adding %x as favorite node", dest);


This is a functional change, the original code only checks for CLIENT_BASE

Yup, wouldn't you think this is a bug? Read #37 in the summary. Why would routers or router_late automatically like nodes messaging it when is_messagable is disabled by default for these nodes.

caveman99 · 2026-03-04T07:55:08Z

src/platform/nrf52/main-nrf52.cpp

-        (IS_ONE_OF(config.device.role, meshtastic_Config_DeviceConfig_Role_TRACKER,
-                   meshtastic_Config_DeviceConfig_Role_TAK_TRACKER, meshtastic_Config_DeviceConfig_Role_SENSOR) &&
-         config.power.is_power_saving == true)) {
+    if (msecToWake != portMAX_DELAY && (isTrackerOrSensorRole(config.device.role) && config.power.is_power_saving == true)) {


someticmes trackerOrSensor includes the TAK Tracker, sometimes not

And in the instances it does not, there is a reason probably why it should. #20 above, for example. Why does TAK_TRACKER use want_response for position packets?

Same with nodeinfo, why does TAK_TRACKER use want_response here? (#13) it seems like it shouldn't, but willing to hear why it would. Seems unnecessary load.

h3lix1 · 2026-03-04T09:40:46Z

@caveman99 I respectfully disagree about the time to create this diff, the time taken to review it before sublission, and what it is trying to accomplish.

The use of macros might save a few bytes on the stack, but there are a few examples in the code where this is done both ways (macros and functions) and I like functions for any flexibility this may need in future.

What I found is a lot of inconsistency in the code about how router, router_late, and client_base, tracker, and tak_tracker is handled. I'm trying to find a solution here, if only to shine light on the problem. Role management isn't fine grained, it's a mess.

h3lix1 and others added 30 commits September 25, 2025 18:29

Merge pull request #2 from meshtastic/develop

809801f

Rebase with upstream

Merge branch 'meshtastic:develop' into develop

566f6a8

Merge branch 'meshtastic:develop' into develop

6da792a

Merge branch 'meshtastic:develop' into develop

d36b29a

Merge branch 'meshtastic:develop' into develop

b5b9087

Merge branch 'meshtastic:develop' into develop

c36049a

Merge branch 'meshtastic:develop' into develop

31e254b

Merge branch 'meshtastic:develop' into develop

014d645

Merge branch 'meshtastic:develop' into develop

b4f8e9d

Merge branch 'meshtastic:develop' into develop

3b613c9

Merge branch 'meshtastic:develop' into develop

e6bffd0

Merge branch 'meshtastic:develop' into develop

2b6f1dc

Merge branch 'meshtastic:develop' into develop

1314e5d

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

e453cb5

…evelop

Merge branch 'meshtastic:develop' into develop

107e25e

Merge branch 'meshtastic:develop' into develop

875830b

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

dc8fb77

…evelop

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

6fe1bb0

…evelop

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

3ac081b

…evelop

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

da7b734

…evelop

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

c5ea613

…evelop

Merge branch 'develop' of github.com:h3lix1/mesh-firmware-sunl into d…

f144767

…evelop

Added (previously skipped) areas, and added better reasoning for role…

1ca3692

…s skipped.

Added (previously skipped) areas, and added better reasoning for role…

7cd3504

…s skipped.

Added more helpers, combined a few that should have already been comb…

8ff7ea3

…ined.

Miseed a couple files.

3b13a4a

h3lix1 added 4 commits March 3, 2026 20:22

Merge branch 'refactor/role-helpers' of github.com:h3lix1/mesh-firmwa…

b8763a4

…re-sunl into refactor/role-helpers # Conflicts: # src/PowerFSM.cpp # src/main.cpp # src/mesh/Default.cpp # src/meshUtils.h # src/modules/CannedMessageModule.cpp # src/modules/NodeInfoModule.cpp

Remove ESP32 power management doc from refactor branch

4ab16dc

Update some logic what was modified to change it back

4fd04f9

refactor(RadioInterface): clarify rebroadcast logic for ROUTER role

c4b3eba

github-actions bot added needs-review Needs human review bugfix Pull request that fixes bugs labels Mar 4, 2026

h3lix1 changed the title ~~feat: Refactor roles to helper functions where possible. Fixed instances where found with reasioning.~~ feat: Refactor roles to helper functions where possible. Fixed issues where found with reasioning. Mar 4, 2026

caveman99 requested changes Mar 4, 2026

View reviewed changes

caveman99 added triaged Reviewed by the team, has enough information and ready to work on now. cleanup Code cleanup or refactor ai-generated Possible AI-generated low-quality content and removed bugfix Pull request that fixes bugs needs-review Needs human review labels Mar 4, 2026

Uh oh!

Conversation

h3lix1 commented Mar 4, 2026

Summary

NOTE: This was assisted with the use of both Claude Opus 4.6 and GPT 5.3-Codex. The logic changes are all mine.

Changes

Functional change details

#13 — NodeInfoModule.cpp requestWantResponse (line 99)

#14 — PowerFSM.cpp isPowered() (line 41) and PowerFSM_setup() (line 265)

#15 — Default.h IF_ROUTER macro (line 44)

#16 — Default.cpp getConfiguredOrDefaultMsScaled() (lines 41, 45)

#17 — MeshService.cpp isPreferredRebroadcaster (line 299)

#18 — AdminModule.cpp rebroadcast mode restriction (line 653)

#19 — sleep.cpp shouldLoraWake() (line 549)

#20 — PositionModule.cpp want_response on position packets (line 380)

#22–23 — BaseTelemetryModule.h isSensorOrRouterRole() → isSensorOrRouterRole() free function

#29 — NodeDB.cpp node_info_broadcast_secs default (line 670)

#30 — StoreForwardModule.cpp auto-enable S&F server (line 591)

#37 — CannedMessageModule.cpp DM auto-favorite gate (line 1077)

Intentionally unchanged

🤝 Attestations

Uh oh!

caveman99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h3lix1 commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

#13 — NodeInfoModule.cpp `requestWantResponse` (line 99)

#14 — PowerFSM.cpp `isPowered()` (line 41) and `PowerFSM_setup()` (line 265)

#15 — Default.h `IF_ROUTER` macro (line 44)

#16 — Default.cpp `getConfiguredOrDefaultMsScaled()` (lines 41, 45)

#17 — MeshService.cpp `isPreferredRebroadcaster` (line 299)

#19 — sleep.cpp `shouldLoraWake()` (line 549)

#20 — PositionModule.cpp `want_response` on position packets (line 380)

#22–23 — BaseTelemetryModule.h `isSensorOrRouterRole()` → `isSensorOrRouterRole()` free function

#29 — NodeDB.cpp `node_info_broadcast_secs` default (line 670)

h3lix1 commented Mar 4, 2026 •

edited

Loading