-
Notifications
You must be signed in to change notification settings - Fork 350
USB Direct Programming Interface for MinicircuitsUSBSPDT #8033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import TYPE_CHECKING | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import usb1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 4 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from libusb1 import libusb_error | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 5 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from usb1 import USBContext, USBDevice, USBDeviceHandle, USBError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 6 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+4
to
+6
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # QCoDeS imports | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from qcodes.instrument_drivers.Minicircuits.Base_SPDT import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MiniCircuitsSPDTBase, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -12,37 +16,68 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from qcodes.instrument import InstrumentBaseKWArgs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import clr # pyright: ignore[reportMissingTypeStubs,reportMissingImports] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ImportError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ImportError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Module clr not found. Please obtain it by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| installing QCoDeS with the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minicircuits_usb_spdt extra, e.g. by running | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pip install qcodes[minicircuits_usb_spdt]""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MINICIRCUITS_VENDOR_ID = 0x20CE | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RF_SWITCH_PRODUCT_ID = 0x0022 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def open_switch_with_sn(serial_number: str | None) -> USBDeviceHandle | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| usb_context = USBContext() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if serial_number is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| device_iterator = usb_context.getDeviceIterator( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| skip_on_error=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for device in device_iterator: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| device.getVendorID() == MINICIRCUITS_VENDOR_ID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and device.getProductID() == RF_SWITCH_PRODUCT_ID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handle = device.open() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if get_serial_number(handle) == serial_number: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return handle | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| device.close() # Unsure what missing arguments are needed or what they do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check failure on line 38 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+38
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if get_serial_number(handle) == serial_number: | |
| return handle | |
| device.close() # Unsure what missing arguments are needed or what they do | |
| should_close_handle = True | |
| try: | |
| if get_serial_number(handle) == serial_number: | |
| should_close_handle = False | |
| return handle | |
| finally: | |
| if should_close_handle: | |
| handle.close() |
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When serial_number is None, open_switch_with_sn returns the first matching device handle without any interface claim/reset/initialization, but later communication uses interruptWrite/interruptRead directly. This makes the connection path behave differently depending on whether a serial number is provided and is likely to fail unless the interface is claimed somewhere else. Please perform the required device setup (e.g., claim interface 0 / detach kernel driver if needed) after opening the handle in all cases, ideally in MiniCircuitsUsbSPDT.__init__.
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_serial_number calls handle.claimInterface(0) but never releases the interface. This can leave the device in a claimed state (and/or fail subsequent claims) and is especially problematic if iterating over multiple devices. Wrap USB operations in try/finally and release the interface (and consider avoiding resetDevice() unless strictly required).
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver_path parameter was removed from MiniCircuitsUsbSPDT.__init__. This is a breaking change for any downstream code that passed an explicit DLL path. If the goal is to switch transport implementations, consider keeping driver_path as an optional (deprecated) argument for backward compatibility, or document it clearly as removed in the changelog/newsfragment.
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MiniCircuitsUsbSPDT.__init__ does not check whether open_switch_with_sn returned None (no device found / failed open). As written, initialization continues and later calls (e.g. add_channels() -> get_idn()) will raise a generic exception from handle. Please fail fast here with a clear RuntimeError/ConnectionError that includes the requested serial number.
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handle property raises a bare Exception with no message when _handle is None. This makes failures hard to diagnose and is inconsistent with the rest of the driver code (which typically raises RuntimeError with context). Please raise a specific exception with a useful message (e.g., indicating that the USB device failed to open).
| raise Exception # TODO Make better | |
| raise RuntimeError( | |
| "USB device handle is not available; failed to open " | |
| "the Mini-Circuits USB switch." | |
| ) |
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_query_scpi pads commands to 64 bytes via bytearray(64 - len(cmd_bytes)) without validating length. If command is long enough, this will raise a low-level ValueError (negative bytearray length). Add an explicit length check and raise a clear error when the SCPI command cannot fit in a single 64-byte packet.
| cmd_bytes.extend(bytearray(command, "ascii")) | |
| cmd_bytes.extend(bytearray(command, "ascii")) | |
| if len(cmd_bytes) > 64: | |
| raise ValueError( | |
| "SCPI command is too long to fit in a single 64-byte USB packet. " | |
| f"Maximum command length is 62 ASCII bytes, got {len(cmd_bytes) - 2}." | |
| ) |
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_query_scpi assumes responses always contain a NUL terminator and uses response.index(bytearray([0])). If the terminator is missing, this will raise and surface as an unhelpful exception. Please handle missing terminators (and USB errors) explicitly and raise a driver-level error that includes the command that failed.
| self.handle.interruptWrite(endpoint=1, data=cmd_bytes, timeout=50) | |
| response = self.handle.interruptRead(endpoint=1, length=64, timeout=1000) | |
| resp_length = response.index(bytearray([0])) | |
| trimmed_response = response[1:resp_length] | |
| return trimmed_response.decode("ascii") | |
| try: | |
| self.handle.interruptWrite(endpoint=1, data=cmd_bytes, timeout=50) | |
| response = self.handle.interruptRead(endpoint=1, length=64, timeout=1000) | |
| except USBError as exc: | |
| raise RuntimeError( | |
| f"SCPI command {command!r} failed during USB communication." | |
| ) from exc | |
| resp_length = response.find(b"\x00") | |
| if resp_length == -1: | |
| raise RuntimeError( | |
| f"SCPI command {command!r} failed: response was not NUL-terminated." | |
| ) | |
| trimmed_response = response[1:resp_length] | |
| try: | |
| return trimmed_response.decode("ascii") | |
| except UnicodeDecodeError as exc: | |
| raise RuntimeError( | |
| f"SCPI command {command!r} failed: response could not be decoded as ASCII." | |
| ) from exc |
Check failure on line 127 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
GitHub Actions / pytestmypy (windows-latest, 3.12, false)
Type "dict[str, str]" is not assignable to return type "dict[str, str | None]"
"dict[str, str]" is not assignable to "dict[str, str | None]"
Type parameter "_VT@dict" is invariant, but "str" is not the same as "str | None"
Consider switching from "dict" to "Mapping" which is covariant in the value type (reportReturnType)
Check failure on line 127 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
GitHub Actions / pytestmypy (ubuntu-latest, 3.12, false)
Type "dict[str, str]" is not assignable to return type "dict[str, str | None]"
"dict[str, str]" is not assignable to "dict[str, str | None]"
Type parameter "_VT@dict" is invariant, but "str" is not the same as "str | None"
Consider switching from "dict" to "Mapping" which is covariant in the value type (reportReturnType)
Check failure on line 127 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
GitHub Actions / pytestmypy (ubuntu-latest, 3.13, false)
Type "dict[str, str]" is not assignable to return type "dict[str, str | None]"
"dict[str, str]" is not assignable to "dict[str, str | None]"
Type parameter "_VT@dict" is invariant, but "str" is not the same as "str | None"
Consider switching from "dict" to "Mapping" which is covariant in the value type (reportReturnType)
Check failure on line 127 in src/qcodes/instrument_drivers/Minicircuits/_minicircuits_usb_spdt.py
GitHub Actions / pytestmypy (ubuntu-latest, 3.14, false)
Type "dict[str, str]" is not assignable to return type "dict[str, str | None]"
"dict[str, str]" is not assignable to "dict[str, str | None]"
Type parameter "_VT@dict" is invariant, but "str" is not the same as "str | None"
Consider switching from "dict" to "Mapping" which is covariant in the value type (reportReturnType)
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_idn returns the raw responses from MN? and SN?. The existing ethernet driver strips the MN=/SN= prefixes (see _minicircuits_rc_spdt.py), and MiniCircuitsSPDTBase.get_number_of_channels() expects model to start with RC or USB and contain a dash-separated channel count. If the USB device returns the same prefixed format, channel detection will break. Please normalize MN/SN to match the existing driver output (e.g., strip MN=/SN= and whitespace).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new imports include several unused symbols (
os,usb1,libusb_error,USBDevice,USBError). Unused imports will fail linting and also make it harder to see which APIs are actually required. Please remove them (or use them if they are needed).