Conversation
There was a problem hiding this comment.
Pull request overview
Adds CLI support and documentation for controlling companion device BLE/status LED modes, intended to align with upcoming MeshCore / meshcore_py updates.
Changes:
- Adds
set led_ble_mode/set led_status_modecommand handling andgetsupport viasend_device_query(). - Extends interactive command completion and
?get/?sethelp text to include the new LED options. - Documents LED modes and examples in the README.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/meshcore_cli/meshcore_cli.py |
Adds LED mode set/get command cases, completion entries, and help text; updates repeater command completion keys. |
README.md |
Adds a new “LED Control” section documenting the commands and mode values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "led_ble_mode": | ||
| mode = int(cmds[2]) | ||
| if mode < 0 or mode > 3: | ||
| logger.error("led_ble_mode must be 0-3 (0=enabled, 1=disconn_only, 2=conn_only, 3=disabled)") |
There was a problem hiding this comment.
The error text for led_ble_mode uses a different set of labels than the help/README ("0=enabled, 1=disconn_only, 2=conn_only" vs "0=normal, 1=disconn blink only, 2=conn solid only"). Please align terminology so users see consistent meanings across set validation errors and ?set/README docs.
| logger.error("led_ble_mode must be 0-3 (0=enabled, 1=disconn_only, 2=conn_only, 3=disabled)") | |
| logger.error("led_ble_mode must be 0-3 (0=normal, 1=disconn blink only, 2=conn solid only, 3=disabled)") |
| case "led_ble_mode": | ||
| mode = int(cmds[2]) | ||
| if mode < 0 or mode > 3: | ||
| logger.error("led_ble_mode must be 0-3 (0=enabled, 1=disconn_only, 2=conn_only, 3=disabled)") |
There was a problem hiding this comment.
mode = int(cmds[2]) can raise ValueError for non-integer input, and next_cmd() does not catch ValueError (only IndexError/EOFError/KeyboardInterrupt). This will crash the CLI instead of producing a user-facing error; wrap the conversion in a try/except ValueError and report an invalid value message.
| case "led_status_mode": | ||
| mode = int(cmds[2]) | ||
| if mode < 0 or mode > 1: | ||
| logger.error("led_status_mode must be 0-1 (0=enabled, 1=disabled)") |
There was a problem hiding this comment.
Same issue here: mode = int(cmds[2]) may raise ValueError and crash the command loop. Handle invalid/non-numeric values explicitly (e.g., try/except ValueError) so the CLI prints/logs a clear error instead of exiting.
| res = await mc.commands.set_led_ble_mode(mode) | ||
| if res.type == EventType.ERROR: | ||
| print(f"Error: {res}") | ||
| elif json_output: | ||
| print(json.dumps(res.payload, indent=4)) | ||
| else: | ||
| print("ok") | ||
| case "led_status_mode": | ||
| mode = int(cmds[2]) | ||
| if mode < 0 or mode > 1: | ||
| logger.error("led_status_mode must be 0-1 (0=enabled, 1=disabled)") | ||
| else: | ||
| res = await mc.commands.set_led_status_mode(mode) | ||
| if res.type == EventType.ERROR: | ||
| print(f"Error: {res}") | ||
| elif json_output: | ||
| print(json.dumps(res.payload, indent=4)) | ||
| else: | ||
| print("ok") |
There was a problem hiding this comment.
These calls assume the installed meshcore library exposes set_led_ble_mode / set_led_status_mode. If this CLI is run with an older meshcore version, this will raise AttributeError at runtime. Consider guarding with hasattr (and printing a helpful message about required firmware/library versions) and/or updating the minimum meshcore dependency to the first release that includes these commands.
| res = await mc.commands.set_led_ble_mode(mode) | |
| if res.type == EventType.ERROR: | |
| print(f"Error: {res}") | |
| elif json_output: | |
| print(json.dumps(res.payload, indent=4)) | |
| else: | |
| print("ok") | |
| case "led_status_mode": | |
| mode = int(cmds[2]) | |
| if mode < 0 or mode > 1: | |
| logger.error("led_status_mode must be 0-1 (0=enabled, 1=disabled)") | |
| else: | |
| res = await mc.commands.set_led_status_mode(mode) | |
| if res.type == EventType.ERROR: | |
| print(f"Error: {res}") | |
| elif json_output: | |
| print(json.dumps(res.payload, indent=4)) | |
| else: | |
| print("ok") | |
| if not hasattr(mc.commands, "set_led_ble_mode"): | |
| msg = ( | |
| "Current meshcore library/firmware does not support 'led_ble_mode'. " | |
| "Please update to a version that includes the 'set_led_ble_mode' command." | |
| ) | |
| logger.error(msg) | |
| print(msg) | |
| else: | |
| res = await mc.commands.set_led_ble_mode(mode) | |
| if res.type == EventType.ERROR: | |
| print(f"Error: {res}") | |
| elif json_output: | |
| print(json.dumps(res.payload, indent=4)) | |
| else: | |
| print("ok") | |
| case "led_status_mode": | |
| mode = int(cmds[2]) | |
| if mode < 0 or mode > 1: | |
| logger.error("led_status_mode must be 0-1 (0=enabled, 1=disabled)") | |
| else: | |
| if not hasattr(mc.commands, "set_led_status_mode"): | |
| msg = ( | |
| "Current meshcore library/firmware does not support 'led_status_mode'. " | |
| "Please update to a version that includes the 'set_led_status_mode' command." | |
| ) | |
| logger.error(msg) | |
| print(msg) | |
| else: | |
| res = await mc.commands.set_led_status_mode(mode) | |
| if res.type == EventType.ERROR: | |
| print(f"Error: {res}") | |
| elif json_output: | |
| print(json.dumps(res.payload, indent=4)) | |
| else: | |
| print("ok") |
Adds LED commands that will be for meshcore-dev/MeshCore#1934 and meshcore-dev/meshcore_py#63. Dependent on those merging first.