From 9612aea9b9518abc2e387157cbb548fdf8caee68 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Tue, 3 Sep 2024 21:47:07 -0500 Subject: [PATCH 01/14] Add in a retry mechanism for channel settings Attempts multiple times to fetch things over the admin channel before giving up. --- meshtastic/__main__.py | 6 ++++++ meshtastic/mesh_interface.py | 20 +++++++++++++++++--- meshtastic/node.py | 11 ++++++----- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index f78155d8..ea9f6dda 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -1423,6 +1423,12 @@ def initParser(): action="append", ) + group.add_argument( + "--channel-fetch-retries", + help=("Attempt to retrieve channel settings for --ch-set this many times before giving up."), + default=3, + ) + group.add_argument( "--ch-vlongslow", help="Change to the very long-range and slow channel", diff --git a/meshtastic/mesh_interface.py b/meshtastic/mesh_interface.py index c59fcb3e..b2896685 100644 --- a/meshtastic/mesh_interface.py +++ b/meshtastic/mesh_interface.py @@ -314,7 +314,7 @@ def getTimeAgo(ts) -> Optional[str]: return table def getNode( - self, nodeId: str, requestChannels: bool = True + self, nodeId: str, requestChannels: bool = True, requestChannelRetries: int = 3 ) -> meshtastic.node.Node: """Return a node object which contains device settings and channel info""" if nodeId in (LOCAL_ADDR, BROADCAST_ADDR): @@ -325,8 +325,22 @@ def getNode( if requestChannels: logging.debug("About to requestChannels") n.requestChannels() - if not n.waitForConfig(): - our_exit("Error: Timed out waiting for channels") + retries_left = requestChannelRetries + last_index: int = 0 + while retries_left > 0: + retries_left -= 1 + if not n.waitForConfig(): + new_index: int = len(n.partialChannels) + # each time we get a new channel, reset the counter + if new_index != last_index: + retries_left = requestChannelRetries - 1 + if retries_left <= 0: + our_exit(f"Error: Timed out waiting for channels, giving up") + print("Timed out trying to retrieve channel info, retrying") + n.requestChannels(startingIndex=len(n.partialChannels)) + last_index = new_index + else: + break return n def sendText( diff --git a/meshtastic/node.py b/meshtastic/node.py index 48b9508e..91c2d411 100644 --- a/meshtastic/node.py +++ b/meshtastic/node.py @@ -77,13 +77,14 @@ def setChannels(self, channels): self.channels = channels self._fixupChannels() - def requestChannels(self): + def requestChannels(self, startingIndex: int = 0): """Send regular MeshPackets to ask channels.""" logging.debug(f"requestChannels for nodeNum:{self.nodeNum}") - self.channels = None - self.partialChannels = [] # We keep our channels in a temp array until finished - - self._requestChannel(0) + # only initialize if we're starting out fresh + if startingIndex == 0: + self.channels = None + self.partialChannels = [] # We keep our channels in a temp array until finished + self._requestChannel(startingIndex) def onResponseRequestSettings(self, p): """Handle the response packets for requesting settings _requestSettings()""" From b6547c97374368ca1837f4fa14a7b94403c60e29 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Tue, 3 Sep 2024 22:04:08 -0500 Subject: [PATCH 02/14] actually link up the retry args from the commandline to getNode --- meshtastic/__main__.py | 93 ++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index ea9f6dda..b43d5954 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -273,6 +273,11 @@ def onConnected(interface): try: args = mt_config.args + # convenient place to store any keyword args we pass to getNode + getNode_kwargs = { + "requestChannelRetries": int(args.channel_fetch_retries) + } + # do not print this line if we are exporting the config if not args.export_config: print("Connected to radio") @@ -334,13 +339,13 @@ def onConnected(interface): closeNow = True waitForAckNak = True print(f"Setting device owner to {args.set_owner}") - interface.getNode(args.dest, False).setOwner(args.set_owner) + interface.getNode(args.dest, False, **getNode_kwargs).setOwner(args.set_owner) if args.set_owner_short: closeNow = True waitForAckNak = True print(f"Setting device owner short to {args.set_owner_short}") - interface.getNode(args.dest, False).setOwner( + interface.getNode(args.dest, False, **getNode_kwargs).setOwner( long_name=None, short_name=args.set_owner_short ) @@ -349,7 +354,7 @@ def onConnected(interface): closeNow = True waitForAckNak = True print(f"Setting canned plugin message to {args.set_canned_message}") - interface.getNode(args.dest, False).set_canned_message( + interface.getNode(args.dest, False, **getNode_kwargs).set_canned_message( args.set_canned_message ) @@ -358,12 +363,12 @@ def onConnected(interface): closeNow = True waitForAckNak = True print(f"Setting ringtone to {args.set_ringtone}") - interface.getNode(args.dest, False).set_ringtone(args.set_ringtone) + interface.getNode(args.dest, False, **getNode_kwargs).set_ringtone(args.set_ringtone) if args.pos_fields: # If --pos-fields invoked with args, set position fields closeNow = True - positionConfig = interface.getNode(args.dest).localConfig.position + positionConfig = interface.getNode(args.dest, **getNode_kwargs).localConfig.position allFields = 0 try: @@ -382,12 +387,12 @@ def onConnected(interface): print(f"Setting position fields to {allFields}") setPref(positionConfig, "position_flags", f"{allFields:d}") print("Writing modified preferences to device") - interface.getNode(args.dest).writeConfig("position") + interface.getNode(args.dest, **getNode_kwargs).writeConfig("position") elif args.pos_fields is not None: # If --pos-fields invoked without args, read and display current value closeNow = True - positionConfig = interface.getNode(args.dest).localConfig.position + positionConfig = interface.getNode(args.dest, **getNode_kwargs).localConfig.position fieldNames = [] for bit in positionConfig.PositionFlags.values(): @@ -398,56 +403,56 @@ def onConnected(interface): if args.set_ham: closeNow = True print(f"Setting Ham ID to {args.set_ham} and turning off encryption") - interface.getNode(args.dest).setOwner(args.set_ham, is_licensed=True) + interface.getNode(args.dest, **getNode_kwargs).setOwner(args.set_ham, is_licensed=True) # Must turn off encryption on primary channel - interface.getNode(args.dest).turnOffEncryptionOnPrimaryChannel() + interface.getNode(args.dest, **getNode_kwargs).turnOffEncryptionOnPrimaryChannel() if args.reboot: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).reboot() + interface.getNode(args.dest, False, **getNode_kwargs).reboot() if args.reboot_ota: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).rebootOTA() + interface.getNode(args.dest, False, **getNode_kwargs).rebootOTA() if args.enter_dfu: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).enterDFUMode() + interface.getNode(args.dest, False, **getNode_kwargs).enterDFUMode() if args.shutdown: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).shutdown() + interface.getNode(args.dest, False, **getNode_kwargs).shutdown() if args.device_metadata: closeNow = True - interface.getNode(args.dest, False).getMetadata() + interface.getNode(args.dest, False, **getNode_kwargs).getMetadata() if args.begin_edit: closeNow = True - interface.getNode(args.dest, False).beginSettingsTransaction() + interface.getNode(args.dest, False, **getNode_kwargs).beginSettingsTransaction() if args.commit_edit: closeNow = True - interface.getNode(args.dest, False).commitSettingsTransaction() + interface.getNode(args.dest, False, **getNode_kwargs).commitSettingsTransaction() if args.factory_reset: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).factoryReset() + interface.getNode(args.dest, False, **getNode_kwargs).factoryReset() if args.remove_node: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).removeNode(args.remove_node) + interface.getNode(args.dest, False, **getNode_kwargs).removeNode(args.remove_node) if args.reset_nodedb: closeNow = True waitForAckNak = True - interface.getNode(args.dest, False).resetNodeDb() + interface.getNode(args.dest, False, **getNode_kwargs).resetNodeDb() if args.sendtext: closeNow = True @@ -461,7 +466,7 @@ def onConnected(interface): args.dest, wantAck=True, channelIndex=channelIndex, - onResponse=interface.getNode(args.dest, False).onAckNak, + onResponse=interface.getNode(args.dest, False, **getNode_kwargs).onAckNak, ) else: meshtastic.util.our_exit( @@ -552,7 +557,7 @@ def onConnected(interface): if args.set: closeNow = True waitForAckNak = True - node = interface.getNode(args.dest, False) + node = interface.getNode(args.dest, False, **getNode_kwargs) # Handle the int/float/bool arguments pref = None @@ -591,19 +596,19 @@ def onConnected(interface): configuration = yaml.safe_load(file) closeNow = True - interface.getNode(args.dest, False).beginSettingsTransaction() + interface.getNode(args.dest, False, **getNode_kwargs).beginSettingsTransaction() if "owner" in configuration: print(f"Setting device owner to {configuration['owner']}") waitForAckNak = True - interface.getNode(args.dest, False).setOwner(configuration["owner"]) + interface.getNode(args.dest, False, **getNode_kwargs).setOwner(configuration["owner"]) if "owner_short" in configuration: print( f"Setting device owner short to {configuration['owner_short']}" ) waitForAckNak = True - interface.getNode(args.dest, False).setOwner( + interface.getNode(args.dest, False, **getNode_kwargs).setOwner( long_name=None, short_name=configuration["owner_short"] ) @@ -612,17 +617,17 @@ def onConnected(interface): f"Setting device owner short to {configuration['ownerShort']}" ) waitForAckNak = True - interface.getNode(args.dest, False).setOwner( + interface.getNode(args.dest, False, **getNode_kwargs).setOwner( long_name=None, short_name=configuration["ownerShort"] ) if "channel_url" in configuration: print("Setting channel url to", configuration["channel_url"]) - interface.getNode(args.dest).setURL(configuration["channel_url"]) + interface.getNode(args.dest, **getNode_kwargs).setURL(configuration["channel_url"]) if "channelUrl" in configuration: print("Setting channel url to", configuration["channelUrl"]) - interface.getNode(args.dest).setURL(configuration["channelUrl"]) + interface.getNode(args.dest, **getNode_kwargs).setURL(configuration["channelUrl"]) if "location" in configuration: alt = 0 @@ -647,28 +652,28 @@ def onConnected(interface): interface.localNode.writeConfig("position") if "config" in configuration: - localConfig = interface.getNode(args.dest).localConfig + localConfig = interface.getNode(args.dest, **getNode_kwargs).localConfig for section in configuration["config"]: traverseConfig( section, configuration["config"][section], localConfig ) - interface.getNode(args.dest).writeConfig( + interface.getNode(args.dest, **getNode_kwargs).writeConfig( meshtastic.util.camel_to_snake(section) ) if "module_config" in configuration: - moduleConfig = interface.getNode(args.dest).moduleConfig + moduleConfig = interface.getNode(args.dest, **getNode_kwargs).moduleConfig for section in configuration["module_config"]: traverseConfig( section, configuration["module_config"][section], moduleConfig, ) - interface.getNode(args.dest).writeConfig( + interface.getNode(args.dest, **getNode_kwargs).writeConfig( meshtastic.util.camel_to_snake(section) ) - interface.getNode(args.dest, False).commitSettingsTransaction() + interface.getNode(args.dest, False, **getNode_kwargs).commitSettingsTransaction() print("Writing modified configuration to device") if args.export_config: @@ -681,7 +686,7 @@ def onConnected(interface): if args.seturl: closeNow = True - interface.getNode(args.dest).setURL(args.seturl) + interface.getNode(args.dest, **getNode_kwargs).setURL(args.seturl) # handle changing channels @@ -697,7 +702,7 @@ def onConnected(interface): meshtastic.util.our_exit( "Warning: Channel name must be shorter. Channel not added." ) - n = interface.getNode(args.dest) + n = interface.getNode(args.dest, **getNode_kwargs) ch = n.getChannelByName(args.ch_add) if ch: meshtastic.util.our_exit( @@ -736,7 +741,7 @@ def onConnected(interface): ) else: print(f"Deleting channel {channelIndex}") - ch = interface.getNode(args.dest).deleteChannel(channelIndex) + ch = interface.getNode(args.dest, **getNode_kwargs).deleteChannel(channelIndex) def setSimpleConfig(modem_preset): """Set one of the simple modem_config""" @@ -746,9 +751,9 @@ def setSimpleConfig(modem_preset): "Warning: Cannot set modem preset for non-primary channel", 1 ) # Overwrite modem_preset - prefs = interface.getNode(args.dest).localConfig + prefs = interface.getNode(args.dest, **getNode_kwargs).localConfig prefs.lora.modem_preset = modem_preset - interface.getNode(args.dest).writeConfig("lora") + interface.getNode(args.dest, **getNode_kwargs).writeConfig("lora") # handle the simple radio set commands if args.ch_vlongslow: @@ -778,7 +783,7 @@ def setSimpleConfig(modem_preset): channelIndex = mt_config.channel_index if channelIndex is None: meshtastic.util.our_exit("Warning: Need to specify '--ch-index'.", 1) - node = interface.getNode(args.dest) + node = interface.getNode(args.dest, **getNode_kwargs) ch = node.channels[channelIndex] if args.ch_enable or args.ch_disable: @@ -842,12 +847,12 @@ def setSimpleConfig(modem_preset): if args.get_canned_message: closeNow = True print("") - interface.getNode(args.dest).get_canned_message() + interface.getNode(args.dest, **getNode_kwargs).get_canned_message() if args.get_ringtone: closeNow = True print("") - interface.getNode(args.dest).get_ringtone() + interface.getNode(args.dest, **getNode_kwargs).get_ringtone() if args.info: print("") @@ -855,7 +860,7 @@ def setSimpleConfig(modem_preset): if args.dest == BROADCAST_ADDR: interface.showInfo() print("") - interface.getNode(args.dest).showInfo() + interface.getNode(args.dest, **getNode_kwargs).showInfo() closeNow = True print("") pypi_version = meshtastic.util.check_if_newer_version() @@ -872,7 +877,7 @@ def setSimpleConfig(modem_preset): if args.get: closeNow = True - node = interface.getNode(args.dest, False) + node = interface.getNode(args.dest, False, **getNode_kwargs) for pref in args.get: found = getPref(node, pref[0]) @@ -888,7 +893,7 @@ def setSimpleConfig(modem_preset): if args.qr or args.qr_all: closeNow = True - url = interface.getNode(args.dest, True).getURL(includeAll=args.qr_all) + url = interface.getNode(args.dest, True, **getNode_kwargs).getURL(includeAll=args.qr_all) if args.qr_all: urldesc = "Complete URL (includes all channels)" else: @@ -943,7 +948,7 @@ def setSimpleConfig(modem_preset): print( f"Waiting for an acknowledgment from remote node (this could take a while)" ) - interface.getNode(args.dest, False).iface.waitForAckNak() + interface.getNode(args.dest, False, **getNode_kwargs).iface.waitForAckNak() if args.wait_to_disconnect: print(f"Waiting {args.wait_to_disconnect} seconds before disconnecting") From 3811226a61815fdb3576d64d9c1ae7433e0151c5 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Tue, 3 Sep 2024 22:12:03 -0500 Subject: [PATCH 03/14] add a configurable timeout --- meshtastic/__main__.py | 9 ++++++++- meshtastic/mesh_interface.py | 4 ++-- meshtastic/node.py | 4 ++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index b43d5954..fcfb3852 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -275,7 +275,8 @@ def onConnected(interface): # convenient place to store any keyword args we pass to getNode getNode_kwargs = { - "requestChannelRetries": int(args.channel_fetch_retries) + "requestChannelRetries": int(args.channel_fetch_retries), + "timeout": int(args.timeout) } # do not print this line if we are exporting the config @@ -1434,6 +1435,12 @@ def initParser(): default=3, ) + group.add_argument( + "--timeout", + help="How long to wait for replies", + default=300 + ) + group.add_argument( "--ch-vlongslow", help="Change to the very long-range and slow channel", diff --git a/meshtastic/mesh_interface.py b/meshtastic/mesh_interface.py index b2896685..48fff564 100644 --- a/meshtastic/mesh_interface.py +++ b/meshtastic/mesh_interface.py @@ -314,13 +314,13 @@ def getTimeAgo(ts) -> Optional[str]: return table def getNode( - self, nodeId: str, requestChannels: bool = True, requestChannelRetries: int = 3 + self, nodeId: str, requestChannels: bool = True, requestChannelRetries: int = 3, timeout: int = 300 ) -> meshtastic.node.Node: """Return a node object which contains device settings and channel info""" if nodeId in (LOCAL_ADDR, BROADCAST_ADDR): return self.localNode else: - n = meshtastic.node.Node(self, nodeId) + n = meshtastic.node.Node(self, nodeId, timeout=timeout) # Only request device settings and channel info when necessary if requestChannels: logging.debug("About to requestChannels") diff --git a/meshtastic/node.py b/meshtastic/node.py index 91c2d411..964e6807 100644 --- a/meshtastic/node.py +++ b/meshtastic/node.py @@ -25,14 +25,14 @@ class Node: Includes methods for localConfig, moduleConfig and channels """ - def __init__(self, iface, nodeNum, noProto=False): + def __init__(self, iface, nodeNum, noProto=False, timeout: int = 300): """Constructor""" self.iface = iface self.nodeNum = nodeNum self.localConfig = localonly_pb2.LocalConfig() self.moduleConfig = localonly_pb2.LocalModuleConfig() self.channels = None - self._timeout = Timeout(maxSecs=300) + self._timeout = Timeout(maxSecs=timeout) self.partialChannels = None self.noProto = noProto self.cannedPluginMessage = None From abe1dd47caf3dc33485192ba01b72a7b89131d4a Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 11:36:23 -0500 Subject: [PATCH 04/14] add type to argument --- meshtastic/__main__.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index fcfb3852..a484eb5a 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -275,8 +275,8 @@ def onConnected(interface): # convenient place to store any keyword args we pass to getNode getNode_kwargs = { - "requestChannelRetries": int(args.channel_fetch_retries), - "timeout": int(args.timeout) + "requestChannelRetries": args.channel_fetch_retries, + "timeout": args.timeout } # do not print this line if we are exporting the config @@ -1433,12 +1433,14 @@ def initParser(): "--channel-fetch-retries", help=("Attempt to retrieve channel settings for --ch-set this many times before giving up."), default=3, + type=int, ) group.add_argument( "--timeout", help="How long to wait for replies", - default=300 + default=300, + type=int, ) group.add_argument( From 44cfd72a80e7991b7a62c77fb244455fbf44192d Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 11:39:10 -0500 Subject: [PATCH 05/14] make sure new_index is always an int --- meshtastic/mesh_interface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/meshtastic/mesh_interface.py b/meshtastic/mesh_interface.py index 48fff564..8c9679d5 100644 --- a/meshtastic/mesh_interface.py +++ b/meshtastic/mesh_interface.py @@ -330,14 +330,14 @@ def getNode( while retries_left > 0: retries_left -= 1 if not n.waitForConfig(): - new_index: int = len(n.partialChannels) + new_index: int = len(n.partialChannels) if n.partialChannels else 0 # each time we get a new channel, reset the counter if new_index != last_index: retries_left = requestChannelRetries - 1 if retries_left <= 0: our_exit(f"Error: Timed out waiting for channels, giving up") print("Timed out trying to retrieve channel info, retrying") - n.requestChannels(startingIndex=len(n.partialChannels)) + n.requestChannels(startingIndex=new_index) last_index = new_index else: break From 1967519deb5f1bad03a2cad6fb8a30cfc4e6fc72 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 11:52:27 -0500 Subject: [PATCH 06/14] correct type issue during initial assignment --- meshtastic/node.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/meshtastic/node.py b/meshtastic/node.py index 4b4303f8..278ea8aa 100644 --- a/meshtastic/node.py +++ b/meshtastic/node.py @@ -5,7 +5,7 @@ import logging import time -from typing import Optional, Union +from typing import Optional, Union, List from meshtastic.protobuf import admin_pb2, apponly_pb2, channel_pb2, localonly_pb2, mesh_pb2, portnums_pb2 from meshtastic.util import ( @@ -33,7 +33,7 @@ def __init__(self, iface, nodeNum, noProto=False, timeout: int = 300): self.moduleConfig = localonly_pb2.LocalModuleConfig() self.channels = None self._timeout = Timeout(maxSecs=timeout) - self.partialChannels = None + self.partialChannels: Optional[List] = None self.noProto = noProto self.cannedPluginMessage = None self.cannedPluginMessageMessages = None From aa74db46cb46bb817118cd05da19697f92a1cf8f Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 11:59:03 -0500 Subject: [PATCH 07/14] update message check in test to reflect new message --- meshtastic/tests/test_mesh_interface.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meshtastic/tests/test_mesh_interface.py b/meshtastic/tests/test_mesh_interface.py index 21f80c68..cace7ebd 100644 --- a/meshtastic/tests/test_mesh_interface.py +++ b/meshtastic/tests/test_mesh_interface.py @@ -173,7 +173,7 @@ def test_getNode_not_local_timeout(capsys): assert pytest_wrapped_e.type == SystemExit assert pytest_wrapped_e.value.code == 1 out, err = capsys.readouterr() - assert re.match(r"Error: Timed out waiting for channels", out) + assert re.match(r"Timed out trying to retrieve channel info, retrying", out) assert err == "" From 9e7d5e96abe07ca46542225a22dcd2b301263a4c Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 12:03:51 -0500 Subject: [PATCH 08/14] Rename "retries" to "attempts" Otherwise, semantically, it's off-by-one. --- meshtastic/__main__.py | 4 ++-- meshtastic/mesh_interface.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index 6ae38216..68c785ca 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -275,7 +275,7 @@ def onConnected(interface): # convenient place to store any keyword args we pass to getNode getNode_kwargs = { - "requestChannelRetries": args.channel_fetch_retries, + "requestChannelRetries": args.channel_fetch_attempts, "timeout": args.timeout } @@ -1416,7 +1416,7 @@ def initParser(): ) group.add_argument( - "--channel-fetch-retries", + "--channel-fetch-attempts", help=("Attempt to retrieve channel settings for --ch-set this many times before giving up."), default=3, type=int, diff --git a/meshtastic/mesh_interface.py b/meshtastic/mesh_interface.py index 86e4e804..872c7f0b 100644 --- a/meshtastic/mesh_interface.py +++ b/meshtastic/mesh_interface.py @@ -315,7 +315,7 @@ def getTimeAgo(ts) -> Optional[str]: return table def getNode( - self, nodeId: str, requestChannels: bool = True, requestChannelRetries: int = 3, timeout: int = 300 + self, nodeId: str, requestChannels: bool = True, requestChannelAttempts: int = 3, timeout: int = 300 ) -> meshtastic.node.Node: """Return a node object which contains device settings and channel info""" if nodeId in (LOCAL_ADDR, BROADCAST_ADDR): @@ -326,7 +326,7 @@ def getNode( if requestChannels: logging.debug("About to requestChannels") n.requestChannels() - retries_left = requestChannelRetries + retries_left = requestChannelAttempts last_index: int = 0 while retries_left > 0: retries_left -= 1 @@ -334,7 +334,7 @@ def getNode( new_index: int = len(n.partialChannels) if n.partialChannels else 0 # each time we get a new channel, reset the counter if new_index != last_index: - retries_left = requestChannelRetries - 1 + retries_left = requestChannelAttempts - 1 if retries_left <= 0: our_exit(f"Error: Timed out waiting for channels, giving up") print("Timed out trying to retrieve channel info, retrying") From 62f5201a38fd62bddea656680f8e51379d310242 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 12:04:22 -0500 Subject: [PATCH 09/14] Add test covering retry logic --- meshtastic/tests/test_mesh_interface.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/meshtastic/tests/test_mesh_interface.py b/meshtastic/tests/test_mesh_interface.py index cace7ebd..bacf85cf 100644 --- a/meshtastic/tests/test_mesh_interface.py +++ b/meshtastic/tests/test_mesh_interface.py @@ -176,6 +176,22 @@ def test_getNode_not_local_timeout(capsys): assert re.match(r"Timed out trying to retrieve channel info, retrying", out) assert err == "" +@pytest.mark.unit +@pytest.mark.usefixtures("reset_mt_config") +def test_getNode_not_local_timeout_attempts(capsys): + """Test getNode not local, simulate timeout""" + iface = MeshInterface(noProto=True) + anode = MagicMock(autospec=Node) + anode.waitForConfig.return_value = False + with patch("meshtastic.node.Node", return_value=anode): + with pytest.raises(SystemExit) as pytest_wrapped_e: + iface.getNode("bar2", requestChannelAttempts=2) + assert pytest_wrapped_e.type == SystemExit + assert pytest_wrapped_e.value.code == 1 + out, err = capsys.readouterr() + assert out == 'Timed out trying to retrieve channel info, retrying\nError: Timed out waiting for channels, giving up\n' + assert err == "" + @pytest.mark.unit @pytest.mark.usefixtures("reset_mt_config") From 8f2c397fbfaf89f9378b98536332bc0203eb35aa Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 14:01:41 -0500 Subject: [PATCH 10/14] =?UTF-8?q?missed=20one=20reference=20for=20requestC?= =?UTF-8?q?hannelRetries=20=F0=9F=A4=A1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- meshtastic/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meshtastic/__main__.py b/meshtastic/__main__.py index 68c785ca..77424c8a 100644 --- a/meshtastic/__main__.py +++ b/meshtastic/__main__.py @@ -275,7 +275,7 @@ def onConnected(interface): # convenient place to store any keyword args we pass to getNode getNode_kwargs = { - "requestChannelRetries": args.channel_fetch_attempts, + "requestChannelAttempts": args.channel_fetch_attempts, "timeout": args.timeout } From 73a1bbc7d59ce27ae938a9ad50108bb360ccf8a9 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 16:02:59 -0500 Subject: [PATCH 11/14] add test coverage for changes to requestChannels --- meshtastic/tests/test_node.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/meshtastic/tests/test_node.py b/meshtastic/tests/test_node.py index 5361296b..668fd4d2 100644 --- a/meshtastic/tests/test_node.py +++ b/meshtastic/tests/test_node.py @@ -842,6 +842,32 @@ def test_requestChannel_localNode(caplog): assert re.search(r"Requesting channel 0", caplog.text, re.MULTILINE) assert not re.search(r"from remote node", caplog.text, re.MULTILINE) +@pytest.mark.unit +def test_requestChannels_non_localNode(caplog): + iface = MagicMock(autospec=SerialInterface) + with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo: + mo.localNode.getChannelByName.return_value = None + mo.myInfo.max_channels = 8 + anode = Node(mo, "bar", noProto=True) + anode.partialChannels = ['0'] + with caplog.at_level(logging.DEBUG): + anode.requestChannels(0) + assert re.search(f"Requesting channel 0 info from remote node", caplog.text, re.MULTILINE) + assert anode.partialChannels == [] + +@pytest.mark.unit +def test_requestChannels_non_localNode_starting_index(caplog): + iface = MagicMock(autospec=SerialInterface) + with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo: + mo.localNode.getChannelByName.return_value = None + mo.myInfo.max_channels = 8 + anode = Node(mo, "bar", noProto=True) + anode.partialChannels = ['1'] + with caplog.at_level(logging.DEBUG): + anode.requestChannels(3) + assert re.search(f"Requesting channel 3 info from remote node", caplog.text, re.MULTILINE) + # make sure it hasn't been initialized + assert anode.partialChannels == ['1'] # @pytest.mark.unit # def test_onResponseRequestCannedMessagePluginMesagePart1(caplog): From e561222ea70045bef48443c795c6d66cf30864ee Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 16:06:44 -0500 Subject: [PATCH 12/14] add docstrings --- meshtastic/tests/test_node.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/meshtastic/tests/test_node.py b/meshtastic/tests/test_node.py index 668fd4d2..61ab84a7 100644 --- a/meshtastic/tests/test_node.py +++ b/meshtastic/tests/test_node.py @@ -844,6 +844,7 @@ def test_requestChannel_localNode(caplog): @pytest.mark.unit def test_requestChannels_non_localNode(caplog): + """Test requestChannels() with a starting index of 0""" iface = MagicMock(autospec=SerialInterface) with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo: mo.localNode.getChannelByName.return_value = None @@ -857,6 +858,7 @@ def test_requestChannels_non_localNode(caplog): @pytest.mark.unit def test_requestChannels_non_localNode_starting_index(caplog): + """Test requestChannels() with a starting index of non-0""" iface = MagicMock(autospec=SerialInterface) with patch("meshtastic.serial_interface.SerialInterface", return_value=iface) as mo: mo.localNode.getChannelByName.return_value = None From 34f9be255e1094784f15d1fdff1cb43744b3f283 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 16:20:50 -0500 Subject: [PATCH 13/14] fix unrelated bug when fromStr input is short hex For example, 0x0 will generate an unhandled ValueError. This was caught during a random unit test run for 3.9, so I figure I'd fix it. This is unrelated to the PR otherwise. --- meshtastic/tests/test_util.py | 8 ++++++++ meshtastic/util.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/meshtastic/tests/test_util.py b/meshtastic/tests/test_util.py index 09f67fed..ac8fa155 100644 --- a/meshtastic/tests/test_util.py +++ b/meshtastic/tests/test_util.py @@ -650,3 +650,11 @@ def test_fuzz_fromStr(valstr): assert isinstance(result, int) except ValueError: assert isinstance(result, str) + +def test_shorthex(): + result = fromStr('0x0') + assert result == b'\x00' + result = fromStr('0x5') + assert result == b'\x05' + result = fromStr('0xffff') + assert result == b'\xff\xff' \ No newline at end of file diff --git a/meshtastic/util.py b/meshtastic/util.py index 8cb177cd..3f864a2c 100644 --- a/meshtastic/util.py +++ b/meshtastic/util.py @@ -82,7 +82,7 @@ def fromStr(valstr): val = bytes() elif valstr.startswith("0x"): # if needed convert to string with asBytes.decode('utf-8') - val = bytes.fromhex(valstr[2:]) + val = bytes.fromhex(valstr[2:].zfill(2)) elif valstr.startswith("base64:"): val = base64.b64decode(valstr[7:]) elif valstr.lower() in {"t", "true", "yes"}: From 2026212a00665aea576937055b22309f8cb38c24 Mon Sep 17 00:00:00 2001 From: Derek Arnold Date: Sun, 15 Sep 2024 16:27:40 -0500 Subject: [PATCH 14/14] please pylint with a docstring and newline --- meshtastic/tests/test_util.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/meshtastic/tests/test_util.py b/meshtastic/tests/test_util.py index ac8fa155..236e7872 100644 --- a/meshtastic/tests/test_util.py +++ b/meshtastic/tests/test_util.py @@ -652,9 +652,10 @@ def test_fuzz_fromStr(valstr): assert isinstance(result, str) def test_shorthex(): + """Test the shortest hex string representations""" result = fromStr('0x0') assert result == b'\x00' result = fromStr('0x5') assert result == b'\x05' result = fromStr('0xffff') - assert result == b'\xff\xff' \ No newline at end of file + assert result == b'\xff\xff'