Discourage the use of direct setting of interface specific properties in Instruments

[dkriegner]

Directly setting interface specific communication parameters as suggested in the documentation in the Single Interface section:

e.g.:

def __init__(self, adapter, name="Extreme 5000", baud_rate=2400, **kwargs):
    super().__init__(
        adapter,
        name,
        baud_rate=baud_rate,
        **kwargs
    )

leads to the disadvantage that this device can only be used with this particular connection. Even if a device might offer only a specific hardware interface many transparent converters exist (e.g. RS232 to Ethernet) which can make the device visible at the control computer under a different interface.

With the VisaAdapter one would only need to change the resource name and a connection would be possible if the syntax proposed in the multiple interfaces section of the documentation is used.

For the example above, this would mean:

def __init__(self, adapter, name="Extreme 5000", baud_rate=2400, **kwargs):
    super().__init__(
        adapter,
        name,
        asrl={'baud_rate': baud_rate},
        **kwargs
    )

I therefore would propose to discourage using the "single interface" way and thereby allow the user to use hardware adapters more easily.

In particular for serial device communication I could anyhow only find the LakeShore421 device which uses the "single interface"-way

Other solutions: @bmoneke pointed out in #800 that a user can always specify an Adapter and therefore also the single interface implementation would allow to be used with communication Adapters. While this is true, it requires the user to reapply all other default parameters like read/write_termination, timeout, ... which typically are unchanged if a transparent converter is used.

[LongnoseRob]

Comment from the side-line: pyvisa manages certain properties on resource-level such as read\write_termination these should not be globally overwritten. --> further detail here

[bilderbuchi]

As I'm unfamiliar with these ethernet-serial converters: As I understood it, they are addressed via a TCPIP... (in any case, not ASRL...) VISA resource string. However, if they "transparently" convert, where do they take the baud rate to choose from? IIUC, your proposed way of doing aasrl={'baud_rate': baud_rate} would mean that converter does not get that baud rate information at all.

[dkriegner]

Maybe "transparent" is not a good word here.

We use varies types of such converters in our lab to converge all the communication to Ethernet. The different types work all in the same way:

1. One needs to configure the serial interface parameters: baudrate, databits, stopbits, ... (typically via a web interface)
2. After that, they are addressed via "TCPIP.." VISA resource strings without the need to perform any particular settings of the communication.

With transparent I referred to the fact that with this setup above every byte sent to the TCP/IP port is sent further on the serial interface.

So in fact when using such converters it's sort of indented to ignore the asrl dict.

[bilderbuchi]

Hmmm, I see. So, probably a Moxa NPort or something like that? With your converter, when using baud_rate=baud_rate, you get an error from pyvisa that for TCPIP connections this is an invalid argument, correct? Can you show such an error?

[BenediktBurger]

One point to consider for consistency: If we put the baud rate into that dict. Why not also the read / write termination? The term chars can (and do) differ for different connection types to the same instrument.

For your implementation you need "globally" defined term chars and "local" (that dict) baud rate, right?

Another solution: Could we add an option to ignore excessive kwargs in the Visa Adapter?

[bilderbuchi]

If we put the baud rate into that dict. Why not also the read / write termination? The term chars can (and do) differ for different connection types to the same instrument.

That's a good point/question.

Another solution: Could we add an option to ignore excessive kwargs in the Visa Adapter?

We could feed the kwargs to pyvisa and pop-off-nonexistinent-kwargs, log and retry if the instantiation fails, until it works. We'll also probably have scary log messages from pyvisa. I'd prefer if we find a "cleaner" way to deal with this than swallowing kwargs.

[bilderbuchi]

While this is true, it requires the user to reapply all other default parameters like read/write_termination, timeout, ... which typically are unchanged if a transparent converter is used.

• One needs to configure the serial interface parameters: baudrate, databits, stopbits, ... (typically via a web interface)

• After that, they are addressed via "TCPIP.." VISA resource strings without the need to perform any particular settings of the communication.

Wait, I'm confused. You say that the (serial) communication parameters are set via web interface (on the translator). But then you also say that all other default parameters have to be reapplied (when using a separate Adapter instance) -- why? you also say that you don't need any particular settings of the communication?

[BenediktBurger]

The cleanest solution is to create your adapter instance manually and hand that to the instrument. For that you have to know the instrument details, though and give them manually (at least some) to that adapter.

Through this discussion, the manual adapter creation seems for me the best way for these converters. (I know, that it is not the most convenient way)

[BenediktBurger]

As all the connection information is in the init, it should not be difficult to create adapter instances for your devices.

[msmttchr]

As all the connection information is in the init, it should not be difficult to create adapter instances for your devices.

I fully agree with this approach which is in line with PyMeasure custom adapter use cases.

[BenediktBurger]

I have an idea, how we could facilitate the use of an external adapter (like Prologix or in this case):

Instruments have their connection settings in a dictionary at class level. If started with a resource name, the instrument takes this dict for the visa Adapter. Any user may use this dictionary easily for instantiating their own adapter (ignoring some parts of that dict, if necessary).

Would that solution be fine with you @dkriegner?

This would be the small Implementation necessary in Instrument:

class Instrument:
    comm_kwargs = {} # for compatibility with older instruments
    def init:
    ... VisaAdapter(..., **self.comm_kwargs, **kwargs)

For less name clashes, it could be **self.comm_kwargs.update(kwargs) as well. That way manual definitions replace those done in that dict.

[dkriegner]

I try to catch up with the discussion:

Hmmm, I see. So, probably a Moxa NPort or something like that?

I do not know this particular one, but it looks comparable to what we are using.

With your converter, when using baud_rate=baud_rate, you get an error from pyvisa that for TCPIP connections this is an invalid argument, correct? Can you show such an error?

sorry for the screenshot.

Wait, I'm confused. You say that the (serial) communication parameters are set via web interface (on the translator). But then you also say that all other default parameters have to be reapplied (when using a separate Adapter instance) -- why? you also say that you don't need any particular settings of the communication?

With the other default parameters I mean the terminators. Since the Serial/Ethernet translator passes through everything 1:1 these are the same for this instrument independent of the used hardware connection. I agree that there are cases where depending on the hardware connection these are different, but this seems already solved since one can specify read/write_termination in the asrl (and equivalent) dictionaries. A test for this is already in place: https://github.com/pymeasure/pymeasure/blob/9013d6f6d2c5666063b30d6b0bbe8d34deae859d/tests/instruments/test_connection_configuration.py#L61-L62

The solution proposed by @bmoneke sounds promising to me. The only thing I am wondering is if you do not replace one dictionary which you seem to dislike by another one? Code might get even more complicated with the proposed solution as it is with when using the MultiProtocol Adapter approach.

If you prefer to take no action (or even disallow code as in PR #800) then it just makes the user code duplicating the terminators in case such an adapter is used. I can live with that, but would find the already existing multiprotocol way the best solution.

[dkriegner]

One more note: IMHO this Serial/Ethernet translators can't be compared with the Prologix Adapter. The Prologix adapter has many commands it processes on its own via the same interface used for the device communication.

[BenediktBurger]

I mentioned the PrologixAdapter, as it has to be created manually (and requires the information).

My problem with your solution is, that it is unclear, why some parameters should be defined for all connection types (term chars) and others only for specific connection types (baud_rate). What do we do with the timeout? Is it global or specific? The ethernet connection adds lag and therefore might require a larger timeout.

With my solution, it is clear, that you define globally, what all (or the only) connection type require and specifically, what only some connection types require.

[dkriegner]

Ok, I agree. Likely many Instruments need to adapt this new behavior but this can likely be done step by step.

Regarding timeout: Indeed this is a valid concern, but it can't be solved in pymeasure. The added lag strongly depends on which kind of translator is used. Cheap ones we had added >100ms, while the better ones are almost negligible (few ms). But changing timeout does not require to define one's own Adapter, so I think it is a minor issue. -> So I would put it to the global dictionary except a specific device requires it otherwise.

[bilderbuchi]

With my solution, it is clear, that you define globally, what all (or the only) connection type require and specifically, what only some connection types require.

However, isn't this already possible right now, if you pass

...
asrl={'baud_rate': baud_rate}, # only for asrl
read_termination='\r\n',  # global
...

arguments? I'm not sure I see what the additional indirection through a class-level dict (that gets overridden by regular kwargs if needed) brings in functionality? :confused: How does it resolve this point

that it is unclear, why some parameters should be defined for all connection types (term chars) and others only for specific connection types (baud_rate).

better than what we currently use?

[BenediktBurger]

However, isn't this already possible right now, if you pass

Yes. My solution (in comparison to dkriegners) keeps that mechanics as is, but makes it more simple to create an adapter manually (because you have all the information available in one point). In fact, with that, you could use a SerialAdapter easily if you liked.

Therefore we do not need to change, how we define the connection.

I hope that made it clearer.

[dkriegner]

I must say the more I think about it, the more I like the proposed solution. It defines a central place to define common Adapter arguments while still maintaining that they can be overwritten as needed.

The proposed solution basically replaces (most of these) setdefault statements, which are now in many devices:

def __init__(self, adapter, name="Extreme 5000", **kwargs):
    kwargs.setdefault('timeout', 120000)
    kwargs.setdefault('read_termination', '\n')
    super().__init__(
        adapter,
        name,
        **kwargs
    )

And at the same time makes it easier to use different Adapters (which now requires studying the code of the Instrument).

[bilderbuchi]

I hope that made it clearer.

It did not clear up the question of how it improves the situation

that it is unclear, why some parameters should be defined for all connection types (term chars) and others only for specific connection types (baud_rate).

[BenediktBurger]

The original request of this issue was, to put some parameters globally and others specifically, due to the requirements of a specific converter.

I propose to follow the documentation: define globally, if possible, and specifically if needed (due to different connection possibilities).

[bilderbuchi]

So, if we go with an Instrument.adapter_kwargs dict as #828 proposes, how does that resolve the original problem of having general kwargs in single-interface instruments that trip up tcpip translator devices?

If the instrument now has SomeInstr.adapter_kwargs = {'baud_rate': 2400} instead of having a default value in a signature, this still fails with a TCPIP... resource string. So, you have to check the contents of that dict, and strip out the baud_rate arg before passing it to your adapter ctor. You save the effort of looking in the signature, or the init code for arguments.

However, all other users now lose the ability to recognize from the signature (and thus also from Python's help facilities) that there is a baud_rate setting with a default, that might be adapted by them. If you want to preserve that info, now you have to duplicate that default setting in both the signature and your class-level kwarg, to preserve the advantage when creating a separate adapter (that needs a certain subset of those args).

Honestly, from my point of view I'm not sure if the proposed remedy does not have more of a negative impact than thought.

I'm slowly thinking that the way with less impact might be to, as the issue titles suggests, discourage giving protocol-specific connection configuration without the protocol selector, i.e. guide people to write asrl={'baud_rate': baud_rate} instead of baud_rate=baud_rate. That will take some education on which args are fully generic (termination, e.g.), but it would not add indirection, duplicate information, and would solve (afaict) the original problem of creating a TCPIP connection with a translator device inbetween.

The drawback will be that people with a translator device might be required to manually look at the instrument init to identify the appropriate settings to manually transfer to the translator device. Which, afaict, they would otherwise get from manually looking at the content of the adapter_kwargs dict.

[bilderbuchi]

I propose to follow the documentation: define globally, if possible, and specifically if needed (due to different connection possibilities).

Ah, I agree with that (I think). I thought you referred to the proposed to switch to the class-level adapter_kwargs way.

[dkriegner]

So, you have to check the contents of that dict, and strip out the baud_rate arg before passing it to your adapter ctor. You save the effort of looking in the signature, or the init code for arguments.

I believe there is still a misunderstanding here. As far as I understand #828, baud_rate is interface specific and therefore never should go into the class-level adapter_kwargs (at least not as baud_rate: 2400). It therefore would also not be lost from the call signature. What should not be in the call signature are the parameters like: "read_termination", "write_termination", "timeout". I think there is little danger of duplication if done right, but identifying what needs to go where could be challenging.

It's true that a user with a translator has to look into the code/manual in any case to identify the settings for the translator. But with #828 one should be able to assume that:

adapter = PrologixAdapter("resource_name", address=5, **MyInstrument.adapter_kwargs)
inst = MyInstrument(adapter)

works, and in your code you could still write

def __init__(self, adapter, name="My Instrument", baud_rate=2400, **kwargs):
    super().__init__(
        adapter,
        name,
        baud_rate=baud_rate,
        **kwargs
    )

avoiding the "asrl" dict.

That being said: I am of course not against my original proposal. It would certainly be a much more minor change.

[BenediktBurger]

@dkriegner your two examples are mutually exclusive unless the adapter_kwargs contains the baud_rate (maybe in asrl={'baud_rate': 2400}) as well. If we have that single point of information, it should contain all information.

Here is a list of all the inits (grep -rzo "__init__(self, adapter[^)]*)" *), rarely any contains connection information in their signature:

 activetechnologies/AWG401x.py:__init__(self, adapter, **kwargs)
 activetechnologies/AWG401x.py:__init__(self, adapter, **kwargs)

 activetechnologies/AWG401x.py:__init__(self, adapter, **kwargs)
 advantest/advantestR3767CG.py:__init__(self, adapter, **kwargs)
 agilent/agilent33220A.py:__init__(self, adapter, **kwargs)
 agilent/agilent33500.py:__init__(self, adapter, **kwargs)
 agilent/agilent33521A.py:__init__(self, adapter, **kwargs)
 agilent/agilent34410A.py:__init__(self, adapter, **kwargs)
 agilent/agilent34450A.py:__init__(self, adapter, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, channel, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, channel, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, channel, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, var_name, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, **kwargs)
 agilent/agilent4156.py:__init__(self, adapter, **kwargs)
 agilent/agilent8257D.py:__init__(self, adapter, **kwargs)
 agilent/agilent8722ES.py:__init__(self, adapter, **kwargs)
 agilent/agilentB1500.py:__init__(self, adapter, **kwargs)
 agilent/agilentE4408B.py:__init__(self, adapter, **kwargs)
 agilent/agilentE4980.py:__init__(self, adapter, **kwargs)
 ametek/ametek7270.py:__init__(self, adapter, **kwargs)
 ami/ami430.py:__init__(self, adapter, **kwargs)
 anaheimautomation/dpseriesmotorcontroller.py:__init__(self, adapter, address=0, encoder_enabled=False, **kwargs)
 anapico/apsin12G.py:__init__(self, adapter, **kwargs)
 andeenhagerling/ah2500a.py:__init__(self, adapter, name=None, timeout=3000,
                 write_termination="\n", read_termination="\n",
                 **kwargs)
 andeenhagerling/ah2700a.py:__init__(self, adapter, name="Andeen Hagerling 2700A Precision Capacitance Bridge",
                 timeout=5000, **kwargs)
 anritsu/anritsuMG3692C.py:__init__(self, adapter, **kwargs)
 anritsu/anritsuMS2090A.py:__init__(self, adapter, name="Anritsu MS2090A Handheld Spectrum Analyzer", **kwargs)
 anritsu/anritsuMS9710C.py:__init__(self, adapter, name="Anritsu MS9710C Optical Spectrum Analyzer", **kwargs)
 anritsu/anritsuMS9740A.py:__init__(self, adapter, **kwargs)
 bkprecision/bkprecision9130b.py:__init__(self, adapter, **kwargs)
 danfysik/danfysik8500.py:__init__(self, adapter, **kwargs)
 deltaelektronika/sm7045d.py:__init__(self, adapter, **kwargs)
 edwards/nxds.py:__init__(self, adapter, **kwargs)
 fakes.py:__init__(self, adapter=None, name=None, includeSCPI=False, **kwargs)
 fluke/fluke7341.py:__init__(self, adapter, **kwargs)
 fwbell/fwbell5080.py:__init__(self, adapter, **kwargs)
 hcp/tc038.py:__init__(self, adapter, address=1, timeout=1000,
                 includeSCPI=False, **kwargs)
 hcp/tc038d.py:__init__(self, adapter, name="TC038D", address=1, timeout=1000,
                 **kwargs)
 heidenhain/nd287.py:__init__(self, adapter, units="mm", **kwargs)
 hp/hp33120A.py:__init__(self, adapter, **kwargs)
 hp/hp3437A.py:__init__(self, adapter, **kwargs)
 hp/hp34401A.py:__init__(self, adapter, **kwargs)
 hp/hp3478A.py:__init__(self, adapter, **kwargs)
 hp/hp8116a.py:__init__(self, adapter, **kwargs)
 hp/hp8657b.py:__init__(self, adapter, **kwargs)
 hp/hplegacyinstrument.py:__init__(self, adapter, name="HP legacy instrument", **kwargs)
 hp/hpsystempsu.py:__init__(self, adapter, **kwargs)
 instrument.py:__init__(self, adapter, name, includeSCPI=True,
                 **kwargs)
 keithley/keithley2000.py:__init__(self, adapter, **kwargs)
 keithley/keithley2260B.py:__init__(self, adapter, read_termination="\n", **kwargs)
 keithley/keithley2306.py:__init__(self, adapter, **kwargs)
 keithley/keithley2400.py:__init__(self, adapter, **kwargs)
 keithley/keithley2450.py:__init__(self, adapter, **kwargs)
 keithley/keithley2600.py:__init__(self, adapter, **kwargs)
 keithley/keithley2700.py:__init__(self, adapter, **kwargs)
 keithley/keithley2750.py:__init__(self, adapter, **kwargs)
 keithley/keithley6221.py:__init__(self, adapter, **kwargs)
 keithley/keithley6517b.py:__init__(self, adapter, **kwargs)
 keysight/keysightDSOX1102G.py:__init__(self, adapter, **kwargs)
 keysight/keysightE36312A.py:__init__(self, adapter, name="Keysight E36312A", **kwargs)
 keysight/keysightN5767A.py:__init__(self, adapter, **kwargs)
 keysight/keysightN7776C.py:__init__(self, adapter, **kwargs)
 lakeshore/lakeshore331.py:__init__(self, adapter, **kwargs)
 lakeshore/lakeshore421.py:__init__(self, adapter, baud_rate=9600, **kwargs)
 lakeshore/lakeshore425.py:__init__(self, adapter, **kwargs)
 lecroy/lecroyT3DSO1204.py:__init__(self, adapter, chunk_size) lecroy/lecroyT3DSO1204.py:__init__(self, adapter, **kwargs)
 mksinst/mks937b.py:__init__(self, adapter, name="MKS 937B vacuum gauge controller", address=253, **kwargs)
 newport/esp300.py:__init__(self, adapter, **kwargs)
 parker/parkerGV6.py:__init__(self, adapter, **kwargs)
 pendulum/cnt91.py:__init__(self, adapter, **kwargs)
 razorbill/razorbillRP100.py:__init__(self, adapter, **kwargs)
 rohdeschwarz/fsl.py:__init__(self, adapter, **kwargs)
 rohdeschwarz/hmp.py:__init__(self, adapter, **kwargs)
 rohdeschwarz/sfm.py:__init__(self, adapter, **kwargs)
 siglenttechnologies/siglent_spd1168x.py:__init__(self, adapter, **kwargs)
 siglenttechnologies/siglent_spd1305x.py:__init__(self, adapter, **kwargs)
 siglenttechnologies/siglent_spdbase.py:__init__(self, adapter, **kwargs)
 signalrecovery/dsp7265.py:__init__(self, adapter, **kwargs)
 srs/sg380.py:__init__(self, adapter, **kwargs)
 srs/sr510.py:__init__(self, adapter, **kwargs)
 srs/sr570.py:__init__(self, adapter, **kwargs)
 srs/sr830.py:__init__(self, adapter, **kwargs)
 srs/sr860.py:__init__(self, adapter, **kwargs)
 tektronix/afg3152c.py:__init__(self, adapter, **kwargs)
 tektronix/tds2000.py:__init__(self, adapter, **kwargs)
 temptronic/temptronic_ats525.py:__init__(self, adapter, **kwargs)
 temptronic/temptronic_ats545.py:__init__(self, adapter, **kwargs)
 temptronic/temptronic_base.py:__init__(self, adapter, name="ATSBase", **kwargs)
 temptronic/temptronic_eco560.py:__init__(self, adapter, **kwargs)
 texio/texioPSW360L30.py:__init__(self, adapter, **kwargs)
 thermotron/thermotron3800.py:__init__(self, adapter, **kwargs)
 thorlabs/thorlabspm100usb.py:__init__(self, adapter, **kwargs)
 thorlabs/thorlabspro8000.py:__init__(self, adapter, **kwargs)
 toptica/ibeamsmart.py:__init__(self, adapter, name="Toptica IBeam Smart laser diode", **kwargs)
 yokogawa/yokogawa7651.py:__init__(self, adapter, **kwargs)
 yokogawa/yokogawags200.py:__init__(self, adapter, **kwargs)

[dkriegner]

Ok, if you want really everything in the adapter_kwargs I agree with @bilderbuchi that duplication is unavoidable and clearly a negative thing. I thought that adapter_kwargs will only contain what you need for all adapters.

[bilderbuchi]

My yardstick for what should go into the signature: Parameters that the end user can be expected to want to tweak. Having those in the signature directly informs the user about which "knobs" are available to turn.

Unfortunately, due to the large variation in instrument (hardware) implementations we are interfacing with, what exactly can/should be changed varies from instrument to instrument...

• A primary candidate would be adjusting a timeout, as people have faster/slower or more or less capable serial adapters/PC serial peripherals, and want reliable and as fast as possible communication
• Similar reasoning for baud_rate. Some instrument hardware can be configured with what they can deal with (so you'd adjust accordingly on the pymeasure side). However, some others might have it hard-coded, then it does not make sense to put this in the signature, as changing it would render comms nonfunctional. Many just use the default serial settings, but some don't.
• Similar case for read_termination/write_termination. Some instrument hardware can configure it (by used transport method, even), for some it's hard-coded. For some the termination chars differ by interface, for some it's the same across them all.

Add to that the fact that we want implementations/interfaces to be as clear as possible (to reduce confusion/bug potential/duplication), and as flexible as possible (to accomodate use cases of varying "exoticness"), you can imagine that there's a certain tension, and resulting challenge in identifying a "good" design approach/implementation guidelines.

[dkriegner]

I understand (and agree with) your reasoning!

So I think it boils down to what I originally proposed: Keep the core code as is with all its flexibility. If you want to help supporting user-defined Adapters it would likely be good to discourage what is suggested in the single interface part of the documentation. This would not change anything regarding the possible call signatures of the Instrument. It merely would mean to sometimes have one dictionary more than absolutely needed inside the Instrument.__init__.

[BenediktBurger]

User defined adapters ignore the connection settings in the instrument, therefore it does not matter, whether it is defined as single connection or multi connection.

[dkriegner]

User defined adapters ignore the connection settings in the instrument, therefore it does not matter, whether it is defined as single connection or multi connection.

Of course you are right. Given that most of the time one will use "just" a different Visa resource name the single connection definition basically disables this option.

[BenediktBurger]

I don't understand you.

If you have

adapter = SomeAdapter(...)
Inst = MyInst(adapter...)

The adapter does not get anything defined in "MyInst.init".