Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive serial port discovery and diagnostics module to the Mouser application, addressing issue #380. The module provides hardware-aware COM port enumeration with vendor/product identification, safe accessibility testing, and device classification heuristics (RFID readers, balances, etc.). The implementation includes extensive unit tests with 32 test cases covering VID/PID parsing, port classification, safe probing with mocked hardware, and full enumeration scenarios. Two utility scripts support standalone operation: a log generator for non-GUI diagnostics and a PyInstaller build script for creating a deployable PoC executable.
Changes:
- Added
shared/port_discovery.pywith lazy-initialized logger, VID/PID parsing, safe open/close probing, port classification, and Linux glob fallback for pyserial enumeration issues - Added comprehensive test suite in
tests/test_port_discovery.pywith 32 tests using mocked serial hardware - Added standalone diagnostic script
generate_port_discovery_log.pyand PyInstaller build scriptbuild_port_discovery_exe.py
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/port_discovery.py | Core port discovery module with VID/PID parsing, safe port probing, classification heuristics, Linux fallback, and rotating file logger |
| tests/test_port_discovery.py | Comprehensive unit tests covering VID/PID parsing, classification, safe probing, enumeration, and logging with fully mocked serial hardware |
| generate_port_discovery_log.py | Standalone script to run port discovery diagnostics without GUI, saves output to log file and creates snapshot for deliverable |
| build_port_discovery_exe.py | PyInstaller build configuration for creating Mouser_PoC_PortDiscovery.exe |
| port_discovery_log_output.txt | Sample log output showing port discovery format (no ports detected example) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
The test suite doesn't include test coverage for the Linux fallback mechanism (shared/port_discovery.py lines 200-213). Consider adding a test that mocks platform.system() to return "Linux", mocks comports() to return an empty list, and mocks glob.glob() to return test devices like ["/dev/ttyUSB0"], verifying that the fallback correctly creates _Shim objects with proper attributes.
| @patch("shared.port_discovery.platform.system") | |
| @patch(_COMPORTS) | |
| @patch("shared.port_discovery.glob.glob") | |
| def test_linux_fallback_used_when_no_comports( | |
| self, mock_glob, mock_comports, mock_system | |
| ): | |
| """On Linux, fallback globbing is used when comports() returns no ports.""" | |
| mock_system.return_value = "Linux" | |
| mock_comports.return_value = [] | |
| mock_glob.return_value = ["/dev/ttyUSB0"] | |
| result = discover_ports(safe_probe=False) | |
| assert len(result) == 1 | |
| assert result[0]["device"] == "/dev/ttyUSB0" |
|
|
||
| # Build the PyInstaller argument list | ||
| args = [ | ||
| os.path.join(PROJECT_ROOT, "main.py"), |
There was a problem hiding this comment.
The build script is using main.py as the entry point, but main.py launches the full GUI application (as seen in main.py lines 60-80). For a port discovery PoC executable that runs diagnostics without the GUI, the entry point should be generate_port_discovery_log.py instead. Change this line to: os.path.join(PROJECT_ROOT, "generate_port_discovery_log.py")
| os.path.join(PROJECT_ROOT, "main.py"), | |
| os.path.join(PROJECT_ROOT, "generate_port_discovery_log.py"), |
| python generate_port_discovery_log.py | ||
|
|
||
| Output: | ||
| logs/mouser_startup.log (appended – rotating file) |
There was a problem hiding this comment.
The comment indicates the log file will be "logs/mouser_startup.log" but the actual code creates "logs/mouser_port_discovery.log" (as defined in shared/port_discovery.py line 58). Update the comment to match the actual filename.
| logs/mouser_startup.log (appended – rotating file) | |
| logs/mouser_port_discovery.log (appended – rotating file) |
| opened – bool, True if the port could be opened | ||
| latency – float, seconds the open/close cycle took | ||
| error – str or None |
There was a problem hiding this comment.
The docstring states "latency – float, seconds the open/close cycle took" but the actual implementation returns latency_ms in milliseconds (line 141, 143, 147, 151). Either update the docstring to say "latency_ms – float, milliseconds the open/close cycle took" or rename the dict key to "latency" and document it correctly.
| opened – bool, True if the port could be opened | |
| latency – float, seconds the open/close cycle took | |
| error – str or None | |
| opened – bool, True if the port could be opened | |
| latency_ms – float, milliseconds the open/close cycle took | |
| error – str or None |
| def get_log_file_path() -> str: | ||
| """Return the path of the active log file (initialises logger if needed).""" |
There was a problem hiding this comment.
If the logger is already configured (e.g., in tests, line 48-49), the function returns early without setting _log_file_path. This means get_log_file_path() would return None in such cases. Consider setting _log_file_path even when handlers exist, or documenting that get_log_file_path() may return None in test environments.
| def get_log_file_path() -> str: | |
| """Return the path of the active log file (initialises logger if needed).""" | |
| def get_log_file_path() -> str | None: | |
| """Return the path of the active log file (initialises logger if needed). | |
| Note: If the logger has been configured externally (e.g. in tests without | |
| a file handler), this function may return ``None``. | |
| """ |
| DATAS = [ | ||
| (os.path.join(PROJECT_ROOT, "settings"), "settings"), | ||
| (os.path.join(PROJECT_ROOT, "shared", "sounds"), os.path.join("shared", "sounds")), | ||
| (os.path.join(PROJECT_ROOT, "shared", "images"), os.path.join("shared", "images")), | ||
| ] | ||
|
|
||
| # Hidden imports that PyInstaller may miss | ||
| HIDDEN_IMPORTS = [ | ||
| "serial", | ||
| "serial.tools", | ||
| "serial.tools.list_ports", | ||
| "serial.tools.list_ports_common", | ||
| "serial.tools.list_ports_windows", | ||
| "customtkinter", | ||
| "CTkMessagebox", | ||
| "CTkMenuBar", | ||
| "tkcalendar", | ||
| "pandas", | ||
| "cryptography", | ||
| "getmac", | ||
| ] | ||
|
|
||
| # Build the PyInstaller argument list | ||
| args = [ | ||
| os.path.join(PROJECT_ROOT, "main.py"), | ||
| "--name", "Mouser_PoC_PortDiscovery", | ||
| "--noconfirm", | ||
| "--clean", | ||
| "--onedir", | ||
| "--console", # console visible for log output during PoC testing | ||
| ] | ||
|
|
||
| # Add data files | ||
| for src, dest in DATAS: | ||
| args.extend(["--add-data", f"{src}{SEP}{dest}"]) | ||
|
|
||
| # Add hidden imports | ||
| for mod in HIDDEN_IMPORTS: | ||
| args.extend(["--hidden-import", mod]) | ||
|
|
||
| # Collect-all for customtkinter (it bundles themes / assets) | ||
| args.extend(["--collect-all", "customtkinter"]) |
There was a problem hiding this comment.
The port discovery PoC executable is intended to be a standalone diagnostic tool without GUI (as per PR description and generate_port_discovery_log.py lines 3-5). However, the build script includes GUI-related dependencies (customtkinter, CTkMessagebox, CTkMenuBar, tkcalendar at lines 32-35, 60) and data files like images and sounds (lines 20-22) that aren't needed for port discovery. These should be removed to reduce executable size and complexity for the diagnostic tool.
| log_file = get_log_file_path() | ||
| log.info("Port discovery complete. Log file: %s", log_file) | ||
| log.info("=" * 70) | ||
|
|
||
| print(f"\nDiagnostic log written to: {log_file}") | ||
|
|
||
| # Copy snapshot for deliverable | ||
| dest = os.path.join(os.path.dirname(__file__), "port_discovery_log_output.txt") | ||
| shutil.copy2(log_file, dest) |
There was a problem hiding this comment.
If get_log_file_path() returns None (which can happen when the logger is pre-configured with handlers), then log_file will be None, causing shutil.copy2() at line 42 to fail with a TypeError. Add a check for None before attempting to copy the file, or ensure get_log_file_path() never returns None.
…_file_path to UPPER_CASE module constants\n- Remove redundant serial reimports inside functions\n- Extract _log_single_port helper to reduce branch/statement count\n- Add docstrings, fix membership tests, keyword-only args in tests\n- Add apt-get update before portaudio19-dev install in Pylint.yaml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DATAS = [ | ||
| (os.path.join(PROJECT_ROOT, "settings"), "settings"), | ||
| (os.path.join(PROJECT_ROOT, "shared", "sounds"), os.path.join("shared", "sounds")), | ||
| (os.path.join(PROJECT_ROOT, "shared", "images"), os.path.join("shared", "images")), | ||
| ] |
There was a problem hiding this comment.
The DATAS list includes settings, sounds, and images directories that are not used by the port discovery module. Since generate_port_discovery_log.py only performs hardware diagnostics without a GUI, these data files are unnecessary and will bloat the executable size. Remove these entries to keep the executable lean.
| """Empty or None hardware-ID returns all None.""" | ||
| assert _parse_vid_pid("")["vid"] is None | ||
| assert _parse_vid_pid("")["pid"] is None |
There was a problem hiding this comment.
The test comment states "Empty or None hardware-ID returns all None" but only tests empty string. Add a test case for None input (e.g., _parse_vid_pid(None)) to verify the function handles None correctly, not just empty strings.
| """Empty or None hardware-ID returns all None.""" | |
| assert _parse_vid_pid("")["vid"] is None | |
| assert _parse_vid_pid("")["pid"] is None | |
| """Empty or None hardware-ID returns all None.""" | |
| # Empty string input | |
| assert _parse_vid_pid("")["vid"] is None | |
| assert _parse_vid_pid("")["pid"] is None | |
| # None input | |
| assert _parse_vid_pid(None)["vid"] is None | |
| assert _parse_vid_pid(None)["pid"] is None |
| HIDDEN_IMPORTS = [ | ||
| "serial", | ||
| "serial.tools", | ||
| "serial.tools.list_ports", | ||
| "serial.tools.list_ports_common", | ||
| "serial.tools.list_ports_windows", | ||
| "customtkinter", | ||
| "CTkMessagebox", | ||
| "CTkMenuBar", | ||
| "tkcalendar", | ||
| "pandas", | ||
| "cryptography", | ||
| "getmac", | ||
| ] |
There was a problem hiding this comment.
The hidden imports include many GUI-related dependencies (customtkinter, CTkMessagebox, CTkMenuBar, tkcalendar) that are not needed for a standalone port discovery diagnostic tool. These should be removed since the executable should only run port discovery diagnostics without launching the GUI. Only the serial-related imports (serial, serial.tools.*) are necessary for this tool.
| # Collect-all for customtkinter (it bundles themes / assets) | ||
| args.extend(["--collect-all", "customtkinter"]) | ||
|
|
There was a problem hiding this comment.
The collect-all directive for customtkinter is unnecessary since the port discovery executable should not include any GUI components. This should be removed after fixing the main entry point to use generate_port_discovery_log.py.
| # Collect-all for customtkinter (it bundles themes / assets) | |
| args.extend(["--collect-all", "customtkinter"]) |
| for pattern in ("/dev/ttyUSB*", "/dev/ttyACM*"): | ||
| for dev in glob.glob(pattern): | ||
| class _Shim: # pylint: disable=too-few-public-methods | ||
| """Lightweight stand-in for a missing ListPortInfo.""" | ||
| def __init__(self, device): | ||
| self.device = device | ||
| self.description = "Unknown (glob fallback)" | ||
| self.hwid = "" | ||
| self.manufacturer = None | ||
| self.product = None | ||
| self.interface = None |
There was a problem hiding this comment.
The _Shim class is defined inside a nested loop, causing it to be redefined for every matching device file. Move the class definition outside the loops (right after the platform check on line 199) to avoid unnecessary redefinition and improve code clarity.
| for pattern in ("/dev/ttyUSB*", "/dev/ttyACM*"): | |
| for dev in glob.glob(pattern): | |
| class _Shim: # pylint: disable=too-few-public-methods | |
| """Lightweight stand-in for a missing ListPortInfo.""" | |
| def __init__(self, device): | |
| self.device = device | |
| self.description = "Unknown (glob fallback)" | |
| self.hwid = "" | |
| self.manufacturer = None | |
| self.product = None | |
| self.interface = None | |
| class _Shim: # pylint: disable=too-few-public-methods | |
| """Lightweight stand-in for a missing ListPortInfo.""" | |
| def __init__(self, device): | |
| self.device = device | |
| self.description = "Unknown (glob fallback)" | |
| self.hwid = "" | |
| self.manufacturer = None | |
| self.product = None | |
| self.interface = None | |
| for pattern in ("/dev/ttyUSB*", "/dev/ttyACM*"): | |
| for dev in glob.glob(pattern): |
|
|
||
| import os | ||
|
|
||
| import PyInstaller.__main__ |
There was a problem hiding this comment.
PyInstaller is used in this build script but is not listed in requirements.txt. Add PyInstaller to requirements.txt to ensure the build environment has all necessary dependencies.
| 2026-02-23 14:36:44 | INFO | Detected 0 port(s): | ||
| 2026-02-23 14:36:44 | INFO | (none) | ||
| 2026-02-23 14:36:44 | INFO | ---------------------------------------------------------------------- | ||
| 2026-02-23 14:36:44 | INFO | Port discovery complete. Log file: C:\Users\Panic\Capstone\Mouser\logs\mouser_port_discovery.log |
There was a problem hiding this comment.
The log output file contains a hardcoded Windows path (C:\Users\Panic\Capstone\Mouser...) that appears to be from the developer's local machine. This file should be regenerated in CI or the path should be sanitized before committing to avoid exposing local filesystem information in the repository.
| 2026-02-23 14:36:44 | INFO | Port discovery complete. Log file: C:\Users\Panic\Capstone\Mouser\logs\mouser_port_discovery.log | |
| 2026-02-23 14:36:44 | INFO | Port discovery complete. Log file: logs/mouser_port_discovery.log |
| class TestDiscoverPorts: | ||
| """Tests for the main port enumeration function.""" | ||
|
|
||
| @patch(_COMPORTS) | ||
| def test_no_ports(self, mock_comports): | ||
| """Returns empty list when no ports are detected.""" | ||
| mock_comports.return_value = [] | ||
| result = discover_ports(safe_probe=False) | ||
| assert not result | ||
|
|
||
| @patch(_COMPORTS) | ||
| def test_single_port_no_probe(self, mock_comports): | ||
| """Single port enumerated without probing.""" | ||
| mock_comports.return_value = [ | ||
| _make_fake_port("COM3", description="USB-SERIAL CH340", | ||
| hwid="USB VID:PID=1A86:7523 SER=123") | ||
| ] | ||
| result = discover_ports(safe_probe=False) | ||
| assert len(result) == 1 | ||
| assert result[0]["device"] == "COM3" | ||
| assert result[0]["vid"] == "1A86" | ||
| assert result[0]["pid"] == "7523" | ||
| assert result[0]["serial_number"] == "123" | ||
| assert "probe" not in result[0] | ||
|
|
||
| @patch("shared.port_discovery._safe_open_close") | ||
| @patch(_COMPORTS) | ||
| def test_single_port_with_probe(self, mock_comports, mock_probe): | ||
| """Single port with safe_probe=True includes probe result.""" | ||
| mock_comports.return_value = [ | ||
| _make_fake_port("COM3", description="USB-SERIAL CH340", | ||
| hwid="USB VID:PID=1A86:7523") | ||
| ] | ||
| mock_probe.return_value = {"opened": True, "latency_ms": 1.5, "error": None} | ||
|
|
||
| result = discover_ports(safe_probe=True) | ||
| assert len(result) == 1 | ||
| assert result[0]["probe"]["opened"] is True | ||
| mock_probe.assert_called_once_with("COM3") | ||
|
|
||
| @patch(_COMPORTS) | ||
| def test_multiple_ports_sorted(self, mock_comports): | ||
| """Ports are returned sorted by device name.""" | ||
| mock_comports.return_value = [ | ||
| _make_fake_port("COM5"), | ||
| _make_fake_port("COM1"), | ||
| _make_fake_port("COM3"), | ||
| ] | ||
| result = discover_ports(safe_probe=False) | ||
| devices = [r["device"] for r in result] | ||
| assert devices == ["COM1", "COM3", "COM5"] | ||
|
|
||
| @patch(_COMPORTS) | ||
| def test_port_classification_included(self, mock_comports): | ||
| """Each port entry includes a 'category' field.""" | ||
| mock_comports.return_value = [ | ||
| _make_fake_port("COM3", description="RFID Reader", | ||
| hwid="USB VID:PID=1A86:7523") | ||
| ] | ||
| result = discover_ports(safe_probe=False) | ||
| assert result[0]["category"] == "RFID Reader" | ||
|
|
||
| @patch(_COMPORTS) | ||
| def test_unknown_vid(self, mock_comports): | ||
| """Port with unrecognized VID is classified as Unknown.""" | ||
| mock_comports.return_value = [ | ||
| _make_fake_port("COM3", description="Generic Device", | ||
| hwid="USB VID:PID=FFFF:FFFF") | ||
| ] | ||
| result = discover_ports(safe_probe=False) | ||
| assert result[0]["category"] == "Unknown" | ||
|
|
||
|
|
There was a problem hiding this comment.
The port_discovery module includes a Linux-specific glob fallback mechanism (lines 198-211 in shared/port_discovery.py) for detecting /dev/ttyUSB* and /dev/ttyACM* devices when pyserial's comports() returns no results. However, there are no tests covering this fallback logic. Add tests to verify the Linux glob fallback works correctly, similar to the test_linux_fallback_when_no_pyserial_results test in tests/test_ports.py.
Fixes #380
What was changed?
Added a serial port discovery module, unit tests, a standalone log generator, a PyInstaller build script, and a sample log output. The new files are shared/port_discovery.py, tests/test_port_discovery.py, generate_port_discovery_log.py, build_port_discovery_exe.py, and port_discovery_log_output.txt.
Why was it changed?
The application had no way to enumerate or diagnose connected serial hardware at startup. When users report connectivity issues with RFID readers or balances, there was no diagnostic data to work from. This module provides non-destructive device detection logging that can be used for troubleshooting without opening the full GUI.
How was it changed?
shared/port_discovery.py is a self-contained module with its own rotating file logger. Key functions include _safe_open_close() which probes port accessibility without reading or writing data, _classify_port() which categorizes devices using keyword heuristics and a known VID table for CH340, FTDI, CP210x and others, discover_ports() which enumerates all COM ports, and log_port_discovery() which writes formatted results to the log.
tests/test_port_discovery.py contains 32 tests across 5 classes covering VID/PID parsing, classification, mocked serial probes, port enumeration, and log output verification. All serial hardware is mocked so tests run on any machine.
generate_port_discovery_log.py runs port discovery without the GUI and saves output to logs/mouser_port_discovery.log.
build_port_discovery_exe.py builds the Mouser_PoC_PortDiscovery.exe via PyInstaller.