fix(radio): serialize live LoRa configuration changes - #11311
Conversation
📝 WalkthroughWalkthroughThis PR replaces direct LoRa configuration mutation with staged asynchronous application. It adds hardware-aware validation, rollback, TX/RX barriers, beacon restoration, queued Portduino requests, menu integration, and extensive test coverage. ChangesLoRa configuration workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mesh/MeshPacketQueue.cpp (1)
160-221: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the unconditional
*evictedwrites with an assert.
replaceLowerPriorityPacketdereferencesevictedwithout a null check in three branches (Lines 173, 189, 212). The only current caller,enqueue(Line 75), always passes a valid address, so this is not reachable today. Addassert(evicted != nullptr);at function entry to make the invariant explicit and to fail fast if a future caller passesnullptr, instead of leaving an unguarded pointer dereference.🛡️ Proposed fix to assert the invariant
bool MeshPacketQueue::replaceLowerPriorityPacket(meshtastic_MeshPacket *p, meshtastic_MeshPacket **evicted) { + assert(evicted != nullptr); if (queue.empty()) { return false; // No packets to replace }Based on learnings, the applicable path instruction states: "Use
assert()for invariants that should never fail."🤖 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/MeshPacketQueue.cpp` around lines 160 - 221, Add an assert at the start of MeshPacketQueue::replaceLowerPriorityPacket to require evicted is non-null before any branch can dereference it. Keep the existing replacement logic unchanged and use assert() to enforce this caller invariant.Sources: Coding guidelines, Path instructions
src/mesh/SX128xInterface.cpp (1)
163-195: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDe-assert RXEN/TXEN before the early return on standby failure.
Lines 171-172 return before the RXEN/TXEN pins go LOW and before
disableInterrupt(). Iflora.standby()fails, the external RF switch and the PA stay enabled, and interrupts stay armed. The previous code always reached the de-assert block. Move the failure return after the GPIO and interrupt cleanup, or perform the cleanup on both paths.🐛 Proposed fix
if (err != RADIOLIB_ERR_NONE) LOG_ERROR("SX128x standby %s%d", radioLibErr, err); - if (err != RADIOLIB_ERR_NONE) - return err; `#if` ARCH_PORTDUINOisReceiving = false; // If we were receiving, not any more activeReceiveStart = 0; disableInterrupt(); + if (err != RADIOLIB_ERR_NONE) + return err; if (completePacket)🤖 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/SX128xInterface.cpp` around lines 163 - 195, Update SX128xInterface<T>::setStandby so standby failure does not bypass cleanup: ensure RXEN/TXEN are de-asserted, interrupts are disabled, and the existing state cleanup runs before returning the lora.standby() error. Preserve the normal completePacket handling and successful return behavior.src/modules/AdminModule.cpp (1)
846-939: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA name-only owner change does not update the staged owner candidate, so a later apply reverts it.
Line 851 copies
pendingOwnerConfig.candidateintocandidatewhen an owner change is already staged. IflicenseChangedis false, line 936 writes the new names into the liveowneronly.pendingOwnerConfig.candidatekeeps the old names. When the staged radio apply later succeeds,completeLoRaConfigApplyexecutesowner = pendingOwnerConfig.candidate(line 1335) and the name change is lost.Update the staged candidate in the same place.
🐛 Proposed fix
- owner = candidate; + if (pendingOwnerConfig.active) + pendingOwnerConfig.candidate = candidate; + owner = candidate; service->reloadOwner(!hasOpenEditTransaction); saveChanges(SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE); return true;🤖 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 846 - 939, Update the non-license-change path in AdminModule::handleSetOwner so that when pendingOwnerConfig.active is true, it also assigns the modified candidate to pendingOwnerConfig.candidate before or alongside updating owner. Preserve the existing live-owner update and save behavior, ensuring later completeLoRaConfigApply uses the latest name values.
🧹 Nitpick comments (21)
src/mesh/raspihttp/PiWebServer.cpp (1)
409-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe drain-all branch is unreachable.
valueAllis declared at line 392 and never assigned, sovalueAll == "true"is always false. The new 503 handling inside this branch can never run. Either parse the request parameter intovalueAll, or delete the branch and keep only the single-protobuf path. The branch also sets the response body twice (lines 415 and 417) and overwrites it on every loop iteration, so it would need rework before it could be enabled.🤖 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/raspihttp/PiWebServer.cpp` around lines 409 - 421, Make the drain-all path in the request handler reachable by assigning the incoming request’s all-values parameter to valueAll, or remove that path if only single-protobuf responses are supported. If retaining it, rework the loop so the response body is set once with the complete drained payload, and eliminate the conflicting duplicate response setup while preserving the 503 handling from submitFromRadio.test/test_mesh_beacon/test_main.cpp (1)
1747-1753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the static beacon radio state in
tearDown.
MeshBeaconModuleholds static state: the temporary-config flag, the pending-restore flag, and the target-radio-settings table. Each new test clears that state only at the end of its own body, for example line 1512 and line 1533. Unity aborts the test body on a failed assertion, so those cleanup calls are skipped. The leaked flag then changes the result of later tests and hides the first failure.Clear the temporary state and the target-settings table in
tearDownso each test starts from a known state.🤖 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_mesh_beacon/test_main.cpp` around lines 1747 - 1753, Update tearDown to reset MeshBeaconModule’s static temporary-config flag, pending-restore flag, and target-radio-settings table in addition to the existing module cleanup. Use the class’s existing reset/clear mechanisms so cleanup always runs after assertion failures and each test begins with pristine beacon state.src/mesh/LR11x0ConfigApply.h (2)
59-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the overloaded
setFrequencycontract.
lr11x0SetFrequencyForBandcallsops.setFrequency(frequency, targetWideBand)on line 62. The second parameter of the two-argument overload isskipCalibration(seesrc/mesh/LR11x0Interface.hline 110). The band flag therefore doubles as the calibration-skip flag. The behavior is intentional for wide band, but the call reads as a band argument. Rename the local flag or add a one-line comment.🤖 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/LR11x0ConfigApply.h` around lines 59 - 74, Clarify the overloaded setFrequency call in lr11x0SetFrequencyForBand by documenting that targetWideBand is intentionally passed as the skipCalibration argument, or rename the local flag to reflect that dual meaning while preserving the existing wide-band behavior.
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTie the power cap to the resolved band, not the requested region.
lr11x0BandPolicyForselectswideBandfromregion.wideLora && supportsWideBand, but selectsmaxPowerfromregion.wideLoraalone. If a sub-GHz-only LR1110 ever receives a wide-LoRa region, the radio stays on sub-GHz while the power cap becomeswideBandMaxPower. Validation currently rejects that combination, so this is defensive only.♻️ Proposed change to derive both fields from the resolved band
- return {region.wideLora && supportsWideBand, region.wideLora ? wideBandMaxPower : subGhzMaxPower}; + const bool wideBand = region.wideLora && supportsWideBand; + return {wideBand, wideBand ? wideBandMaxPower : subGhzMaxPower};🤖 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/LR11x0ConfigApply.h` around lines 38 - 42, Update lr11x0BandPolicyFor so the resolved wide-band condition, region.wideLora && supportsWideBand, determines both the wide-band flag and selected power cap; use subGhzMaxPower whenever that condition is false.src/mesh/RF95Interface.cpp (1)
276-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso clear
activeReceiveStarton standby.The other drivers reset
activeReceiveStart = 0next toisReceiving = false(seesrc/mesh/SX126xInterface.cppline 322 andsrc/mesh/LR20x0Interface.cppline 258). RF95 leaves the stale timestamp. IfreceiveDetected()consults it, a stale value can affect receive-state decisions after a reconfiguration.🤖 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/RF95Interface.cpp` around lines 276 - 292, Update RF95Interface::setStandby so it resets activeReceiveStart to 0 alongside isReceiving = false, matching the standby state cleanup used by the other radio drivers; leave the existing error handling and transmission cleanup unchanged.src/mesh/LR11x0Interface.cpp (1)
209-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the correct failing step in
reinitializeForBand.Line 211 reports
LR11x0ApplyStep::STANDBYwhenlr11x0BeginForBandfails, and line 234 reportsLR11x0ApplyStep::OUTPUT_POWERwhensetRegulatorDCDCfails. Both labels mislead the log at line 188. Add dedicated steps, or log the operation name directly.🤖 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/LR11x0Interface.cpp` around lines 209 - 234, Update reinitializeForBand’s failure reporting so lr11x0BeginForBand failures and setRegulatorDCDC failures identify their actual operations instead of using STANDBY and OUTPUT_POWER. Add dedicated LR11x0ApplyStep values if the existing reporting requires enum steps, and ensure the log at the failure path uses those corrected labels.src/mesh/RadioLibInterface.cpp (3)
301-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Throttlefor the timeout check.The coding guidelines require
Throttlefor elapsed-time checks instead of rawmillis()arithmetic.♻️ Proposed change
- const bool timedOut = static_cast<uint32_t>(nowMsec - request->requestedAtMsec) >= request->timeoutMsec; + const bool timedOut = !Throttle::isWithinTimespanMs(request->requestedAtMsec, request->timeoutMsec);Note:
nowMsecis then unused for this check; keep it if other call sites depend on the passed timestamp.As per coding guidelines: "Use
Throttlefor time-based rate limiting and elapsed-time checks instead of rawmillis()arithmetic."🤖 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/RadioLibInterface.cpp` at line 301, Replace the raw elapsed-time arithmetic in the timeout check with the established Throttle mechanism, using the timeout value from request->timeoutMsec and preserving the existing timedOut behavior. Keep nowMsec only if it is required by other call sites or logic.Source: Coding guidelines
617-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the beacon TX branch instead of interleaving braces with
#if.The
#endifat line 635 closes inside theelse ifbody, and lines 638-641 and 657-659 re-open conditional blocks. The brace structure is therefore only balanced when both preprocessor states are traced separately. This is easy to break in later edits. Extract the beacon-specific handling into a helper (for examplehandleBeaconTxScheduling(txp)) that returns whether the caller must return early, and keep a single#if !MESHTASTIC_EXCLUDE_BEACONaround the helper body.🤖 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/RadioLibInterface.cpp` around lines 617 - 659, Extract the beacon-specific TX reconfiguration and scheduling logic from the surrounding transmit branch into a helper such as handleBeaconTxScheduling(txp), returning whether the caller should exit early. Keep the helper’s implementation under one `#if` !MESHTASTIC_EXCLUDE_BEACON guard, remove the interleaved conditional braces and internal `#endif` from the caller, and preserve the existing failed, in-progress, channel-active, and queue-send behavior.
606-616: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSchedule a retry when the drop-dequeue claim fails.
dequeueTxPacketIfConfigApplyAllowed()returnsnullptrwhenclaimConfigApplyTxStart()fails. In that case this branch does nothing: the invalid packet stays at the queue front and no newTRANSMIT_DELAY_COMPLETEDnotification is scheduled from here. The queue then only restarts whenserviceConfigApply()finalizes and callssetTransmitDelay(). Add an explicit reschedule so the path does not depend on that side effect.♻️ Proposed change
meshtastic_MeshPacket *bad = dequeueTxPacketIfConfigApplyAllowed(); if (bad != nullptr) { LOG_DEBUG("Beacon: invalid TX radio config, dropping packet 0x%08x", bad->id); MeshBeaconModule::clearTargetRadioSettings(bad); packetPool.release(bad); - setTransmitDelay(); } + setTransmitDelay();🤖 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/RadioLibInterface.cpp` around lines 606 - 616, Update the invalid beacon-config branch around dequeueTxPacketIfConfigApplyAllowed() to call setTransmitDelay() when the dequeue returns nullptr, ensuring a retry is scheduled when claimConfigApplyTxStart() fails. Preserve the existing drop, cleanup, and delay behavior for successfully dequeued packets without adding duplicate scheduling.src/modules/MeshBeaconModule.h (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument all four
RadioConfigResultvalues.The comment describes
RECONFIGUREDandFAILEDonly. Callers must also distinguishIN_PROGRESS(another transition holds the radio; retry later) fromUNCHANGED(no switch was performed).RadioLibInterfacebranches on all four, so state each meaning here.🤖 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/MeshBeaconModule.h` around lines 41 - 47, Update the reconfigureForBeaconTX documentation to define all four RadioConfigResult outcomes: RECONFIGURED requires rerunning the CCA transmit delay, FAILED prevents transmission, IN_PROGRESS means another transition owns the radio and the caller should retry later, and UNCHANGED means no radio switch was performed.src/mesh/RadioLibInterface.h (1)
281-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the access-specifier churn.
This region now switches
protected:→private:→protected:within about 20 lines. Group the new members with the existing sections of the same visibility so the class surface stays easy to read.🤖 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/RadioLibInterface.h` around lines 281 - 299, Reorganize the declarations around scheduleBeaconRestoreRetry(), startSend(), and getQueueStatus() to avoid switching visibility from protected to private and back within this short section. Move each member into the nearest existing section matching its intended visibility, preserving access semantics while removing the redundant access-specifier churn.src/platform/portduino/SimRadio.cpp (1)
235-251: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMatch the null-pointer handling used by
RadioLibInterface.
RadioLibInterface::onNotifycheckstxp != nullptraftertxQueue.dequeue()(src/mesh/RadioLibInterface.cpplines 649-650). Here the code relies onassert(txp). IfNDEBUGis defined for a Portduino build, the assert disappears andstartSend(txp)dereferences a null pointer. The invariant is expected to hold, so keep the assert, but add the guard for the release path.🛡️ Proposed change
if (claimConfigApplyTxStart()) { meshtastic_MeshPacket *txp = txQueue.dequeue(); assert(txp); - if (configApplyTxInhibited()) { + if (txp == nullptr) { + releaseConfigApplyTxStart(); + break; + } + if (configApplyTxInhibited()) {🤖 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/platform/portduino/SimRadio.cpp` around lines 235 - 251, Update the transmit handling around txQueue.dequeue() in the SimRadio notify path to retain assert(txp) while guarding all packet-dependent operations with txp != nullptr. Match RadioLibInterface::onNotify behavior so release builds skip startSend, airtime accounting, and notification when dequeue returns null, while preserving configuration cleanup.src/mesh/MeshPacketQueue.h (1)
22-25: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the
evictedownership transfer inenqueue.An eviction can transfer a displaced packet to the caller; the third parameter is optional, so callers that omit it may not know they must release the replacement packet. If the caller passes
nullptr,MeshPacketQueue::enqueue()releases the packet internally.♻️ Proposed doc change
/** enqueue a packet, return false if full * `@param` dropped Optional pointer to a bool that will be set to true if a packet was dropped + * `@param` evicted Optional out-pointer receiving a displaced lower-priority packet. Ownership passes + * to the caller, which must release it. */ bool enqueue(meshtastic_MeshPacket *p, bool *dropped = nullptr, meshtastic_MeshPacket **evicted = nullptr);🤖 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/MeshPacketQueue.h` around lines 22 - 25, Update the enqueue documentation in MeshPacketQueue to describe the ownership behavior of the optional evicted parameter: when provided, it receives the displaced packet and the caller must release it; when nullptr, enqueue releases the displaced packet internally. Keep the existing dropped parameter documentation and enqueue signature unchanged.src/modules/MeshBeaconModule.cpp (1)
77-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the return value of
getHomeRadioConfig, then the return value is documented.
meshBeaconModule::getHomeRadioConfig(prepared.previous, &homePrimaryChannel);calls the function but does not check the return value. Add the beacon-override semantics to the declaration comment insrc/modules/MeshBeaconModule.h; otherwise updateAdminModuleto handle the no-data case.🤖 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/MeshBeaconModule.cpp` around lines 77 - 92, Document the return contract of MeshBeaconModule::getHomeRadioConfig in its declaration: return true when temporary beacon override data is available and applied, and false when no override data exists while leaving the current configuration in place. Keep the existing call in AdminModule unchanged since the documented beacon-override semantics make ignoring the return value intentional.src/graphics/draw/MenuHandler.cpp (1)
129-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the queue failure to the user, not only to the log.
The menu callbacks discard the return value, so a rejected request produces no visible feedback. The user sees the previous region, preset, or slot with no explanation. Show a short banner when the request is not accepted.
♻️ Suggested change
bool requestMenuLoRaConfig(const meshtastic_Config_LoRaConfig &candidate, AdminModule::MenuLoRaTransition transition = AdminModule::MenuLoRaTransition::NONE) { if (!adminModule || !adminModule->requestMenuLoRaConfig(candidate, transition)) { LOG_WARN("Unable to queue LoRa configuration change from menu"); + IF_SCREEN(screen->showSimpleBanner("Radio busy\nTry again", 3000)); return false; } return true; }🤖 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/graphics/draw/MenuHandler.cpp` around lines 129 - 138, Update requestMenuLoRaConfig so a failed adminModule->requestMenuLoRaConfig call displays a short user-facing banner in addition to logging and returning false. Ensure rejected menu requests provide visible feedback while successful requests retain the existing behavior.src/mesh/RadioInterface.cpp (2)
1049-1058: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Throttlefor the timeout check.
serviceConfigApplycomputes the elapsed time with rawmillis()arithmetic. The file already includesThrottle.hat line 17. Use the helper so the elapsed-time convention stays consistent across the radio path.♻️ Suggested change
- if (static_cast<uint32_t>(nowMsec - request->requestedAtMsec) >= request->timeoutMsec) { + if (!Throttle::isWithinTimespanMs(request->requestedAtMsec, request->timeoutMsec)) { finishConfigApply(request, RadioConfigApplyResult::TIMED_OUT); return; }Confirm the helper signature before applying;
nowMsecthen becomes unused in this function.As per coding guidelines: "Use
Throttlefromsrc/mesh/Throttle.hfor time-based rate limiting and elapsed-time checks; do not use rawmillis()arithmetic such asmillis() > lastMs + N, because it is rollover-unsafe."🤖 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/RadioInterface.cpp` around lines 1049 - 1058, Update RadioInterface::serviceConfigApply to use the existing Throttle helper for the request timeout check instead of raw nowMsec/requestedAtMsec subtraction. Confirm Throttle’s API and preserve the TIMED_OUT result when the timeout elapses; remove nowMsec from the function if it becomes unused.Source: Coding guidelines
1626-1628: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the staged-configuration reference binding.
The comma expression inside the conditional operator makes the assignment to
stagedConfigeasy to miss. An explicit branch reads better and keeps the same behavior.♻️ Suggested change
const bool stagedApply = configApplyLoraConfig != nullptr; meshtastic_Config_LoRaConfig stagedConfig; - meshtastic_Config_LoRaConfig &loraConfig = stagedApply ? (stagedConfig = *configApplyLoraConfig, stagedConfig) : config.lora; + if (stagedApply) + stagedConfig = *configApplyLoraConfig; + meshtastic_Config_LoRaConfig &loraConfig = stagedApply ? stagedConfig : config.lora;🤖 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/RadioInterface.cpp` around lines 1626 - 1628, Replace the conditional/comma-expression initialization of loraConfig near stagedApply with an explicit branch: copy *configApplyLoraConfig into stagedConfig and bind loraConfig to stagedConfig when stagedApply is true; otherwise bind it directly to config.lora. Preserve the existing reference semantics and behavior.src/modules/AdminModule.cpp (2)
516-560: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated staged-commit logic in the commit path and the expiry path. Both sites repeat the same sequence:
normalizePendingChannelPrimary(), thehasStagedLoRaConfig/requiresRadioApplydecision,tryBeginLoRaConfigApply(), theservice->requestLoRaConfig(...)call with the same six arguments, the same rollback ofpendingChannelConfig/pendingOwnerConfig/pendingLoRaConfig/pendingMenuLoRaTransition, and the sameunrelatedSegmentssave. The copies already differ (pendingOwnerConfig.activeis part of the staged-config test only in the expiry path, and the saves use differentshouldRebootvalues), which shows the divergence risk.
src/modules/AdminModule.cpp#L516-L560: extract the shared sequence into one private helper, for examplebool commitStagedEdits(int editedSegments, bool shouldReboot), and call it here.src/modules/AdminModule.cpp#L2403-L2449: call the same helper, and confirm whether omittingpendingOwnerConfig.activefrom the commit-path staged-config test is intentional.🤖 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 516 - 560, Extract the duplicated staged-commit flow into a private helper such as commitStagedEdits(int editedSegments, bool shouldReboot), including normalization, apply-busy handling, requestLoRaConfig, rollback, and unrelated-segment saving. Update src/modules/AdminModule.cpp lines 516-560 and 2403-2449 to call the helper while preserving each site’s shouldReboot behavior; also verify whether pendingOwnerConfig.active must be included in the commit-path staged-configuration check.
1169-1222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated slot-preservation bookkeeping.
requestLoRaConfig(incoming, fromOthers)andrequestMenuLoRaConfigrepeat the sameprevious/previousUsesDefaultselection, the samechannel_numcollapse, and the samepreserveDefaultFrequencySlotpost-assignment. Two copies of this rule will diverge. Move the shared part into one private helper, and keep only the transition-specific field changes inrequestMenuLoRaConfig.🤖 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 1169 - 1222, Extract the duplicated previous-slot selection, channel_num collapse, and preserveDefaultFrequencySlot assignment from AdminModule::requestLoRaConfig and AdminModule::requestMenuLoRaConfig into one private helper. Have both methods use that helper while retaining only the menu transition-specific field changes in requestMenuLoRaConfig, and preserve each method’s existing requestLoRaConfig invocation parameters.src/mesh/RadioConfigApply.h (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInitialize the remaining members for consistency.
previous,candidate,requestedAtMsec, andtimeoutMsechave no default initializers, while every other member does.MeshService::requestLoRaConfigsets all of them today, so there is no current defect. Default initialization removes the risk if a future path submits a partially filled request.♻️ Suggested change
- meshtastic_Config_LoRaConfig previous; - meshtastic_Config_LoRaConfig candidate; - uint32_t requestedAtMsec; - uint32_t timeoutMsec; + meshtastic_Config_LoRaConfig previous = meshtastic_Config_LoRaConfig_init_zero; + meshtastic_Config_LoRaConfig candidate = meshtastic_Config_LoRaConfig_init_zero; + uint32_t requestedAtMsec = 0; + uint32_t timeoutMsec = 0;🤖 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/RadioConfigApply.h` around lines 20 - 23, Default-initialize previous, candidate, requestedAtMsec, and timeoutMsec in the request structure to match the initialization pattern of its other members, while preserving MeshService::requestLoRaConfig behavior.src/mesh/Channels.cpp (1)
393-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider sharing the channel-repair invariants with
fixupChannel/onConfigChanged.
setChannelInFilere-implements three invariants that already exist in this file: thehas_settingsreset toDISABLED, the legacy"Default"name collapse (fixupChannel, lines 63-74), and the "restore a PRIMARY slot" fallback (onConfigChanged, lines 335-348). The copies can diverge when one path changes. Extract a static helper that repairs one channel in a suppliedmeshtastic_ChannelFileand call it from both paths.Note that the file-based path also skips
generateHash(), which the live path performs infixupChannel. That is correct today becausepublishPendingChannels()callschannels.onConfigChanged(), but a shared helper would make the split explicit.🤖 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/Channels.cpp` around lines 393 - 436, Extract a static channel-repair helper from the duplicated logic in setChannelInFile, covering missing settings reset, legacy “Default” name cleanup, and primary-slot restoration. Update fixupChannel and onConfigChanged to call this helper so both live and file-based paths share the same invariants, while preserving generateHash() only in the live path and keeping publishPendingChannels() behavior unchanged.
🤖 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.
Inline comments:
In `@src/graphics/draw/MenuHandler.cpp`:
- Around line 349-351: Update licensedToNormalConfirmMenu() and its selection
handling so a timeout or dismissal, as well as selecting “Keep licensed” while
the alert banner is focused, preserves the normal licensed mode instead of
applying pendingRegion as an unlicensed mode. Ensure the alert-banner callback
path and default transition both use the licensed transition, while retaining
the explicit exit behavior for the other option.
In `@src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp`:
- Around line 361-364: Update the preset candidate construction in the menu
handler containing requestMenuLoRaConfig to clear channel_num and
override_frequency before submitting it. Match the existing MenuHandler
preset-selection behavior while preserving use_preset and modem_preset
assignments.
In `@src/mesh/LR11x0Interface.cpp`:
- Around line 262-273: Update LR11x0Interface<T>::makeReconfigureParams and its
caller flow to preserve the boolean result from
RadioLibInterface::reconfigure(), then combine that baseSuccess with the
hardware reconfiguration result before returning from
LR11x0Interface::reconfigure(), matching the behavior of SX126xInterface and
RF95Interface.
In `@src/mesh/MeshService.cpp`:
- Around line 203-211: Update the FINALIZING branch around
loRaConfigApplyRequest and RadioInterface::finalizeConfigApply to add a bounded
timeout guard using request.requestedAtMsec, request.timeoutMsec, and
Throttle::isWithinTimespanMs. When the request is no longer tracked and the
timeout expires, transition loRaConfigApplyState to IDLE and call
adminModule->finalizeLoRaConfigApply(), preventing FINALIZING from repeating
indefinitely while preserving the existing completion path.
In `@src/mesh/RadioInterface.cpp`:
- Around line 1146-1170: Update the correction flow around hardwareConfigFor,
normalizeConfigLora, and corrected.config so correction always normalizes
bootConfig first, preserving any hardware-derived region such as LORA_24 instead
of accepting config.lora with UNSET. If the original region is non-UNSET, record
bootConfigCorrection from the corrected result and keep
bootConfigCorrectionPending behavior unchanged.
In `@src/mesh/RadioLibInterface.cpp`:
- Around line 560-567: Update the reception-restart guards in the shown transmit
and ISR_RX cases to use the negation of the disjunction, so startReceive() runs
only when neither configApplyReceptionIsHeld() nor configApplyTxInhibited() is
true. Preserve the surrounding setTransmitDelay(), handleReceiveInterrupt(), and
case flow.
- Around line 324-339: Update the beacon-restore branch in the
configuration-apply flow to check timedOut before calling
deferConfigApply(request); when the request has timed out, finish it with
RadioConfigApplyResult::TIMED_OUT and return. Preserve the existing retry and
delayed-notification behavior only for non-timed-out beacon restoration results.
In `@src/mesh/raspihttp/PiWebServer.cpp`:
- Line 375: Update the LOG_DEBUG call near the PUT request handling to use a
format specifier compatible with the size_t variable s, preserving the existing
byte-count message and avoiding an int conversion.
- Around line 659-665: Update PiWebServerThread::processPendingRequests() to
save the current VFS mountpoint before switching to configWeb.rootPath, then
restore the saved mountpoint after webAPI.processPendingRequests() completes
when no pending requests remain. Preserve the existing pending-request guard and
web request processing behavior.
- Around line 106-119: Update HttpAPI::submit to wait for request->completed
only up to the configured REQUEST_TIMEOUT_MS duration, using the shared
PendingRequest lifetime safely; on timeout, mark the request as cancelled as
needed and return failure so the HTTP layer responds with 503, while preserving
the existing completed/cancelled result handling. Add the requested chrono
include and timeout constant in the relevant declarations.
- Around line 198-217: Update HttpAPI::stopAcceptingRequests() to stop accepting
new requests while allowing requests already listed in inFlightRequests to
finish normally; do not mark or notify those active requests as
cancelled/completed. Add a drain mechanism that waits for
processPendingRequests() handlers to complete before stopAcceptingRequests()
returns, while preserving cancellation for queued requests removed from
requests.
In `@src/mesh/SX126xInterface.cpp`:
- Around line 330-337: Update setStandby() in src/mesh/SX126xInterface.cpp
(lines 330-337), LR11x0Interface.cpp (305-307), LR20x0Interface.cpp (266-269),
RF95Interface.cpp (294-297), and SX128xInterface.cpp (197-200) to call the bool
overload outside assert, store its return value, and assert that stored value so
the radio enters standby in NDEBUG builds.
In `@src/modules/AdminModule.cpp`:
- Around line 1224-1244: Snapshot the complete staged channel state before the
mutations in AdminModule::requestLoRaConfig, including pendingChannelConfig and
any related staged primary data, then restore that snapshot on every rejected
request path. Replace the conditional rollback after prepareLoRaConfig failure
so it always restores the prior state, and apply the same restoration in the
service->requestLoRaConfig failure branch.
- Around line 353-362: Remove the !RadioLibInterface::instance condition from
the LORA_24 rejection check in the meshtastic_AdminMessage_set_config case,
leaving the wideLora() validation intact so staged validation via
checkConfigRegion and normalizeConfigLora handles unavailable radio hardware
consistently.
In `@src/platform/portduino/SimRadio.cpp`:
- Around line 64-71: Update SimRadio::finalizeConfigApply to defer finalization
when a configuration-apply send is in flight, following the send-in-flight guard
and deferred-finalize pattern used by RadioLibInterface::serviceConfigApply.
Ensure the deferred path avoids releasing the TX barrier or starting another
send until the active onNotify operation has called releaseConfigApplyTxStart(),
while preserving the existing base finalization and queued-transmit delay
behavior.
In `@test/test_admin_radio/test_main.cpp`:
- Around line 2843-2873: Install the replacement ScriptedConfigApplyRadio and
assign scriptedRadio before any assertions that can abort in
test_setPrimaryChannel_activeTxDefersRadioApplyUntilSendingCompletes. Apply the
same ordering fix in
test_editTransaction_primaryChannelDefersAndReconfiguresOnce, or update setUp to
recreate the radio whenever scriptedRadio is nullptr, ensuring subsequent tests
cannot dereference a cleared global.
In `@test/test_radio/test_main.cpp`:
- Around line 134-149: Prevent out-of-bounds scripted-result access in both
radio test doubles: at test/test_radio/test_main.cpp lines 134-149, bound the
index used by reconfigure() for appliedRegions and reconfigureResults; at lines
53-57, bound reconfigureResults access in the other reconfigure implementation.
Preserve scripted values and return the final scripted value after the sequence
is exhausted.
---
Outside diff comments:
In `@src/mesh/MeshPacketQueue.cpp`:
- Around line 160-221: Add an assert at the start of
MeshPacketQueue::replaceLowerPriorityPacket to require evicted is non-null
before any branch can dereference it. Keep the existing replacement logic
unchanged and use assert() to enforce this caller invariant.
In `@src/mesh/SX128xInterface.cpp`:
- Around line 163-195: Update SX128xInterface<T>::setStandby so standby failure
does not bypass cleanup: ensure RXEN/TXEN are de-asserted, interrupts are
disabled, and the existing state cleanup runs before returning the
lora.standby() error. Preserve the normal completePacket handling and successful
return behavior.
In `@src/modules/AdminModule.cpp`:
- Around line 846-939: Update the non-license-change path in
AdminModule::handleSetOwner so that when pendingOwnerConfig.active is true, it
also assigns the modified candidate to pendingOwnerConfig.candidate before or
alongside updating owner. Preserve the existing live-owner update and save
behavior, ensuring later completeLoRaConfigApply uses the latest name values.
---
Nitpick comments:
In `@src/graphics/draw/MenuHandler.cpp`:
- Around line 129-138: Update requestMenuLoRaConfig so a failed
adminModule->requestMenuLoRaConfig call displays a short user-facing banner in
addition to logging and returning false. Ensure rejected menu requests provide
visible feedback while successful requests retain the existing behavior.
In `@src/mesh/Channels.cpp`:
- Around line 393-436: Extract a static channel-repair helper from the
duplicated logic in setChannelInFile, covering missing settings reset, legacy
“Default” name cleanup, and primary-slot restoration. Update fixupChannel and
onConfigChanged to call this helper so both live and file-based paths share the
same invariants, while preserving generateHash() only in the live path and
keeping publishPendingChannels() behavior unchanged.
In `@src/mesh/LR11x0ConfigApply.h`:
- Around line 59-74: Clarify the overloaded setFrequency call in
lr11x0SetFrequencyForBand by documenting that targetWideBand is intentionally
passed as the skipCalibration argument, or rename the local flag to reflect that
dual meaning while preserving the existing wide-band behavior.
- Around line 38-42: Update lr11x0BandPolicyFor so the resolved wide-band
condition, region.wideLora && supportsWideBand, determines both the wide-band
flag and selected power cap; use subGhzMaxPower whenever that condition is
false.
In `@src/mesh/LR11x0Interface.cpp`:
- Around line 209-234: Update reinitializeForBand’s failure reporting so
lr11x0BeginForBand failures and setRegulatorDCDC failures identify their actual
operations instead of using STANDBY and OUTPUT_POWER. Add dedicated
LR11x0ApplyStep values if the existing reporting requires enum steps, and ensure
the log at the failure path uses those corrected labels.
In `@src/mesh/MeshPacketQueue.h`:
- Around line 22-25: Update the enqueue documentation in MeshPacketQueue to
describe the ownership behavior of the optional evicted parameter: when
provided, it receives the displaced packet and the caller must release it; when
nullptr, enqueue releases the displaced packet internally. Keep the existing
dropped parameter documentation and enqueue signature unchanged.
In `@src/mesh/RadioConfigApply.h`:
- Around line 20-23: Default-initialize previous, candidate, requestedAtMsec,
and timeoutMsec in the request structure to match the initialization pattern of
its other members, while preserving MeshService::requestLoRaConfig behavior.
In `@src/mesh/RadioInterface.cpp`:
- Around line 1049-1058: Update RadioInterface::serviceConfigApply to use the
existing Throttle helper for the request timeout check instead of raw
nowMsec/requestedAtMsec subtraction. Confirm Throttle’s API and preserve the
TIMED_OUT result when the timeout elapses; remove nowMsec from the function if
it becomes unused.
- Around line 1626-1628: Replace the conditional/comma-expression initialization
of loraConfig near stagedApply with an explicit branch: copy
*configApplyLoraConfig into stagedConfig and bind loraConfig to stagedConfig
when stagedApply is true; otherwise bind it directly to config.lora. Preserve
the existing reference semantics and behavior.
In `@src/mesh/RadioLibInterface.cpp`:
- Line 301: Replace the raw elapsed-time arithmetic in the timeout check with
the established Throttle mechanism, using the timeout value from
request->timeoutMsec and preserving the existing timedOut behavior. Keep nowMsec
only if it is required by other call sites or logic.
- Around line 617-659: Extract the beacon-specific TX reconfiguration and
scheduling logic from the surrounding transmit branch into a helper such as
handleBeaconTxScheduling(txp), returning whether the caller should exit early.
Keep the helper’s implementation under one `#if` !MESHTASTIC_EXCLUDE_BEACON guard,
remove the interleaved conditional braces and internal `#endif` from the caller,
and preserve the existing failed, in-progress, channel-active, and queue-send
behavior.
- Around line 606-616: Update the invalid beacon-config branch around
dequeueTxPacketIfConfigApplyAllowed() to call setTransmitDelay() when the
dequeue returns nullptr, ensuring a retry is scheduled when
claimConfigApplyTxStart() fails. Preserve the existing drop, cleanup, and delay
behavior for successfully dequeued packets without adding duplicate scheduling.
In `@src/mesh/RadioLibInterface.h`:
- Around line 281-299: Reorganize the declarations around
scheduleBeaconRestoreRetry(), startSend(), and getQueueStatus() to avoid
switching visibility from protected to private and back within this short
section. Move each member into the nearest existing section matching its
intended visibility, preserving access semantics while removing the redundant
access-specifier churn.
In `@src/mesh/raspihttp/PiWebServer.cpp`:
- Around line 409-421: Make the drain-all path in the request handler reachable
by assigning the incoming request’s all-values parameter to valueAll, or remove
that path if only single-protobuf responses are supported. If retaining it,
rework the loop so the response body is set once with the complete drained
payload, and eliminate the conflicting duplicate response setup while preserving
the 503 handling from submitFromRadio.
In `@src/mesh/RF95Interface.cpp`:
- Around line 276-292: Update RF95Interface::setStandby so it resets
activeReceiveStart to 0 alongside isReceiving = false, matching the standby
state cleanup used by the other radio drivers; leave the existing error handling
and transmission cleanup unchanged.
In `@src/modules/AdminModule.cpp`:
- Around line 516-560: Extract the duplicated staged-commit flow into a private
helper such as commitStagedEdits(int editedSegments, bool shouldReboot),
including normalization, apply-busy handling, requestLoRaConfig, rollback, and
unrelated-segment saving. Update src/modules/AdminModule.cpp lines 516-560 and
2403-2449 to call the helper while preserving each site’s shouldReboot behavior;
also verify whether pendingOwnerConfig.active must be included in the
commit-path staged-configuration check.
- Around line 1169-1222: Extract the duplicated previous-slot selection,
channel_num collapse, and preserveDefaultFrequencySlot assignment from
AdminModule::requestLoRaConfig and AdminModule::requestMenuLoRaConfig into one
private helper. Have both methods use that helper while retaining only the menu
transition-specific field changes in requestMenuLoRaConfig, and preserve each
method’s existing requestLoRaConfig invocation parameters.
In `@src/modules/MeshBeaconModule.cpp`:
- Around line 77-92: Document the return contract of
MeshBeaconModule::getHomeRadioConfig in its declaration: return true when
temporary beacon override data is available and applied, and false when no
override data exists while leaving the current configuration in place. Keep the
existing call in AdminModule unchanged since the documented beacon-override
semantics make ignoring the return value intentional.
In `@src/modules/MeshBeaconModule.h`:
- Around line 41-47: Update the reconfigureForBeaconTX documentation to define
all four RadioConfigResult outcomes: RECONFIGURED requires rerunning the CCA
transmit delay, FAILED prevents transmission, IN_PROGRESS means another
transition owns the radio and the caller should retry later, and UNCHANGED means
no radio switch was performed.
In `@src/platform/portduino/SimRadio.cpp`:
- Around line 235-251: Update the transmit handling around txQueue.dequeue() in
the SimRadio notify path to retain assert(txp) while guarding all
packet-dependent operations with txp != nullptr. Match
RadioLibInterface::onNotify behavior so release builds skip startSend, airtime
accounting, and notification when dequeue returns null, while preserving
configuration cleanup.
In `@test/test_mesh_beacon/test_main.cpp`:
- Around line 1747-1753: Update tearDown to reset MeshBeaconModule’s static
temporary-config flag, pending-restore flag, and target-radio-settings table in
addition to the existing module cleanup. Use the class’s existing reset/clear
mechanisms so cleanup always runs after assertion failures and each test begins
with pristine beacon state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c0940f-0432-4a60-8705-f9b806ed933a
📒 Files selected for processing (43)
src/Power.cppsrc/concurrency/NotifiedWorkerThread.cppsrc/concurrency/NotifiedWorkerThread.hsrc/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/main.cppsrc/mesh/Channels.cppsrc/mesh/Channels.hsrc/mesh/LR11x0ConfigApply.hsrc/mesh/LR11x0Interface.cppsrc/mesh/LR11x0Interface.hsrc/mesh/LR20x0Interface.cppsrc/mesh/LR20x0Interface.hsrc/mesh/MeshPacketQueue.cppsrc/mesh/MeshPacketQueue.hsrc/mesh/MeshService.cppsrc/mesh/MeshService.hsrc/mesh/NodeDB.cppsrc/mesh/RF95Interface.cppsrc/mesh/RF95Interface.hsrc/mesh/RadioConfigApply.hsrc/mesh/RadioInterface.cppsrc/mesh/RadioInterface.hsrc/mesh/RadioLibInterface.cppsrc/mesh/RadioLibInterface.hsrc/mesh/STM32WLE5JCInterface.cppsrc/mesh/SX126xInterface.cppsrc/mesh/SX126xInterface.hsrc/mesh/SX128xInterface.cppsrc/mesh/SX128xInterface.hsrc/mesh/raspihttp/PiWebServer.cppsrc/mesh/raspihttp/PiWebServer.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/modules/MeshBeaconModule.cppsrc/modules/MeshBeaconModule.hsrc/platform/portduino/SimRadio.cppsrc/platform/portduino/SimRadio.htest/support/AdminModuleTestShim.htest/test_admin_radio/test_main.cpptest/test_http_content_handler/test_main.cpptest/test_mesh_beacon/test_main.cpptest/test_radio/test_main.cpp
| applyLoraRegion(pendingRegion, | ||
| selected == 1 ? AdminModule::MenuLoRaTransition::EXIT_LICENSED : AdminModule::MenuLoRaTransition::NONE); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect banner dismissal/timeout semantics for overlay banners.
set -euo pipefail
fd -t f 'BannerOverlay*' src | xargs -r -I{} sh -c 'echo "== {}"; cat -n "{}"'
rg -n -C6 'bannerCallback' src/graphics --glob '!**/MenuHandler.cpp' | head -100Repository: meshtastic/firmware
Length of output: 7940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MenuHandler.cpp relevant section =="
sed -n '310,365p' src/graphics/draw/MenuHandler.cpp | cat -n -v
echo "== NotificationRenderer banner callback selection logic =="
rg -n -C8 'drawBannercallback|alertBannerCallback|curSelected|optionsEnumPtr|optionsArrayPtr|showOverlayBanner' src/graphics --glob '!**/MenuHandler.cpp' | head -220
echo "== MenuHandler LoraRegionPicker related definitions/usages =="
rg -n -C4 'pendingRegion|LoraRegionPicker|confirmOptions|showOverlayBanner' src/graphics src --glob '!**/NotificationRenderer.cpp' | head -220Repository: meshtastic/firmware
Length of output: 33128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== NotificationRenderer.cpp drawBannercallback and selection/dismiss handling =="
fd -t f 'NotificationRenderer.cpp' src | xargs -r -I{} sh -c 'echo "== {}"; rg -n -C8 "void NotificationRenderer::drawBannercallback|alertBannerCallback|curSelected|alertBannerUntil|pauseBanner|current_notification_type|SELECT|UP|DOWN|ENTER" "{}" | head -300'
echo "== Header declarations for banner state/callbacks =="
fd -t h 'NotificationRenderer.h|Screen.h' src | xargs -r -I{} sh -c 'echo "== {}"; rg -n -C4 "alertBannerCallback|curSelected|alertBannerUntil|pauseBanner|optionsCount|durationMs|InitialSelected" "{}"'Repository: meshtastic/firmware
Length of output: 13647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== NotificationRenderer.cpp selection_picker option callback section =="
sed -n '260,370p' src/graphics/draw/NotificationRenderer.cpp | cat -n -v
echo "== NotificationRenderer helper: isOverlayBannerShowing/relevant option handling =="
rg -n -C5 'isOverlayBannerShowing|drawSelectionPicker|alertBannerCallback\\(curSelected\\)|resetBanner\\(\\);\\s*$' src/graphics/draw/NotificationRenderer.cpp src/graphics/draw/NotificationRenderer.h
echo "== all selection_callback callers of showOverlayBanner =="
rg -n -C3 'showOverlayBanner\\(.*bannerCallback|optionsEnumPtr|alertBannerCallback\\(curSelected\\)|alertBannerCallback\\(selected\\)|alertBannerCallback\\(' src/graphics src --glob '!**/MenuHandler.cpp'Repository: meshtastic/firmware
Length of output: 8570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all selection_picker callers =="
rg -n -C3 'notificationTypeEnum::selection_picker|InitialSelected|optionsEnumPtr|optionsArrayPtr' src/graphics/src src/graphics/draw src/graphics/draw/MenuHandler.cpp src/graphics/draw/NotificationRenderer.cpp
echo "== NotificationRenderer drawAlertBannerOverlay if option logic exists anywhere =="
rg -n -C8 'drawAlertBannerOverlay|alertBannerCallback|curSelected|resetBanner\\(\\)' src/graphics/draw/NotificationRenderer.cpp src/graphics/draw/NotificationRenderer.h
echo "== static initializer behavior probe in repository headers =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/graphics/Screen.h')
print(p.read_text(errors='ignore').split('struct BannerOverlayOptions')[1].split('}')[0])
PYRepository: meshtastic/firmware
Length of output: 50375
Keep licensed to normal licensed mode if the prompt times out/dismisses.
licensedToNormalConfirmMenu() has no notificationType = text_banner, so drawAlertBannerOverlay() runs and calls alertBannerCallback(curSelected) on selection. Since LicensedToNormalConfirmMenu lacks an explicit text_banner, the timeout path does not call the callback, but if an option is selected while the banner is focused on Keep licensed, it applies pendingRegion as an unlicensed mode.
🤖 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/graphics/draw/MenuHandler.cpp` around lines 349 - 351, Update
licensedToNormalConfirmMenu() and its selection handling so a timeout or
dismissal, as well as selecting “Keep licensed” while the alert banner is
focused, preserves the normal licensed mode instead of applying pendingRegion as
an unlicensed mode. Ensure the alert-banner callback path and default transition
both use the licensed transition, while retaining the explicit exit behavior for
the other option.
Hmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm....
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/AdminModule.cpp (1)
532-542: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the edit transaction staged when queuing fails.
service->requestLoRaConfig()can reject aftertryBeginLoRaConfigApply()succeeds. Lines 527-529 close the transaction and clear deferred segments before that call. Lines 536-540 then discard all staged channel, owner, LoRa, and menu state.Only close the transaction and clear staged state after the request is accepted. On rejection, cancel the apply, retain the candidates and deferred segments, refresh transaction activity, and allow retry. Add a queue-rejection test.
🤖 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 532 - 542, Update the LoRa apply flow around tryBeginLoRaConfigApply() and requestLoRaConfig() so a rejected queue request cancels the apply without clearing the staged channel, owner, LoRa, or menu candidates or closing the edit transaction. Refresh transaction activity and preserve the staged state so the user can retry; only clear deferred segments and close the transaction after requestLoRaConfig() is accepted. Add a test covering queue rejection and retry with the staged candidates retained.
🤖 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.
Outside diff comments:
In `@src/modules/AdminModule.cpp`:
- Around line 532-542: Update the LoRa apply flow around
tryBeginLoRaConfigApply() and requestLoRaConfig() so a rejected queue request
cancels the apply without clearing the staged channel, owner, LoRa, or menu
candidates or closing the edit transaction. Refresh transaction activity and
preserve the staged state so the user can retry; only clear deferred segments
and close the transaction after requestLoRaConfig() is accepted. Add a test
covering queue rejection and retry with the staged candidates retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b043a87b-c400-4f4f-a241-a8053dd421bd
📒 Files selected for processing (22)
src/graphics/draw/MenuHandler.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/mesh/LR11x0ConfigApply.hsrc/mesh/LR11x0Interface.cppsrc/mesh/LR11x0Interface.hsrc/mesh/LR20x0Interface.cppsrc/mesh/MeshPacketQueue.cppsrc/mesh/MeshPacketQueue.hsrc/mesh/RF95Interface.cppsrc/mesh/RadioInterface.cppsrc/mesh/RadioLibInterface.cppsrc/mesh/SX126xInterface.cppsrc/mesh/SX128xInterface.cppsrc/mesh/Throttle.cppsrc/mesh/Throttle.hsrc/mesh/raspihttp/PiWebServer.cppsrc/mesh/raspihttp/PiWebServer.hsrc/modules/AdminModule.cppsrc/platform/portduino/SimRadio.cpptest/test_admin_radio/test_main.cpptest/test_http_content_handler/test_main.cpptest/test_radio/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp
- src/mesh/SX126xInterface.cpp
- src/mesh/RadioLibInterface.cpp
- src/mesh/raspihttp/PiWebServer.cpp
- src/mesh/MeshPacketQueue.cpp
- src/mesh/LR20x0Interface.cpp
- src/mesh/RF95Interface.cpp
- src/graphics/draw/MenuHandler.cpp
- src/mesh/LR11x0ConfigApply.h
- src/mesh/SX128xInterface.cpp
- src/mesh/raspihttp/PiWebServer.h
- src/mesh/MeshPacketQueue.h
- src/platform/portduino/SimRadio.cpp
- test/test_radio/test_main.cpp
- src/mesh/RadioInterface.cpp
This is way too many changes for one PR
|
We already do this?
Not sure that we don't already do this
Is this someone else's PR repackaged?
This is a separate piece of work, which I was working up to in the "fewer reloads / tidyup" PR.
Pretty sure this is part of either someone else's PR or my fewer reloads work. Also worth noting that the "wide" lora is supposed to segregate sub- and 2.4-ghz BW modes, albeit crudely. If you want to tilt at that windmill, this isn't the way to do it. You've stated testing, but have you confirmed this "leaving node unresponsive" state?
This is not the way to solve that issue.
What does "1600 is normalized to a supported LORA_24 profile" mean?
Pretty sure that's why we have a billion radio interface files.
Pretty sure this is why I pulled all my hair out over checkOrClampConfigLora but if you have an improvement, maybe package that up separately? |
|
Thanks for the scope feedback. I split the reported 2.4 GHz bandwidth failure into #11317, a focused five-file change that only adds active-radio bandwidth validation and tests.\n\nThe replacement was validated on a TLORA T3S3/SX128x: invalid 125 kHz was corrected to the LORA_24 default without losing responsiveness, and 200/400/800/1600 all applied successfully. The original SHORT_FAST configuration and identity were restored and survived reboot.\n\nPRs #11181 and #11279 remain the owners for restart reduction and live LR20x0 band switching. I am closing this broad PR to avoid overlapping those efforts. |
3 participants
Summary
Why
2.4 GHz radios could be offered or sent sub-GHz bandwidth values. Values such as 125 kHz could reach an SX128x/LR1121 driver even though the wide-band hardware only accepts its discrete 203.125/406.25/812.5 kHz modes, leaving the node unresponsive. LR1121 band changes also required a reboot because a live reload did not safely coordinate the RF path and calibration sequence.
The previous reload path could mutate the published configuration before the radio accepted it, interrupt an active packet, and persist a value after a failed hardware apply. This change makes the radio apply transactional and reports the terminal result to the initiating configuration transaction.
Compatibility
Validation
1a797a7: 945/945 native tests passed across 43 suites, plus cleanmuzi-base,tlora-t3s3-v1, andtbeam-s3-corebuilds1a797a7: focused Admin/radio transaction 155/155, radio 66/66, HTTP request handling 8/8, and MeshBeacon 61/61 tests passed;picowandpico2wcppcheck passed8458d2f: Muzi Base (LR1121), TLORA T3S3 (SX128x), and T-Beam S3 Core (LR1121)Attestations
Summary by CodeRabbit