Add sensor-config commands (guidance needed)#903
Add sensor-config commands (guidance needed)#903oscgonfer wants to merge 2 commits intomeshtastic:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new CLI/admin path to send sensor configuration commands (SCD4x and SEN5x) over the existing ADMIN_APP mechanism, intended to configure the new Air Quality module sensors.
Changes:
- Add
Node.sensorConfig()to construct and sendAdminMessage.sensor_configcommands. - Add
--sensor-configCLI flag and dispatch it toNode.sensorConfig().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
meshtastic/node.py |
Introduces sensorConfig() to map CLI tokens into AdminMessage.sensor_config.* protobuf fields and send via _sendAdmin(). |
meshtastic/__main__.py |
Adds --sensor-config argparse option and invokes sensorConfig() from onConnected(). |
| def sensorConfig(self, commands: List = None): | ||
| """Send a sensor configuration command""" | ||
| self.ensureSessionKey() | ||
|
|
||
| p = admin_pb2.AdminMessage() | ||
| if any(['scd4x_config' in command for command in commands]): | ||
| cleanup_commands = [command.replace('scd4x_config.', '') for command in commands] | ||
|
|
||
| if 'factory_reset' in cleanup_commands: | ||
| print ("Performing factory reset on SCD4X") | ||
| p.sensor_config.scd4x_config.factory_reset = True | ||
| else: | ||
| if 'set_asc' in cleanup_commands: | ||
| if cleanup_commands[cleanup_commands.index('set_asc')+1] == "true": | ||
| p.sensor_config.scd4x_config.set_asc = True | ||
| print ("Setting SCD4X ASC mode") | ||
| elif cleanup_commands[cleanup_commands.index('set_asc')+1] == "false": | ||
| p.sensor_config.scd4x_config.set_asc = False | ||
| print ("Setting SCD4X FRC mode") | ||
| else: | ||
| print( | ||
| f'Not valid argument for sensor_config.scd4x_config.set_asc' | ||
| ) | ||
| if 'set_target_co2_conc' in cleanup_commands: | ||
| try: | ||
| target_co2_conc = int(cleanup_commands[cleanup_commands.index('set_target_co2_conc')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for target CO2 conc' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X target CO2 conc to {target_co2_conc}") | ||
| p.sensor_config.scd4x_config.set_target_co2_conc = target_co2_conc | ||
| send_command = True | ||
| if 'set_temperature' in cleanup_commands: | ||
| try: | ||
| temperature = float(cleanup_commands[cleanup_commands.index('set_temperature')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference temperature' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference temperature to {temperature}") | ||
| p.sensor_config.scd4x_config.set_temperature = temperature | ||
| send_command = True | ||
| if 'set_altitude' in cleanup_commands: | ||
| try: | ||
| altitude = int(cleanup_commands[cleanup_commands.index('set_altitude')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference altitude' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference altitude to {altitude}") | ||
| p.sensor_config.scd4x_config.set_altitude = altitude | ||
| if 'set_ambient_pressure' in cleanup_commands: | ||
| try: | ||
| ambient_pressure = int(cleanup_commands[cleanup_commands.index('set_ambient_pressure')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference ambient pressure' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference ambient pressure to {ambient_pressure}") | ||
| p.sensor_config.scd4x_config.set_ambient_pressure = ambient_pressure | ||
|
|
||
| if any(['sen5x_config' in command for command in commands]): | ||
| cleanup_commands = [command.replace('sen5x_config.', '') for command in commands] | ||
| if 'set_one_shot_mode' in cleanup_commands: | ||
| if cleanup_commands[cleanup_commands.index('set_one_shot_mode')+1] == "true": | ||
| p.sensor_config.sen5x_config.set_one_shot_mode = True | ||
| print ("Setting SEN5X one shot mode") | ||
| elif cleanup_commands[cleanup_commands.index('set_one_shot_mode')+1] == "false": | ||
| p.sensor_config.sen5x_config.set_one_shot_mode = False | ||
| print ("Setting SEN5X continuous mode") | ||
| else: | ||
| print( | ||
| f'Not valid argument for sensor_config.sen5x_config.set_one_shot_mode' | ||
| ) | ||
|
|
||
| # How to represent a HANDLED event? | ||
| if self == self.iface.localNode: | ||
| onResponse = None | ||
| else: | ||
| onResponse = self.onAckNak | ||
|
|
||
| if p.ByteSize(): | ||
| # TODO - Should this require a response? | ||
| return self._sendAdmin(p, onResponse=onResponse) | ||
| else: |
There was a problem hiding this comment.
New parsing/sending behavior in sensorConfig() isn’t covered by unit tests. The repo already has meshtastic/tests/test_node.py exercising other admin methods (e.g. reboot(), shutdown()). Please add tests that validate: (1) valid tokens set the expected protobuf fields, and (2) invalid/missing values are handled without raising (especially the IndexError cases).
| if args.sensor_config: | ||
| closeNow = True | ||
| waitForAckNak = True | ||
| interface.getNode(args.dest, False, **getNode_kwargs).sensorConfig(args.sensor_config) | ||
|
|
There was a problem hiding this comment.
--sensor-config is executed after the global ACK/NAK wait block, but it sets waitForAckNak = True only inside this late block. As a result, remote --sensor-config commands won’t trigger the “Waiting for an acknowledgment…” path and the interface may close immediately after sending. Please move this handling up with the other remote-admin actions (before the ACK/NAK wait section), or explicitly call iface.waitForAckNak() right after sensorConfig() when --dest is remote.
| group.add_argument( | ||
| "--sensor-config", | ||
| help="Send a sensor admin command to configure sensor parameters.", | ||
| action="store", | ||
| nargs='+', | ||
| default=None | ||
| ) |
There was a problem hiding this comment.
New CLI surface area (--sensor-config) isn’t covered by the existing meshtastic/tests/test_main.py CLI tests (which already validate many other flags). Please add at least a basic unit test that initParser() accepts --sensor-config and that onConnected() routes it to Node.sensorConfig() with the expected argument list.
| def sensorConfig(self, commands: List = None): | ||
| """Send a sensor configuration command""" | ||
| self.ensureSessionKey() | ||
|
|
There was a problem hiding this comment.
sensorConfig() defaults commands to None but immediately iterates over it (for command in commands), which will raise a TypeError if the method is called without CLI args (or with an empty value from other API consumers). Please guard against commands is None (and possibly len(commands)==0) early and exit with a clear message or exception.
| if not commands: | |
| print("No sensor configuration commands were provided.") | |
| return |
| if 'factory_reset' in cleanup_commands: | ||
| print ("Performing factory reset on SCD4X") | ||
| p.sensor_config.scd4x_config.factory_reset = True | ||
| else: | ||
| if 'set_asc' in cleanup_commands: | ||
| if cleanup_commands[cleanup_commands.index('set_asc')+1] == "true": | ||
| p.sensor_config.scd4x_config.set_asc = True | ||
| print ("Setting SCD4X ASC mode") | ||
| elif cleanup_commands[cleanup_commands.index('set_asc')+1] == "false": | ||
| p.sensor_config.scd4x_config.set_asc = False | ||
| print ("Setting SCD4X FRC mode") | ||
| else: | ||
| print( | ||
| f'Not valid argument for sensor_config.scd4x_config.set_asc' | ||
| ) | ||
| if 'set_target_co2_conc' in cleanup_commands: | ||
| try: | ||
| target_co2_conc = int(cleanup_commands[cleanup_commands.index('set_target_co2_conc')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for target CO2 conc' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X target CO2 conc to {target_co2_conc}") | ||
| p.sensor_config.scd4x_config.set_target_co2_conc = target_co2_conc | ||
| send_command = True | ||
| if 'set_temperature' in cleanup_commands: | ||
| try: | ||
| temperature = float(cleanup_commands[cleanup_commands.index('set_temperature')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference temperature' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference temperature to {temperature}") | ||
| p.sensor_config.scd4x_config.set_temperature = temperature | ||
| send_command = True | ||
| if 'set_altitude' in cleanup_commands: | ||
| try: | ||
| altitude = int(cleanup_commands[cleanup_commands.index('set_altitude')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference altitude' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference altitude to {altitude}") | ||
| p.sensor_config.scd4x_config.set_altitude = altitude | ||
| if 'set_ambient_pressure' in cleanup_commands: | ||
| try: | ||
| ambient_pressure = int(cleanup_commands[cleanup_commands.index('set_ambient_pressure')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference ambient pressure' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference ambient pressure to {ambient_pressure}") |
There was a problem hiding this comment.
Several options read their value via cleanup_commands[cleanup_commands.index(name) + 1] without checking that a following token exists. If a user passes an option as the last argument (e.g. scd4x_config.set_asc with no value), this will raise IndexError and crash the CLI. Please validate that the option has a value (and ideally that the value token isn’t another *_config.* key) before indexing.
| if 'factory_reset' in cleanup_commands: | |
| print ("Performing factory reset on SCD4X") | |
| p.sensor_config.scd4x_config.factory_reset = True | |
| else: | |
| if 'set_asc' in cleanup_commands: | |
| if cleanup_commands[cleanup_commands.index('set_asc')+1] == "true": | |
| p.sensor_config.scd4x_config.set_asc = True | |
| print ("Setting SCD4X ASC mode") | |
| elif cleanup_commands[cleanup_commands.index('set_asc')+1] == "false": | |
| p.sensor_config.scd4x_config.set_asc = False | |
| print ("Setting SCD4X FRC mode") | |
| else: | |
| print( | |
| f'Not valid argument for sensor_config.scd4x_config.set_asc' | |
| ) | |
| if 'set_target_co2_conc' in cleanup_commands: | |
| try: | |
| target_co2_conc = int(cleanup_commands[cleanup_commands.index('set_target_co2_conc')+1]) | |
| except ValueError: | |
| print( | |
| f'Invalid value for target CO2 conc' | |
| ) | |
| return | |
| else: | |
| print (f"Setting SCD4X target CO2 conc to {target_co2_conc}") | |
| p.sensor_config.scd4x_config.set_target_co2_conc = target_co2_conc | |
| send_command = True | |
| if 'set_temperature' in cleanup_commands: | |
| try: | |
| temperature = float(cleanup_commands[cleanup_commands.index('set_temperature')+1]) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference temperature' | |
| ) | |
| return | |
| else: | |
| print (f"Setting SCD4X Reference temperature to {temperature}") | |
| p.sensor_config.scd4x_config.set_temperature = temperature | |
| send_command = True | |
| if 'set_altitude' in cleanup_commands: | |
| try: | |
| altitude = int(cleanup_commands[cleanup_commands.index('set_altitude')+1]) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference altitude' | |
| ) | |
| return | |
| else: | |
| print (f"Setting SCD4X Reference altitude to {altitude}") | |
| p.sensor_config.scd4x_config.set_altitude = altitude | |
| if 'set_ambient_pressure' in cleanup_commands: | |
| try: | |
| ambient_pressure = int(cleanup_commands[cleanup_commands.index('set_ambient_pressure')+1]) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference ambient pressure' | |
| ) | |
| return | |
| else: | |
| print (f"Setting SCD4X Reference ambient pressure to {ambient_pressure}") | |
| def _get_scd4x_option_value(option_name: str) -> Optional[str]: | |
| """ | |
| Safely get the value token that follows an SCD4X option name | |
| in cleanup_commands. Returns None if no valid value is found. | |
| """ | |
| try: | |
| idx = cleanup_commands.index(option_name) | |
| except ValueError: | |
| return None | |
| value_index = idx + 1 | |
| if value_index >= len(cleanup_commands): | |
| print(f"Missing value for sensor_config.scd4x_config.{option_name}") | |
| return None | |
| value_token = cleanup_commands[value_index] | |
| # Treat any other known SCD4X option name as not being a valid value | |
| scd4x_option_names = { | |
| 'factory_reset', | |
| 'set_asc', | |
| 'set_target_co2_conc', | |
| 'set_temperature', | |
| 'set_altitude', | |
| 'set_ambient_pressure', | |
| } | |
| if value_token in scd4x_option_names: | |
| print(f"Missing value for sensor_config.scd4x_config.{option_name}") | |
| return None | |
| return value_token | |
| if 'factory_reset' in cleanup_commands: | |
| print("Performing factory reset on SCD4X") | |
| p.sensor_config.scd4x_config.factory_reset = True | |
| else: | |
| if 'set_asc' in cleanup_commands: | |
| asc_value = _get_scd4x_option_value('set_asc') | |
| if asc_value == "true": | |
| p.sensor_config.scd4x_config.set_asc = True | |
| print("Setting SCD4X ASC mode") | |
| elif asc_value == "false": | |
| p.sensor_config.scd4x_config.set_asc = False | |
| print("Setting SCD4X FRC mode") | |
| elif asc_value is not None: | |
| print( | |
| f'Not valid argument for sensor_config.scd4x_config.set_asc' | |
| ) | |
| if 'set_target_co2_conc' in cleanup_commands: | |
| value = _get_scd4x_option_value('set_target_co2_conc') | |
| if value is None: | |
| return | |
| try: | |
| target_co2_conc = int(value) | |
| except ValueError: | |
| print( | |
| f'Invalid value for target CO2 conc' | |
| ) | |
| return | |
| else: | |
| print(f"Setting SCD4X target CO2 conc to {target_co2_conc}") | |
| p.sensor_config.scd4x_config.set_target_co2_conc = target_co2_conc | |
| send_command = True | |
| if 'set_temperature' in cleanup_commands: | |
| value = _get_scd4x_option_value('set_temperature') | |
| if value is None: | |
| return | |
| try: | |
| temperature = float(value) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference temperature' | |
| ) | |
| return | |
| else: | |
| print(f"Setting SCD4X Reference temperature to {temperature}") | |
| p.sensor_config.scd4x_config.set_temperature = temperature | |
| send_command = True | |
| if 'set_altitude' in cleanup_commands: | |
| value = _get_scd4x_option_value('set_altitude') | |
| if value is None: | |
| return | |
| try: | |
| altitude = int(value) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference altitude' | |
| ) | |
| return | |
| else: | |
| print(f"Setting SCD4X Reference altitude to {altitude}") | |
| p.sensor_config.scd4x_config.set_altitude = altitude | |
| if 'set_ambient_pressure' in cleanup_commands: | |
| value = _get_scd4x_option_value('set_ambient_pressure') | |
| if value is None: | |
| return | |
| try: | |
| ambient_pressure = int(value) | |
| except ValueError: | |
| print( | |
| f'Invalid value for reference ambient pressure' | |
| ) | |
| return | |
| else: | |
| print(f"Setting SCD4X Reference ambient pressure to {ambient_pressure}") |
| print (f"Setting SCD4X target CO2 conc to {target_co2_conc}") | ||
| p.sensor_config.scd4x_config.set_target_co2_conc = target_co2_conc | ||
| send_command = True | ||
| if 'set_temperature' in cleanup_commands: | ||
| try: | ||
| temperature = float(cleanup_commands[cleanup_commands.index('set_temperature')+1]) | ||
| except ValueError: | ||
| print( | ||
| f'Invalid value for reference temperature' | ||
| ) | ||
| return | ||
| else: | ||
| print (f"Setting SCD4X Reference temperature to {temperature}") | ||
| p.sensor_config.scd4x_config.set_temperature = temperature | ||
| send_command = True |
There was a problem hiding this comment.
send_command is assigned but never read. This is dead code and may trigger linting failures; either remove it or use it to control the “Nothing to request”/sending behavior consistently across all sensor config fields.
| if 'factory_reset' in cleanup_commands: | ||
| print ("Performing factory reset on SCD4X") | ||
| p.sensor_config.scd4x_config.factory_reset = True | ||
| else: | ||
| if 'set_asc' in cleanup_commands: | ||
| if cleanup_commands[cleanup_commands.index('set_asc')+1] == "true": | ||
| p.sensor_config.scd4x_config.set_asc = True | ||
| print ("Setting SCD4X ASC mode") | ||
| elif cleanup_commands[cleanup_commands.index('set_asc')+1] == "false": | ||
| p.sensor_config.scd4x_config.set_asc = False | ||
| print ("Setting SCD4X FRC mode") | ||
| else: | ||
| print( | ||
| f'Not valid argument for sensor_config.scd4x_config.set_asc' | ||
| ) |
There was a problem hiding this comment.
This method prints user-facing status/errors directly. In node.py, other admin actions typically use logger.info()/warning() (e.g. reboot() / shutdown()), which is easier to test and integrates with library consumers. Consider switching these print() calls to logging (and letting the CLI decide what to print) to keep Node usable as a library API.
This is a pull request to add sensor configuration commands for the new sensors implemented in the Air Quality module and which available configs are in the
admin.protoalready: https://github.com/meshtastic/protobufs/blob/master/meshtastic/admin.proto#L644-L654Support / guidance needed parts
I have the feeling that I am doing something wrong while parsing the options, as I assume there must be some sort of better option than manually typing all of them... (see https://github.com/fablabbcn/meshtastic-python/blob/b54ac0e0f0aaee60c5a54069e47bbb6c7818447a/meshtastic/node.py#L980-L1021)
Reaching local node works, but remote nodes doesn't:
An example of what I am doing to test this is:
Which successfully reaches the node and gets processed via
admincommand when the target node is directly on that IP address. That node also has MQTT enabled, which also gives me some info:However, when targeting another node on that mesh:
I don't manage to make these admin commands to reach the
dest, getting the following theMQTTfeedback: