Fix: Explicitly detect PSLab device on Windows 11 (Fixes #269) - #711

Please address the comments from this code review:
## Overall Comments
- Accessing `p.vid` and `p.pid` directly in the port scan can raise `AttributeError` on some platforms/adapters; consider using `getattr(p, "vid", None)` / `getattr(p, "pid", None)` to make the detection more robust.
- The detailed `sys.stderr.write` logging in `async_connect` looks like diagnostic output; if this is intended to remain, consider routing it through the existing logging/debug mechanism or gating it behind a verbosity flag to avoid noisy stderr in normal use.
- Catching a broad `Exception` when instantiating `ScienceLab` and then substituting a `DummyDevice` may hide real connection/setup issues; it might be safer to either narrow the exception types or propagate/log more context so failures are visible to callers.
## Individual Comments
### Comment 1
<location> `scripts/device_detection.py:33-42` </location>
<code_context>
+
  # First connection attempt
-  self.device = ScienceLab()
+  try:
+  if pslab_port:
+  sys.stderr.write(f"Attempting to connect to PSLab on {pslab_port}\n")
+  self.device = ScienceLab(port=pslab_port)
+  else:
+  sys.stderr.write("PSLab not found in port scan, trying auto-detect\n")
+  self.device = ScienceLab()
+  except TypeError:
+  sys.stderr.write("ScienceLab does not accept 'port' argument, falling back to auto-detect\n")
+  self.device = ScienceLab()
+  except Exception as e:
+  sys.stderr.write(f"Error during connection: {e}\n")
+  # Create a dummy device object if instantiation failed completely
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching a broad Exception here can hide real issues and make failures harder to diagnose.
This blanket `except Exception` will also catch unrelated programming or environment errors in `ScienceLab(...)`, potentially causing the code to continue with a `DummyDevice` and obscure real failures. Prefer catching only the specific connection/serial exceptions `ScienceLab` can raise, or at minimum log a full traceback so unexpected errors remain visible.
</issue_to_address>
### Comment 2
<location> `scripts/device_detection.py:40-42` </location>
<code_context>
+  else:
+  sys.stderr.write("PSLab not found in port scan, trying auto-detect\n")
+  self.device = ScienceLab()
+  except TypeError:
+  sys.stderr.write("ScienceLab does not accept 'port' argument, falling back to auto-detect\n")
+  self.device = ScienceLab()
+  except Exception as e:
+  sys.stderr.write(f"Error during connection: {e}\n")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The TypeError fallback may mask other constructor issues, not just a missing 'port' parameter.
Catching bare `TypeError` here will also hide unrelated bugs thrown from within `ScienceLab.__init__`. To only handle the "unsupported `port`" case, consider checking the constructor signature ahead of time or filtering on the exception message before falling back to `ScienceLab()`; otherwise genuine errors will be treated as a normal auto-detect path.
```suggestion
 except TypeError as e:
 # Only treat this as a "no port argument" case if the error is about an
 # unexpected or unsupported 'port' keyword argument. Otherwise, re-raise
 # so genuine constructor bugs are not silently ignored.
 msg = str(e)
 if (
 "unexpected keyword argument 'port'" in msg
 or "got an unexpected keyword argument 'port'" in msg
 or ("port" in msg and "keyword" in msg and "argument" in msg)
 ):
 sys.stderr.write(
 "ScienceLab does not accept 'port' argument, falling back to auto-detect\n"
 )
 self.device = ScienceLab()
 else:
 raise
```
</issue_to_address>
### Comment 3
<location> `scripts/device_detection.py:45-49` </location>
<code_context>
+  self.device = ScienceLab()
+  except Exception as e:
+  sys.stderr.write(f"Error during connection: {e}\n")
+  # Create a dummy device object if instantiation failed completely
+  # so that self.device.connected check doesn't crash
+  class DummyDevice:
+  connected = False
+  self.device = DummyDevice()
+
  output = None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Creating DummyDevice inside the exception block is a bit heavy and may be incomplete if more attributes are used later.
Defining `DummyDevice` in the `except` block creates a new class on every failure and only provides `connected`. If other `self.device` members are used later, this can turn a connection failure into harder-to-debug attribute errors. Consider a lightweight placeholder (e.g. `types.SimpleNamespace(connected=False)`) or a single module-level `DummyDevice` that implements the minimal interface expected downstream.
Suggested implementation:
```python
 except Exception as e:
 sys.stderr.write(f"Error during connection: {e}\n")
 # Create a lightweight dummy device object if instantiation failed completely
 # so that self.device.connected check doesn't crash
 self.device = types.SimpleNamespace(connected=False)
```
You will also need to ensure `types` is imported at the top of `scripts/device_detection.py`, for example:
- Add `import types` alongside the other imports, or
- If there is an existing `import types` line, no further changes are needed.
If other attributes on `self.device` are required elsewhere (e.g. `self.device.read`, `self.device.write`), you may extend the `SimpleNamespace` to include them, for example:
`types.SimpleNamespace(connected=False, read=lambda *a, **k: None, write=lambda *a, **k: None)`.
</issue_to_address>
### Comment 4
<location> `scripts/device_detection.py:24-29` </location>
<code_context>
  def async_connect(self):
+  # Debug: List all ports
+  found_ports = list_ports.comports()
+  sys.stderr.write(f"Scanning {len(found_ports)} ports:\n")
+  pslab_port = None
+  for p in found_ports:
+  sys.stderr.write(f" - {p.device}: VID={p.vid}, PID={p.pid}\n")
+  if p.vid == 0x04D8 and p.pid == 0x00DF:
+  pslab_port = p.device
+
</code_context>
<issue_to_address>
**suggestion:** Unconditional verbose logging to stderr may be noisy in production environments.
This will emit the full port scan (`Scanning...`, `VID/PID`, and connection attempts) to `stderr` on every `async_connect()` call, which can be noisy in GUI apps or where `stderr` is user-visible. Consider gating this behind a debug/verbose flag or routing it through the existing logging system at an appropriate log level instead of using `sys.stderr.write` directly.
Suggested implementation:
```python
 self.device_detection_thread.join()
 def async_connect(self):
 # Debug: List all ports (only logs when DEBUG is enabled)
 found_ports = list_ports.comports()
 if logger.isEnabledFor(logging.DEBUG):
 logger.debug("Scanning %d ports:", len(found_ports))
 pslab_port = None
 for p in found_ports:
 if logger.isEnabledFor(logging.DEBUG):
 logger.debug(" - %s: VID=%s, PID=%s", p.device, p.vid, p.pid)
 if p.vid == 0x04D8 and p.pid == 0x00DF:
 pslab_port = p.device
```
To fully implement this change you should also:
1. Ensure `logging` is imported in `scripts/device_detection.py`, e.g. `import logging`.
2. Ensure a module- or class-level logger named `logger` exists, e.g. `logger = logging.getLogger(__name__)`, or adjust the calls to use whatever logger your codebase already uses (for example, `self.logger` instead of `logger`).
3. Optionally configure the logging level externally (e.g., via your app’s logging config) so that these messages only appear when DEBUG logging is enabled.
</issue_to_address>