QSwitch.py update to work with firmware version 2.0#420
QSwitch.py update to work with firmware version 2.0#420JoostvanderHeijden wants to merge 10 commits into
Conversation
In firmware version 2.0, the ethernet communication goes via a UDP connection instead of TCP/IP. This driver therefore requires the connection to be defined (UDP, TCP/IP or serial) to allow backwards compatibility.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the QSwitch driver to support firmware version 2.0 by introducing UDP communication along with maintaining backwards compatibility using TCP/IP and serial (USB) connections. Key changes include updating the base class from VisaInstrument to Instrument, adding UDP branches in the init, write, and ask methods, and renaming the serial setup method to _set_up_visa.
| raise ValueError(f'Error: {errors} after executing {cmd}') | ||
| if self._udp_mode: # UDP (ethernet) commands | ||
| cmd_lower = cmd.lower() | ||
| is_open_close_cmd = cmd_lower.find("clos ",0,12) != -1 or (cmd_lower.find("close ",0,12) != -1) or (cmd_lower.find("open ",0,12) != -1) |
There was a problem hiding this comment.
[nitpick] Consider clarifying the detection of open/close commands. Using substring search such as 'clos ' may be error prone; a more robust parsing approach could improve reliability.
| is_open_close_cmd = cmd_lower.find("clos ",0,12) != -1 or (cmd_lower.find("close ",0,12) != -1) or (cmd_lower.find("open ",0,12) != -1) | |
| is_open_close_cmd = re.match(r"^(clos|close|open)\s", cmd_lower) is not None |
There was a problem hiding this comment.
This suggestion won't work, since the open and close commands can have a precursor of "route:" they do not need to be the first part of the string. Also, "close:stat?" would query the status of closed relays, and should not be included in this command. So searching "close ", "clos ", and "open ", seems the most efficient way.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
trim trailing whitespaces
| from time import sleep as sleep_s | ||
| from qcodes.instrument.parameter import DelegateParameter | ||
| from qcodes.instrument.visa import VisaInstrument | ||
| from qcodes.instrument.visa import Instrument |
There was a problem hiding this comment.
| from qcodes.instrument.visa import Instrument | |
| from qcodes.instrument import Instrument |
| @@ -2,14 +2,16 @@ | |||
| import itertools | |||
| from time import sleep as sleep_s | |||
| from qcodes.instrument.parameter import DelegateParameter | |||
There was a problem hiding this comment.
| from qcodes.instrument.parameter import DelegateParameter | |
| from qcodes.parameters import DelegateParameter |
| Tuple, Sequence, List, Dict, Set, Union, Optional) | ||
| from packaging.version import parse | ||
| import socket | ||
| import pyvisa as visa |
There was a problem hiding this comment.
The following imports are missing
from typing_extensions import Unpack
from qcodes.instrument import VisaInstrumentKWArgs
from typing import cast
| class QSwitch(Instrument): | ||
|
|
||
| def __init__(self, name: str, address: str, **kwargs) -> None: | ||
| def __init__(self, name: str, address: str, **kwargs: "Unpack[InstrumentBaseKWArgs]") -> None: |
There was a problem hiding this comment.
| def __init__(self, name: str, address: str, **kwargs: "Unpack[InstrumentBaseKWArgs]") -> None: | |
| def __init__(self, name: str, address: str, **kwargs: "Unpack[VisaInstrumentKWArgs]") -> None: |
There was a problem hiding this comment.
Since we are making use of visa args here like visalib
| self._check_instrument_name(name) | ||
| super().__init__(name, address, terminator='\n', **kwargs) | ||
| self._set_up_serial() | ||
| if 'ASRL' in address: |
There was a problem hiding this comment.
I suggest simplifying this to something like
if 'ASRL' in address or 'TCPIP' in address:
self._udp_mode = False
self._switch = cast(visa.resources.MessageBasedResource, visa.ResourceManager(visalib).open_resource(address))
self._set_up_visa()
Since these 2 branches are identical. Note that the visa resource is always a MessageBasedResource but in principle could be other resources so we need the cast here
| self._switch.read_termination = '\n' | ||
| self._switch.timeout = 5000 | ||
| self._switch.query_delay = 0.01 | ||
| self._switch.baud_rate = 9600 |
There was a problem hiding this comment.
This is not an attribute for non serial instruments. Effectively this just creats a new attr. Either this line should only be set for serial instuments (wrapped in if isinstance ... logic or the line should have a #type: ignore added to it
|
@JoostvanderHeijden Sorry for the delay. Left explanation inline of the remaining issues |
In the latest QSwitch firmware version 2.0 the ethernet communication protocol has been updated to UDP. As UDP is not available for VISA instruments, the class has been changed to instrument. TCP/IP (for compatibility with older firmware versions) and serial (USB) communication are also still available.