Fix '--get security' (incorrect AdminMessage.ConfigType value).#907
Fix '--get security' (incorrect AdminMessage.ConfigType value).#907cpatulea wants to merge 1 commit intomeshtastic:masterfrom
Conversation
requestConfig was assuming that the order of fields in meshtastic.LocalConfig matches the order of enum values in AdminMessage.ConfigType. This is true for 'device', 'position', etc. but is NOT true for 'security' due to the intervening 'version' field. Look up config fields by name, not index, to prevent this error in the future. LocalConfig.security was introduced in meshtastic/protobufs#553
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #907 +/- ##
==========================================
- Coverage 59.82% 59.81% -0.01%
==========================================
Files 24 24
Lines 4329 4328 -1
==========================================
- Hits 2590 2589 -1
Misses 1739 1739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes an incorrect --get security behavior in the Meshtastic Python CLI/library by ensuring Node.requestConfig() maps LocalConfig fields to AdminMessage.ConfigType by enum name rather than relying on protobuf field ordering.
Changes:
- Update
Node.requestConfig()to deriveAdminMessage.ConfigTypefrom theLocalConfigfield name (fixingsecuritymapping). - Keep module config requests using the prior index-based behavior.
| msgIndex = configType.index | ||
| if configType.containing_type.name == "LocalConfig": | ||
| p.get_config_request = msgIndex | ||
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") |
There was a problem hiding this comment.
AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") will raise ValueError if a LocalConfig field doesn’t have a corresponding ConfigType enum (e.g. LocalConfig.version). Because requestConfig() is reachable from the CLI via --get <field>, this becomes an uncaught exception for a user typo/unsupported field. Consider catching the error (or checking membership in the enum) and exiting with a clear message (or explicitly rejecting version).
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") | |
| enum_name = configType.name.upper() + "_CONFIG" | |
| # Validate that the constructed enum name exists to avoid ValueError from .Value() | |
| if enum_name not in admin_pb2.AdminMessage.ConfigType.keys(): | |
| our_exit( | |
| f"Unsupported configuration field '{configType.name}' cannot be requested.", | |
| 1, | |
| ) | |
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(enum_name) |
| else: | ||
| p.get_module_config_request = msgIndex | ||
| p.get_module_config_request = configType.index |
There was a problem hiding this comment.
The PR description says config fields should be looked up by name (not index) to prevent future enum/field-order mismatches, but the module-config branch still uses configType.index for get_module_config_request. Either extend the name-based approach to ModuleConfigType as well (likely via an explicit mapping table, since some names differ like external_notification vs EXTNOTIF_CONFIG), or update the PR description to reflect that only LocalConfig is addressed.
| if configType.containing_type.name == "LocalConfig": | ||
| p.get_config_request = msgIndex | ||
| p.get_config_request = admin_pb2.AdminMessage.ConfigType.Value(configType.name.upper() + "_CONFIG") | ||
| else: | ||
| p.get_module_config_request = msgIndex | ||
| p.get_module_config_request = configType.index |
There was a problem hiding this comment.
This change fixes a subtle mapping bug (LocalConfig.security vs ConfigType.SECURITY_CONFIG) but requestConfig() behavior isn’t covered by an active unit test (the existing requestConfig test in meshtastic/tests/test_node.py is commented out). Adding a unit test that asserts requesting the security field descriptor sets AdminMessage.get_config_request to SECURITY_CONFIG would prevent regressions as protobufs evolve.
requestConfig was assuming that the order of fields in meshtastic.LocalConfig matches the order of enum values in AdminMessage.ConfigType. This is true for 'device', 'position', etc. but is NOT true for 'security' due to the intervening 'version' field.
Look up config fields by name, not index, to prevent this error in the future.
LocalConfig.security was introduced in meshtastic/protobufs#553