Fixing issue #845: inconsistent behavior in chained --set when one of them fails

meshtastic/__main__.py

@@ -193,8 +193,12 @@ def traverseConfig(config_root, config, interface_config) -> bool:
     return True
 
 
-def setPref(config, comp_name, raw_val) -> bool:
-    """Set a channel or preferences value"""
+def setPref(config, comp_name: str, raw_val) -> bool:
+    """Set a channel or preferences value
+    config: LocalConfig, ModuleConfig structures from protobuf
+    comp_name: dotted name of configuration entry
+    raw_val: value to set
+    """
 
     name = splitCompoundName(comp_name)
 
@@ -209,7 +213,7 @@ def setPref(config, comp_name, raw_val) -> bool:
     config_type = objDesc.fields_by_name.get(name[0])
     if config_type and config_type.message_type is not None:
         for name_part in name[1:-1]:
-            part_snake_name = meshtastic.util.camel_to_snake((name_part))
+            part_snake_name = meshtastic.util.camel_to_snake(name_part)
             config_part = getattr(config, config_type.name)
             config_type = config_type.message_type.fields_by_name.get(part_snake_name)
     pref = None
@@ -234,7 +238,7 @@ def setPref(config, comp_name, raw_val) -> bool:
 
     enumType = pref.enum_type
     # pylint: disable=C0123
-    if enumType and type(val) == str:
+    if enumType and isinstance(val, str):
         # We've failed so far to convert this string into an enum, try to find it by reflection
         e = enumType.values_by_name.get(val)
         if e:
@@ -628,12 +632,11 @@ def onConnected(interface):
             closeNow = True
             waitForAckNak = True
             node = interface.getNode(args.dest, False, **getNode_kwargs)
-
             # Handle the int/float/bool arguments
-            pref = None
-            fields = set()
+            fields = []
+            allSettingOK = True
             for pref in args.set:
-                found = False
+                actSettingOK = False
                 field = splitCompoundName(pref[0].lower())[0]
                 for config in [node.localConfig, node.moduleConfig]:
                     config_type = config.DESCRIPTOR.fields_by_name.get(field)
@@ -642,30 +645,29 @@ def onConnected(interface):
                             node.requestConfig(
                                 config.DESCRIPTOR.fields_by_name.get(field)
                             )
-                        found = setPref(config, pref[0], pref[1])
-                        if found:
-                            fields.add(field)
+                        if actSettingOK := setPref(config, pref[0], pref[1]):
                             break
+                fields.append((field, actSettingOK, pref[0]),)
+                allSettingOK = allSettingOK and actSettingOK
 
-            if found:
+            # only write to radio when all settings can be processed. If one setting is wrong, drop everything.
+            # This shall ensure consistency of settings in the radio
+            if allSettingOK:
+                fieldsToWrite = set([field[0] for field in fields])     # keep only one occurence of a field
                 print("Writing modified preferences to device")
-                if len(fields) > 1:
+                if len(fieldsToWrite) > 1:
                     print("Using a configuration transaction")
                     node.beginSettingsTransaction()
-                for field in fields:
+                for field in fieldsToWrite:
                     print(f"Writing {field} configuration to device")
                     node.writeConfig(field)
-                if len(fields) > 1:
+                if len(fieldsToWrite) > 1:
                     node.commitSettingsTransaction()
             else:
-                if mt_config.camel_case:
-                    print(
-                        f"{node.localConfig.__class__.__name__} and {node.moduleConfig.__class__.__name__} do not have an attribute {pref[0]}."
-                    )
-                else:
-                    print(
-                        f"{node.localConfig.__class__.__name__} and {node.moduleConfig.__class__.__name__} do not have attribute {pref[0]}."
-                    )
+                print("Cannot process command due to errors in parameters:")
+                for field, flag, settingName in fields:
+                    if not flag:
+                        print(f"{node.localConfig.__class__.__name__} and {node.moduleConfig.__class__.__name__} do not have an attribute {settingName} in category {field}.")
                 print("Choices are...")
                 printConfig(node.localConfig)
                 printConfig(node.moduleConfig)
@@ -927,11 +929,11 @@ def setSimpleConfig(modem_preset):
             # Handle the channel settings
             for pref in args.ch_set or []:
                 if pref[0] == "psk":
-                    found = True
+                    actSettingOK = True
                     ch.settings.psk = meshtastic.util.fromPSK(pref[1])
                 else:
-                    found = setPref(ch.settings, pref[0], pref[1])
-                if not found:
+                    actSettingOK = setPref(ch.settings, pref[0], pref[1])
+                if not actSettingOK:
                     category_settings = ["module_settings"]
                     print(
                         f"{ch.settings.__class__.__name__} does not have an attribute {pref[0]}."
@@ -1003,9 +1005,9 @@ def setSimpleConfig(modem_preset):
             closeNow = True
             node = interface.getNode(args.dest, False, **getNode_kwargs)
             for pref in args.get:
-                found = getPref(node, pref[0])
+                actSettingOK = getPref(node, pref[0])
 
-            if found:
+            if actSettingOK:
                 print("Completed getting preferences")
 
         if args.nodes:

meshtastic/mesh_interface.py

@@ -106,7 +106,7 @@ def __init__(
         self.isConnected: threading.Event = threading.Event()
         self.noProto: bool = noProto
         self.localNode: meshtastic.node.Node = meshtastic.node.Node(
-            self, -1, timeout=timeout
+            self, -1, timeout=timeout, noProto=self.noProto
         )  # We fixup nodenum later
         self.myInfo: Optional[
             mesh_pb2.MyNodeInfo

meshtastic/stream_interface.py

@@ -208,7 +208,7 @@ def __reader(self) -> None:
                             self._rxBuf = empty
                 else:
                     # logger.debug(f"timeout")
-                    pass
+                    time.sleep(0.001)       # don't block the system in case we do not get data
         except serial.SerialException as ex:
             if (
                 not self._wantExit

meshtastic/tests/test_main.py

@@ -23,6 +23,7 @@
 from meshtastic import mt_config
 
 from ..protobuf.channel_pb2 import Channel # pylint: disable=E0611
+from ..protobuf import localonly_pb2, config_pb2, module_config_pb2
 
 # from ..ble_interface import BLEInterface
 from ..node import Node
@@ -503,6 +504,56 @@ def test_main_set_canned_messages(capsys):
         mo.assert_called()
 
 
+@pytest.mark.unit
+@pytest.mark.usefixtures("reset_mt_config")
+@patch("meshtastic.serial_interface.SerialInterface._set_hupcl_with_termios")
+@patch("builtins.open", new_callable=mock_open, read_data="data")
+@patch("serial.Serial")
+@patch("meshtastic.util.findPorts", return_value=["/dev/ttyUSBfake"])
+def test_main_set_3_parameters_OK(mocked_findPorts, mocked_serial, mocked_open, mock_hupcl, capsys):
+    """Test --set with 3 parameters with success"""
+    sys.argv = ["", "--set", "mqtt.enabled", "1", "--set", "mqtt.username", "abc", "--set", "position.gps_enabled", "false"]
+    mt_config.args = sys.argv
+
+    iface = SerialInterface(noProto=True)
+    iface.localNode.localConfig.position.CopyFrom(config_pb2.Config.PositionConfig())
+    iface.localNode.moduleConfig.mqtt.CopyFrom(module_config_pb2.ModuleConfig.MQTTConfig())
+
+    with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo:
+        main()
+        out, err = capsys.readouterr()
+        assert re.search(r"Connected to radio", out, re.MULTILINE)
+        assert re.search(r"Writing mqtt configuration to device", out, re.MULTILINE)
+        assert re.search(r"Writing position configuration to device", out, re.MULTILINE)
+        assert re.search(r"Set position.gps_enabled to false", out, re.MULTILINE)
+        assert err == ""
+        mo.assert_called()
+
+
+@pytest.mark.unit
+@pytest.mark.usefixtures("reset_mt_config")
+@patch("meshtastic.serial_interface.SerialInterface._set_hupcl_with_termios")
+@patch("builtins.open", new_callable=mock_open, read_data="data")
+@patch("serial.Serial")
+@patch("meshtastic.util.findPorts", return_value=["/dev/ttyUSBfake"])
+def test_main_set_3_parameters_NOK(mocked_findPorts, mocked_serial, mocked_open, mock_hupcl, capsys):
+    """Test --set with 3 parameters where 1 parameter is faulty"""
+    sys.argv = ["", "--set", "mqtt.enabled", "1", "--set", "mqtt", "1", "--set", "mqtt.username", "abc"]
+    mt_config.args = sys.argv
+
+    iface = SerialInterface(noProto=True)
+    iface.localNode.localConfig.position.CopyFrom(config_pb2.Config.PositionConfig())
+    iface.localNode.moduleConfig.mqtt.CopyFrom(module_config_pb2.ModuleConfig.MQTTConfig())
+    with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo:
+        main()
+        out, err = capsys.readouterr()
+        assert re.search(r"Connected to radio", out, re.MULTILINE)
+        assert re.search(r"Cannot process command due to errors in parameters", out, re.MULTILINE)
+        assert re.search(r"LocalConfig and LocalModuleConfig do not have an attribute mqtt in category mqtt", out, re.MULTILINE)
+        assert err == ""
+        mo.assert_called()
+
+
 @pytest.mark.unit
 @pytest.mark.usefixtures("reset_mt_config")
 def test_main_get_canned_messages(capsys, caplog, iface_with_nodes):
@@ -1065,14 +1116,14 @@ def test_main_set_with_invalid(mocked_findports, mocked_serial, mocked_open, moc
     mt_config.args = sys.argv
 
     serialInterface = SerialInterface(noProto=True)
-    anode = Node(serialInterface, 1234567890, noProto=True)
-    serialInterface.localNode = anode
+    serialInterface.localNode.nodeNum = 1234567890
 
     with patch("meshtastic.serial_interface.SerialInterface", return_value=serialInterface) as mo:
         main()
         out, err = capsys.readouterr()
         assert re.search(r"Connected to radio", out, re.MULTILINE)
-        assert re.search(r"do not have attribute foo", out, re.MULTILINE)
+        assert re.search(r"Cannot process command due to errors in parameters", out, re.MULTILINE)
+        assert re.search(r"LocalConfig and LocalModuleConfig do not have an attribute foo in category foo", out, re.MULTILINE)
         assert err == ""
         mo.assert_called()