fix(model): gate TAK module config on firmware 2.8.0 - #6435
Merged
Conversation
The v2.7.19 gate was set on protobuf availability rather than firmware support. In v2.7.x, AdminModule::handleSetModuleConfig() has no case for the `tak` submessage, so the node ACKs the write and reboots without storing anything, and NodeDB::saveToDisk() never sets `has_tak`. The result is a settings editor that appears to save, forces a reboot (RebootBehavior.ALWAYS), and always reads back as unspecified. Raise the floor to 2.8.0, matching meshtastic/firmware#11216 (labelled 2.8), which adds the admin set case, the has_tak persist flag, and the TAK_CONFIG remote-admin read case. Refs #6430 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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 (2)
📝 WalkthroughWalkthroughThe TAK configuration capability gate now requires firmware 2.8.0. The obsolete 2.7.19 constant was removed, and tests verify support remains disabled through 2.7.26 and enables at 2.8.0. ChangesTAK configuration capability
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #6430
Why
supportsTakConfigwas gated on2.7.19, but that floor was set on protobuf availability, not firmware support. No released firmware implements the TAK module config write path, so the app has been offering an editor that cannot work:AdminModule::handleSetModuleConfig()in v2.7.x has no case for thetaksubmessage. The message falls through the switch, and the handler still runssaveChanges(SEGMENT_MODULECONFIG, shouldReboot); return true;— so the node ACKs success and reboots, having stored nothing. The value never even takes effect in RAM.NodeDB::saveToDisk()never setsmoduleConfig.has_tak = true, so even a populated struct would be omitted by nanopb on flash write.PhoneAPISTATE_SEND_MODULECONFIGhas notakcase, so the node never echoes a TAK config back — which is what the app renders as "Unspecified".Net user-visible effect (#6430): pick a Team Color and Member Role → Save → node reboots (the screen is
RebootBehavior.ALWAYS) → both fields read "Unspecified" again. The app was doing everything right —TAKConfigItemList.kt→RadioConfigViewModel(per-submessage merge preservestak) →AdminControllerImplsendsAdminMessage.set_module_configwithModuleConfig.tak(field 16) populated — so there was no app-side fix for the persistence itself, only this bad capability floor.Change
Raise the floor to
2.8.0, matching the firmware fix in meshtastic/firmware#11216 (labelled2.8), which adds all three missing pieces:takset case inhandleSetModuleConfig()case meshtastic_ModuleConfig_tak_tag:+moduleConfig.has_tak = truesaveToDisk()never setshas_takmoduleConfig.has_tak = true;in theSEGMENT_MODULECONFIGblockTAK_CONFIGread casecase meshtastic_AdminMessage_ModuleConfigType_TAK_CONFIG:Also removes the now-unused
V2_7_19constant, and records the reasoning in KDoc so the floor doesn't get lowered back onto protobuf availability.Tests
CapabilitiesTest.supportsTakConfig_requires_V2_7_19asserted2.7.19→ true and would have failed. Renamed tosupportsTakConfig_requires_V2_8_0, and2.7.26added as an explicit false case since that is the firmware version in the bug report.Verified:
spotlessApply spotlessCheck :core:model:detekt :core:model:allTests→BUILD SUCCESSFUL.Merge dependency
If 2.8.0 ends up shipping without #11216, the floor needs revisiting. The repo's
UNRELEASED = DeviceVersion("9.9.9")idiom (used bycanRequestNeighborInfo) is the alternative if a hard gate is preferred over a version bet.Related
AdminModule.cppalready handlesSET_MODULE_CONFIGfor TAKNot related, despite surface similarity: the
Router::sendLocalloopback regression on firmwaredevelop(d6b12ea3f). Different failure mode — admin round-trips in #6430 succeed (the node ACKs and reboots); this is a plain missing switch case that predates that range.🤖 Generated with Claude Code
Summary by CodeRabbit