Add helpful checks of channel names and PSK - #10792
Conversation
⚡ 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 (27)
Build artifacts expire on 2026-08-10. Updated for |
github-actions
Bot
added
needs-review
There was a problem hiding this comment.
Pull request overview
This PR adds AdminModule-side validation/warnings to help users avoid channel-name and PSK configurations that unintentionally diverge from modem preset expectations, reducing silent interoperability failures (notably around preset changes and “blank” inputs).
Changes:
- Emit warnings after a channel is saved to catch blank PSKs (with licensing exceptions) and preset-name/PSK mismatches.
- Emit warnings after a modem preset change to detect channels still named like the old preset or colliding with the new preset name.
- Add helper normalization for loose preset-name comparison (case-insensitive, spaces stripped).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/modules/AdminModule.h |
Declares new internal warning helpers for LoRa preset changes and channel updates. |
src/modules/AdminModule.cpp |
Implements normalization + warning logic, and wires warnings into handleSetConfig() and handleSetChannel(). |
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for 623f2a0 |
|
@garthvh @jamesarich Any views on the wording of the messages? I know you're both able to tweak them in-app, but if they're right here, they're right everywhere. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdminModule now queues channel, LoRa configuration, and licensed-mode warnings during edit transactions, then emits coalesced notifications at commit. Direct updates flush immediately. Tests cover conflict detection, coalescing, and licensed-mode behavior. ChangesAdmin warning coalescing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminModule
participant MeshService
Client->>AdminModule: begin_edit_settings
Client->>AdminModule: set_channel or set_config
AdminModule->>AdminModule: queue warning
Client->>AdminModule: commit_edit_settings
AdminModule->>AdminModule: flushChannelWarnings
AdminModule->>MeshService: sendClientNotification
MeshService-->>Client: combined warning
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/test_admin_radio/test_main.cpp (2)
1306-1313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert total warning count for licensed-immediate test.
Unlike its transactional counterpart (line 1328), this test only checks
warningsContaining("Licensed mode activated"), never the totalcapturedWarnings.size(). It wouldn't catch a regression where an unexpected extra warning (e.g., a spurious channel-name warning) is emitted alongside the licensed notice.✅ Suggested addition
sendSetChannel(makeChannel(0, meshtastic_Channel_Role_PRIMARY, "", CUSTOM_KEY, 2)); TEST_ASSERT_EQUAL_INT(1, warningsContaining("Licensed mode activated")); + TEST_ASSERT_EQUAL_INT(1, (int)capturedWarnings.size()); }🤖 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_admin_radio/test_main.cpp` around lines 1306 - 1313, The licensed-immediate test only verifies warningsContaining("Licensed mode activated"), so it can miss extra warnings emitted by sendSetChannel() or ensureLicensedOperation(). Update test_warn_license_noTransaction_emittedImmediately to also assert the total capturedWarnings.size() matches the expected single warning, mirroring the transactional test’s stronger check.
1170-1223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the multi-line banner comment.
The 8-line section-header comment exceeds the "one or two lines at most" limit for code comments. The rest of the helpers (
warningsContaining,makeChannel,sendAdmin, etc.) look correct.✏️ Suggested trim
-// ----------------------------------------------------------------------- -// Channel-configuration warning + coalescing tests -// -// These exercise the real incoming-admin-message path (handleReceivedProtobuf): -// begin_edit_settings / set_channel / commit_edit_settings. Warnings raised while a -// transaction is open must be deferred and collapsed into a single notification at -// commit; outside a transaction each save emits its own single message immediately. -// ----------------------------------------------------------------------- +// Channel-configuration warning + coalescing tests: exercises the real incoming-admin +// path (begin/set_channel/commit) where transaction-scoped warnings must collapse to one.As per coding guidelines, "Keep code comments minimal: one or two lines at most, only explain the why when it is not obvious, and avoid multi-paragraph explanatory comments."
🤖 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_admin_radio/test_main.cpp` around lines 1170 - 1223, Trim the section-header comment above the channel-configuration warning/coalescing test helpers so it is no more than one or two lines. Keep the intent clear but remove the long explanatory banner, leaving the helper functions like warningsContaining, makeChannel, sendAdmin, and sendSetChannel unchanged.Source: Coding guidelines
src/modules/AdminModule.cpp (1)
1965-1981: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the multi-paragraph doc comments to satisfy the repo comment style.
Both
warnOnChannelSet(this block) andwarnOnLoraPresetChange(Lines 2089-2106) carry multi-paragraph explanatory comments. The check logic here is already self-explanatory; condense each to a one/two-line summary of the why.As per coding guidelines for
**/*.{cpp,h}: "Keep code comments minimal: one or two lines at most, only explain the why when it is not obvious, and avoid multi-paragraph explanatory comments."🤖 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/AdminModule.cpp` around lines 1965 - 1981, Trim the long Doxygen block above warnOnChannelSet to a one- or two-line comment that only captures the essential why, and do the same for warnOnLoraPresetChange; remove the multi-paragraph bullet list and detailed scenario explanations so the comments match the repo style while keeping the intent clear.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/modules/AdminModule.cpp`:
- Around line 1965-1981: Trim the long Doxygen block above warnOnChannelSet to a
one- or two-line comment that only captures the essential why, and do the same
for warnOnLoraPresetChange; remove the multi-paragraph bullet list and detailed
scenario explanations so the comments match the repo style while keeping the
intent clear.
In `@test/test_admin_radio/test_main.cpp`:
- Around line 1306-1313: The licensed-immediate test only verifies
warningsContaining("Licensed mode activated"), so it can miss extra warnings
emitted by sendSetChannel() or ensureLicensedOperation(). Update
test_warn_license_noTransaction_emittedImmediately to also assert the total
capturedWarnings.size() matches the expected single warning, mirroring the
transactional test’s stronger check.
- Around line 1170-1223: Trim the section-header comment above the
channel-configuration warning/coalescing test helpers so it is no more than one
or two lines. Keep the intent clear but remove the long explanatory banner,
leaving the helper functions like warningsContaining, makeChannel, sendAdmin,
and sendSetChannel unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fc04ea9-f83e-4052-854c-24b79ad1f0b2
📒 Files selected for processing (3)
src/modules/AdminModule.cppsrc/modules/AdminModule.htest/test_admin_radio/test_main.cpp
The base64 encoding of the default channel key isn't something a user should type in by hand; state the condition instead of a raw key value.
# Conflicts: # test/test_admin_radio/test_main.cpp
* added warnings from firmware for simple channel setting mistakes * more and better checks * one ping only * Drop literal 'AQ==' from blank-PSK channel warnings The base64 encoding of the default channel key isn't something a user should type in by hand; state the condition instead of a raw key value. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
This is the fix for #10764 where a user can think that they have changed presets, but have an explicitly named channel that uses the old preset default name - user on NarrowSlow getting/sending channel messages with hash 0x08 rather than 0x12.
This also fixes the case where a completely blank password is entered rather than AQ==, which again yields an incorrect channel hash (0x0a rather than 0x08 for LongFast, for example) - this is skipped for licensed users.
I have also included a fix for a perennial problem - my inability to remember the difference between LongFast, Longfast, Long Fast, Long fast, long fast and longfast. The firmware checks if a channel name that is lower-cased and stripped of spaces matches the same as the modem preset, and flags this to the user.
If you're on a custom preset, you also get some help to match a channel correctly, although it can't be as exact.
All of these generate phone warnings, but I think they're necessary.
I've tested this, and it generates the warnings as expected.
example log lines:
🤝 Attestations
Promicro DIY
Summary by CodeRabbit
%).