Adapter and Instrument methods

[msmttchr]

As highlighted in #559 (comment) by @CodingMarco, there could be some design issue in the way Instrument and Adapter class implement some methods.
In particular, values and ask methods implemented in Adapter class are simply calling other methods or making text processing which has nothing to do with low level instrument access.
This requires, in certain circumstances, to reimplement them at Instrument class level even though no changes are needed.
A possible solution is to move the default implementation of these methods from Adapter to Instrument

[msmttchr]

Possibly related to #512

[bilderbuchi]

I see you point I think, although I'm wondering if the problem in #559 (i.e., the need to copy the values() implementation) could have been avoided by encapsulating the device-specific comms requirements in a custom Adapter derived e.g. from VISAAdapter, instead of overriding the methods of Instrument. See also #352/ TopticaAdapter. Maybe we should put some recommendation to that effect (and pointer to examples) in the docs?

[msmttchr]

@bilderbuchi I agree some guidelines could be extremely useful, prior to this I would make some effort to clarify the software architecture and what we target with Instrument and what with Adapter classes or may be is the same task.
Also, I have the feeling that there are too many Adapter subclasses and we are using mainly VISAAdapter.

[msmttchr]

In my understanding, looking at the code, I see the following:

• Adapter class/subclasses: they allow to abstract different physical instrument interfaces, for example: VISA, Serial, Telnet, Prologix. Probably Serial and Telnet only exist to support the two variant of Prologix gpib interfaces.
• Instrument class encapsulate behaviour and control interface.

If an adapter is not available, user can develop its own and pass to Instrument class creator.

According to this interpretation, putting instrument behaviour/control inside an adapter could be inappropriate.

Finally, Instrument class normally provide a set of properties and methods to drive it and read and write methods as a way to have a lower level interface.

Some open points:

• What is the recommended paradigm to implement instrument using a binary interface (as opposed to text based interface) ?
• Are users allowed to reimplement read and write method when subclassing Instrument? In which cases ?
• What about timeout and delays ? Do they need to be implemented at Instrument class level or Adapter level ?
• If my instrument needs a specific set of initialization commands, where should they go ? In Instrument or Adapter part ?
• If my instrument has a GPIB interface and the instrument developer created a custom VISAAdapter to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom VISAAdapter. Would it be Instrument a better place to include all the instrument control?
• Message based instrument control is very popular. Are there any other paradigms worth to have some implementation guidelines in a future documentation ?

Just sharing some ideas and doubts to encourage the contribution and have finally better documentation

[LongnoseRob]

In my understanding, looking at the code, I see the following:

* Adapter class/subclasses: they allow to abstract different physical instrument interfaces, for example: VISA, Serial, Telnet, Prologix. Probably Serial and Telnet only exist to support the two variant of Prologix gpib interfaces.

* Instrument class encapsulate behaviour and control interface.

If an adapter is not available, user can develop its own and pass to Instrument class creator.
I have a similiar opinion,

• many modern instruments come with different interface options
• Serial and Telnet could be dropped/delegated as pyvisa also supports these.
• Maybe in the future someone will add new adapters for e.g. EtherCat, Profinet or other Fieldbus-protocols or an adapterr for interaction with LinuxCNC

According to this interpretation, putting instrument behaviour/control inside an adapter could be inappropriate.
Agreed

Finally, Instrument class normally provide a set of properties and methods to drive it and read and write methods as a way to have a lower level interface.

Some open points:

* What is the recommended paradigm to implement instrument using a binary interface (as opposed to text based interface) ?

In this case a custom wrapper-class could be proposed to do the interfaceing between the properties in the instrument definition and the backend (adpter-class and lower HW IF)

There is also the case that we have a mixed communcation, some pars are text-based and some are binary.

* Are users allowed to reimplement `read` and `write` method when subclassing `Instrument`? In which cases ?

* What about timeout and delays ? Do they need to be implemented at Instrument class level or Adapter level ?

IMO, these should be defined in the Instrument class as I think they are instrument-specific

* If my instrument needs a specific set of initialization commands, where should they go ? In Instrument or Adapter part ?

* If my instrument has a GPIB interface and the instrument developer created a custom `VISAAdapter` to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom `VISAAdapter`. Would it be `Instrument` a better place to include all the instrument control?

It might also be worth to think about if in this example the functionality added could also be ported to theunderlaying VISA lib, e.g. pyvisa,

* Message based instrument control is very popular. Are there any other paradigms worth to have some implementation guidelines in a future documentation ?

SCPI itself also supports event-based communication, however for use with pyvisa, pyvisa-py and linux-gpib this is not fully supported yet. Commercial VISA libaries and other HW-drivers might support this.

Just sharing some ideas and doubts to encourage the contribution and have finally better documentation

We should not forget that not everybody working with pymeasure is the absolute python-developer.
Many of the user are students, hobbieist, semi-professionals, who come across pymeasure in one or the other way to solve a problem.

Good documentation is important and I also see it as some kind of advertising for a project.
If I look across what has been implemented on the instruments, we have a wide range of devices already, some RF-stuff, some optics, the typical oscilloscope, power supplies, multi-meters, AWG, etc. all the way to stpper motor drivers and super-conducting magnet controllers.

With this base there should be enough availble to build a libary of use-cases/sucess-stories which could be used as more advanced examples.
For the academics, we could also think about a cite-proposal.

But this might be out the scope of this discussion, back to the more technical discussion about the separation line between adapters and instruments

[bilderbuchi]

In my understanding, looking at the code, I see the following:

Agreed on all points

What is the recommended paradigm to implement instrument using a binary interface (as opposed to text based interface) ?

I think we should probably choose a frame-based method (i.e., message frames are a header + payload), I've seen that in several non-SCPI instruments. After getting the safekeywords and dynamic properties issues sorted, this is near/at the top of my "big-feature" priority list. There is #296 and some beginning in #239 to review/adapt. Maybe it makes sense to take a stab at #249 first, to be able to test what the WIP implementation sends over the wire reasonably easily/without a device. PyVISA also has a RegisterBasedResource that might be useful, but investigating that is still todo.

Are users allowed to reimplement read and write method when subclassing Instrument? In which cases ?

The most frequent pattern is Channel classes derived from Instrument, where the read/write are overridden to append prefixed depending on channel ID. This patter works quite well so far, what could use improvement is that a Channel only has to override read and write, not ask, too (problematic because of our choice to use pyvisa query in VISAAdapter's ask)

What about timeout and delays ? Do they need to be implemented at Instrument class level or Adapter level ?

IMO an Adapter-level concern, but controlled by the Instrument with params passed on during construction. Stuff like query_delay and I think some others still need harmonization among our adapters (#409, #512)

If my instrument needs a specific set of initialization commands, where should they go ? In Instrument or Adapter part ?

Instrument initialiser, after the super call.

If my instrument has a GPIB interface and the instrument developer created a custom VISAAdapter to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom VISAAdapter. Would it be Instrument a better place to include all the instrument control?

Depends, I guess. What do you mean with instrument "behaviour"? Protocol quirks? I think these should be in the adapter, especially if they are shared among several of that manufacturer's model. If having this is mandatory, the instrument implementor should make sure that only an appropriate Adapter instance (or int/str) is passed to the instrument initialiser (as otherwise the device would not work, anyway.

Why do you need/want to use another PrologixAdapter if a specific adapter is available (I'm not familiar with the Prologix stuff)

Message based instrument control is very popular. Are there any other paradigms worth to have some implementation guidelines in a future documentation ?

I see

• frame-based , i.e. header + payload, probably binary (seems within reach and scope for us),
• dll calls, probably proprietary library, most prominently Thorlabs (consider it out of scope because quite complex, lots of API surface, others support that already, we don't have the manpower)
• Address+value + operation (r/w), e.g. Modbus (probably out of scope for us, have also not seen demand, could maybe be based off the frame-based stuff)

I don't know yet where pyvisa RegisterBasedResource fits in, and if that is actually maintained/used in the wild or just a skeleton.

[bilderbuchi]

custom wrapper-class could be proposed to do the interfaceing between the properties in the instrument definition and the backend (adpter-class and lower HW IF)

Yes, this is one thing we need to hash out -- if we have a non-messagebased adapter, how do we define our commands? Right now, everything revolves around control and get_command/set_command, but that does not make much sense in that context. Do we define some DSL with specific command syntax that address some intermediate layer, or some logic in the adapter? Or are the command strings just binary values/maybe addresses?
How do the protocols of various frame-based devices look like?
I'm assuming (confirm/refute please if you can) that there are no devices that have an interchangeable/selectable SCPI and frame-based protocol (possible exception: thorlabs with "old" style SCPI I think, and new style with dll). Does it even make sense to have an Instrument class that supports both/either? Should we split off into SCPIInstrument/MessageBasedInstrument and TBCInstrument? I'd be hesitant to throw away/reimplement a lot of stuff.

To start with, I want to study #239 (again) to get a feeling for things.

SCPI itself also supports event-based communication,

can you explain/summarize that a little more? What is it?

Good documentation is important and I also see it as some kind of advertising for a project.

Absolutely, see #102 and documentation . I think the docs could use a restructuring along the lines of https://documentation.divio.com/ and expansion as in that issue. Alas, manpower :-/ -- do you want to help out, with guidance/reviews from us? If so, let's continue in #102

[LongnoseRob]

custom wrapper-class could be proposed to do the interfaceing between the properties in the instrument definition and the backend (adpter-class and lower HW IF)

Yes, this is one thing we need to hash out -- if we have a non-messagebased adapter, how do we define our commands? Right now, everything revolves around control and get_command/set_command, but that does not make much sense in that context. Do we define some DSL with specific command syntax that address some intermediate layer, or some logic in the adapter? Or are the command strings just binary values/maybe addresses? How do the protocols of various frame-based devices look like? I'm assuming (confirm/refute please if you can) that there are no devices that have an interchangeable/selectable SCPI and frame-based protocol (possible exception: thorlabs with "old" style SCPI I think, and new style with dll). Does it even make sense to have an Instrument class that supports both/either? Should we split off into SCPIInstrument/MessageBasedInstrument and TBCInstrument? I'd be hesitant to throw away/reimplement a lot of stuff.

To start with, I want to study #239 (again) to get a feeling for things.

SCPI itself also supports event-based communication,

can you explain/summarize that a little more? What is it?

Maybe my phrase was not clear and misleading;
I was thinking about event based communication in pyVISA in combination with the extended status reporting defined in SCPI99 page 50

Good documentation is important and I also see it as some kind of advertising for a project.

Absolutely, see #102 and documentation . I think the docs could use a restructuring along the lines of https://documentation.divio.com/ and expansion as in that issue. Alas, manpower :-/ -- do you want to help out, with guidance/reviews from us? If so, let's continue in #102

I will have a look at this and might chime in some ideas/comments in #102

[bilderbuchi]

I was thinking about event based communication in pyVISA in combination with the extended status reporting defined in SCPI99 page 50

Thanks!

[msmttchr]

Are users allowed to reimplement read and write method when subclassing Instrument? In which cases ?

The most frequent pattern is Channel classes derived from Instrument, where the read/write are overridden to append prefixed depending on channel ID. This patter works quite well so far, what could use improvement is that a Channel only has to override read and write, not ask, too (problematic because of our choice to use pyvisa query in VISAAdapter's ask)

I think we're focusing all our consideration on VISAAdapter (based on PyVISA) which, at the end of the day, is covering all the supported interfaces.
I think PyVISA is a great library and the only shortcomings are the relatively complex setup for NI/Keysight GPIB cards (but this is not PyVISA fault) and lack of support of cheap Prologix interfaces.

We could imagine to base everything on PyVISA and drop Prologix support.
(I am sorry, but I don't see any use case for SerialAdapter or TelnetAdapter).
I am not in favour of this option, but I mention it only to clarify my thinking, because it would leave all users relying on Prologix out in the cold.

Other considerations are:

• GPIB is someway obsolete in new instruments, but still very used, so Prologix adapters may become less and less relevant
• New instruments are mainly using LAN or USB interfaces which does not require specific hardware adapter
• SCPI still very popular, but innovative instruments are using other communication languages (e.g. https://github.com/jetperch/pyjoulescope)

In my opinion, we may have two choices:

• Base everything on PyVISA and question why Adapter classes should exist at all
• Keep current structure and limit Adapter classes scope to support different hardware adapters.

What about timeout and delays ? Do they need to be implemented at Instrument class level or Adapter level ?

IMO an Adapter-level concern, but controlled by the Instrument with params passed on during construction. Stuff like query_delay and I think some others still need harmonization among our adapters (#409, #512)

I agree, but when the flexibility defined is not enough, do we need to define a specific adapter or not (my current opinion is no).

If my instrument needs a specific set of initialization commands, where should they go ? In Instrument or Adapter part ?

Instrument initialiser, after the super call.

I agree, but then I don't understand the code here https://github.com/pymeasure/pymeasure/blob/9f265683a2bd28e8a87139e8d01cad37baaafc27/pymeasure/instruments/toptica/adapters.py, where a custom adapter includes instrument specific settings that are lost when you are using a different adapter.

If my instrument has a GPIB interface and the instrument developer created a custom VISAAdapter to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom VISAAdapter. Would it be Instrument a better place to include all the instrument control?

Depends, I guess. What do you mean with instrument "behaviour"? Protocol quirks? I think these should be in the adapter, especially if they are shared among several of that manufacturer's model. If having this is mandatory, the instrument implementor should make sure that only an appropriate Adapter instance (or int/str) is passed to the instrument initialiser (as otherwise the device would not work, anyway.

Why do you need/want to use another PrologixAdapter if a specific adapter is available (I'm not familiar with the Prologix stuff)

GPIB instruments can be interfaced with hardware adapters from National Instruments or other brands supported by PyVisa. You can also use Prologix hardware adapters that are much cheaper and offer a USB (Virtual serial port) or Ethernet interface (unfortunately Prologix is not yet suppoted by PyVISA, pyvisa/pyvisa#112).
So, here we have a bit of name clashing for the term adapter as python design pattern and adapter as hardware interface between instrument and controller.

[bilderbuchi]

AFAIK, USB and LAN can be handled by PyVISA, too.

I agree, but then I don't understand the code here https://github.com/pymeasure/pymeasure/blob/9f265683a2bd28e8a87139e8d01cad37baaafc27/pymeasure/instruments/toptica/adapters.py, where a custom adapter includes instrument specific settings that are lost when you are using a different adapter.

You'll notice that the instrument only accepty int/str and then uses only that specific adapter, so lost settings are not a concern. Any other adapter would not make sense/work anyway because of the specific communication quirks handled by that adapter.

So, here we have a bit of name clashing for the term adapter as python design pattern and adapter as hardware interface between instrument and controller.

Huh, I never understood our "adapter" as a (GoF) design pattern, but as encapsulating the hardware comm/protocol side of things (as opposed to the command set, which belongs to Instrument). At the same time, I did not realise the special role of prologix things as (if you allow the imprecision) "meta-adapters-in-between".

[bilderbuchi]

Re supporting Prologix:

Might be worth it to ping the pyvisa maintainer in that issue, he seems open to merging a PR (and there is some draft code below that comment). It sounds like you have experience/ a Prologix device to test -- maybe a PR adding prologix compatibility to pyvisa would be feasible? also, @paulgoulain who has been active was in that issue, too, maybe you can collaborate?

Failing that, do we need to adapt PrologixAdapter? Does that make sense, or not? Clearly, I don't know enough how these interfaces work or are used, and I don't have a device nor spare time to read up on that.

If this is the main pain point:

If my instrument has a GPIB interface and the instrument developer created a custom VISAAdapter to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom VISAAdapter. Would it be Instrument a better place to include all the instrument control?

Maybe we can find a design pattern to account for that use case? Off the top of my head, if a PrologixAdapter is passed in, but a specific protocol is defined in a ClassSpecificAdapter, wrap that in a PrologixAdapter somehow, or rip out the defined methods and put them into a PrologixAdapter?
Or have an intermediate-layer BehaviourDefinition that both PrologixAdapter and VISAAdapter could use?

Probably would be easier to see if we can get Prologix support merged upstream, then we could probably really, as you say, retire most other adapters in favour of VISAAdapter. 😄

[msmttchr]

Huh, I never understood our "adapter" as a (GoF) design pattern, but as encapsulating the hardware comm/protocol side of things (as opposed to the command set, which belongs to Instrument). At the same time, I did not realise the special role of prologix things as (if you allow the imprecision) "meta-adapters-in-between".

I had the same understanding of hardware adapter. In summary VISAAdapter (via PyVISA) covers pretty much everything, except Prologix adapter, so we need to shed some light in:

• Instrument adapter parameter: what is for ? [My understanding: if you have any hardware interface not covered by VISAAdapter, pass your own Adapter, eg. PrologixAdapter or anything user developed].
  -Different instruments with same interface (e.g. GPIB). I have two different instruments both offering GPIB interface, but language is different: one may be SCPI and another proprietary. Do I need to create a dedicated adapter for one or both instruments or the difference is captured in the Instrument class ? [I believe the latter]
• 'Protocol quirks': what are they ? why I need to add a dedicated Adapter for it ?

The summary question could be, if I ignore for a moment Prologix and I always use VISAAdapter why do I need any other Adapter class at all ?

[bklebel]

Huh, I never understood our "adapter" as a (GoF) design pattern, but as encapsulating the hardware comm/protocol side of things (as opposed to the command set, which belongs to Instrument). At the same time, I did not realise the special role of prologix things as (if you allow the imprecision) "meta-adapters-in-between".

I agree here, I understood the pymeasure "adapter" as encapsulating the communication behavior, while the "Instrument" defines the concrete command set, regardless over which type of communication they are actually sent. So, even if the PrologixAdapter were to be integrated into the VISAAdapter, I think the logical separation of the communication and commands would be important for readability of the code.

If my instrument has a GPIB interface and the instrument developer created a custom VISAAdapter to include some instrument's behaviour, I think I can no longer use my Prologix adapter since it doesn't include the behaviour implemented in the custom VISAAdapter. Would it be Instrument a better place to include all the instrument control?

Maybe we can find a design pattern to account for that use case? Off the top of my head, if a PrologixAdapter is passed in, but a specific protocol is defined in a ClassSpecificAdapter, wrap that in a PrologixAdapter somehow, or rip out the defined methods and put them into a PrologixAdapter?
Or have an intermediate-layer BehaviourDefinition that both PrologixAdapter and VISAAdapter could use?

How about Mixins? I know, this adds complexity to the codebase, and new contributors would possibly need to understand how mixins work in the first place in python, but I think this could be solved through a good documentation, and possibly templates.
The way I imagine it would be to have the special communication behavior of a certain instrument, which needs a custom adapter, implemented only in a mixin class, like a BehaviourDefinition_InstrumentXY, which can then be used with any of the communication adapters. I am not sure whether this would work in all cases, but for example it might be just the thing for #586.

Maybe this could be done even with fully fleshed out adapter classes like
BehaviorDefinition_OxfordInstruments(object),
VISAAdapter(Adapter),
OxInst_Visa(BehaviorDefinition_OxfordInstruments, VISAAdapter),
PrologixAdapter,
OxInst_Visa(BehaviorDefinition_OxfordInstruments, PrologixAdapter),
where in the OxInst_XX classes no definitions occur anymore, they are just there to be used immediately, without a new user needing to understand how they should place the mixin class properly for the inheritance to work as intended.

[bilderbuchi]

Yeah, Mixins could be a nice solution to this problem, as we typically only have to override a couple of classes. There are a couple of rules/guidelines to observe (order of inheritance, super usage, should be named XYMixin) but as you say, docs and maybe templates can help here, and also we probably can restrict this so that only instrument implementors need to be aware, not instrument users.
We previously discussed mixins in the context of how to integrate SCPI functionality in #416 (relevant comment, also on the downsides)

I think here the scope is more limited and the changes are limited to a couple of adapter classes we control (so it's easier to ensure correct usage), so it might be the more attractive option this time around.
I'm thinking that instrument implementors define the instrument subclass and the mixin as needed (based on documentation/examples - the different use cases are few), and pass it to the Instrument constructor via the super call, along with whatever adapter is (similar to how we now handle connection settings). Inside Instrument.__init__ we can combine those as appropriate (e.g. make sure the inheritance order is correct).

So, a user story for the #586 case,

• define class ITC503(Instrument)
• define class OxfordBehaviourMixin
• In ITC503 ctor, use super().__init__(args, behaviour_override=OxfordBehaviourMixin,...)

In Instrument.__init__, something like this sketch could work:

if behaviour_override is not None:
    if isinstance(adapter, Adapter):
        class CustomAdapter(behaviour_override, adapter):
            pass
    else:
        class CustomAdapter(behaviour_override, VISAAdapter):
            pass
    adapter = CustomAdapter(adapter, **kwargs)

This way, there is no need to manually create various adapter subclasses

Alternatively, we could put the mixin on the Instrument subclass (class ITC503(OxfordBehaviourMixin, Instrument)) and override not the adapter's, but the instrument's methods, leaving adapter completely transparent (which hews more towards @msmttchr's approach of associating these protocol quirks more with the instrument than the adapter).
I don't know/haven't checked if only modifying on instrument level can fulfill all protocol modification use cases we have so far encountered.
Advantage: no dynamically defined classes necessary, so less complex/more straightforward/easy to reason about or statically analyse.

[msmttchr]

My gut feeling is that last draft proposal from @bilderbuchi is preferred. In my mind, it is a more linear approach.
Some points are not clear to me, for example:

• Can Mixin include attributes and properties?
• Can Mixin be derived from other classes ? This would be necessary to support dynamic properties.

Regarding the Instrument write/read/ask/values methods, I would define them as proper method and not as bypass to the Adapter class.
For ask method, I would be tempted to implement this in software as a combination of read and write (I think this is also @bilderbuchi opinion). values method definitely implemented in Instrument class. read and write inherited from appropriate mixin (PyVISA wrapper in most cases, but also supporting Prologix or other exhotic adapters)
Of course, the impact on existing code base needs to be evaluated.

[bilderbuchi]

My gut feeling is that last draft proposal from @bilderbuchi is preferred.

Sorry for not numbering my alternatives. I'm assuming you mean this approach "put the mixin on the Instrument subclass". Correct?

Can Mixin include attributes and properties?

Yes, it's a normal class. It is primarily convention that Python mixins are named BlablaMixin, and that they only define methods that override another class's methods. Attributes are possible afaik but I suggest that they remain _private. In effect, Mixins do not add public API to a class.

Can Mixin be derived from other classes ? This would be necessary to support dynamic properties.

Yes (all classes inherit from object anyway). What do you need this for for a protocol mixin? That should not define any properties afaict?

Regarding the Instrument write/read/ask/values methods, I would define them as proper method and not as bypass to the Adapter class.

I'm afraid I don't fully understand. To clarify what I'm proposing (maybe you're saying the same):

• Instrument.read/write/ask/values methods remain unchanged, handing off communication to self.adapter.read/write/...
• If your device family has communication quirks, you define a Mixin class that implements those quirks in appropriately defined read/write/ask/values methods (again, handing communication off to self.adapter). If you need, private attributes are fine (e.g. for tracking nr of retries), but afaik everything is simpler if Mixin.init does not need arguments to work correctly.
• Your device class now inherits from the Mixin, too, so that the Mixin methods replace the ones in Instrument (as they come earlier in the MRO): class ITC503(OxfordBehaviourMixin, Instrument)
• Adapter is unchanged, and it is now not relevant anymore if VISAAdapter or PrologixAdapter is used, as the behaviour changes are now defined in ITC503, not the adapter.

The main question that I cannot yet answer: Can this approach deal with all "behaviour quirks" that are currently present in the codebase (often implemented in custom Adapter subclasses). This needs some research in the codebase.

For ask method, I would be tempted to implement this in software as a combination of read and write (I think this is also @bilderbuchi opinion

Yes, this is in #409 (comment)

[bklebel]

EDIT: @bilderbuchi was faster.
my version of an answer: afaik Mixins can inherit anything and everything which can be inherited in python, and thus they can e.g. be derived from other classes. Generally speaking, in python, one can mess up the inheritance tree quite significantly, since one class can inherit from multiple classes which can each inherit from "whatever" - this is (I think) also a reason why @bilderbuchi mentioned that the Mixin should be named XYMixin, to keep things clean and readable (and e.g. discourage inheriting from a class which was intended as Mixin at a later point, without altering it significantly).
There is a clear definition of the MRO (method resolution order), which defines from which class in the inheritance tree a method/attribute should be used from by a certain instance of a class.

I do agree in general that instrument specific quirks in the communication feel a bit more on the side of the Instrument class from an abstraction/oop point of view, however, I would still want to keep the communication part well separated from the device control part, and would be a bit uncomfortable to squish read, write, ask and values fully into the Instrument class without them handing off communication to self.adapter.read/... as bilderbuchi mentioned.

[bilderbuchi]

Generally speaking, in python, one can mess up the inheritance tree quite significantly, since one class can inherit from multiple classes which can each inherit from "whatever"

On the contrary, I like the fact that in Python, doing diamond inheritance does not make your code go 💥 (looking at you, C++) 😁
I agree though that correct flow, with MRO and all, take a lot more care with multiple inheritance, and the footgun potential increases significantly. Proper use (and implementation) of super() becomes super (*cough*) important, so the review and checking effort for contributed code increases... which makes the "Adapter mixin" approach more attractive, as we can encapsulate these parts in Instrument and hide them from contributors, who only pass us a class bag of methods.

[bklebel]

Well, yes, I do like it too, I just tend to sometimes disregard good practice and wildly add functionalities via Mixins because it's so convenient - this can be dangerous, I like your term of "footgun potential" :D.

Now I am not sure which approach you named "adpater mixin".
I am going to try and list the mentioned approaches so we have clear names for it, please correct me if I misunderstood someone:
EDIT: added one more approach according to @msmttchr's comments below

Full Instrument Mixin/Inheritance
Read/write/ask/values being proper methods of Instrument, i.e. transferring much of what what the Adapter class does into Instrument, except from the bare minimum of defining a connection, address etc. In this approach, instrument specific quirks are handled close to the instrument's abstraction, either by mixing in different definitions of the read/write/ask/values methods, or by inheriting them. This would most likely work together with integrating the Prologix Adapter into the VISAAdapter (if that's feasible), and henceforth only having one adapter which is used for all types of connections, since it can handle them all.

Static/Dynamic Adapter Level Mixin class
Special communication behavior is defined in a mixin class, which is "mixed in" on the level of the Adapter.
Either statically defined custom adapters which inherit from the BehaviorMixin and the different Adapter classes are used, or they are dynamically generated in the Instrument.__init__ method, as @bilderbuchi described above:

super().__init__(args, behaviour_override=OxfordBehaviourMixin,...)

   if isinstance(adapter, Adapter):
       class CustomAdapter(behaviour_override, adapter):
           pass
   else:
       class CustomAdapter(behaviour_override, VISAAdapter):
           pass
   adapter = CustomAdapter(adapter, **kwargs)

The code of the current adapters (VISAAdapter, PrologixAdapter etc) remains untouched, their methods might become overridden by a Mixin, if one is defined.
In this approach, instrument quirks are handled a bit closer at the communication layer.

Instrument Level Mixin class
Special communication behavior is defined in a mixin class, which is "mixed in" on the level of the Instrument.
The code of the current adapters remains untouched, and their methods are never overridden, instead the quirks are handled by the read/write/ask/values methods of the Instrument, which might be overridden by a Mixin. Like this, the instrument specific things are again closer at the implementation of individual instruments, however, whether this is feasible, and whether communication quirks can be handled on this level needs looking into.
In this case, we would have an instrument definition of for example class ITC503(OxfordBehaviourMixin, Instrument).

Instrument/Adapter Separated Mixin(s) (new on 2022/02/17)
This goes back to the original opening statement of this issue.
read and write commands are defined at the Adapter level, while the methods ask and values are generally moved to the Instrument level. Special communication behavior is defined in a BehaveMixin, which is mixed in at the Adapter level, as low-level communication, while ask and values are considered more high-level communication methods.

[bilderbuchi]

Now I am not sure which approach you named "adpater mixin".

sorry about that >.< Thank you for compiling a summary with named alternatives! With "adapter mixin" I meant "Static/Dynamic Adapter Level Mixin class"

[bklebel]

Separately from listing these approaches in an agnostic way (I hope), in this naming, I prefer the Dynamic Adapter level Mixin. Changes to the codebase and the structure of the classes Adapter and Instrument would be minimal, and only contributors who want to implement a device with quirks would really need to see what is going on, with the respective behavior defined in classes which are then only inherited by the concrete instrument's implementation (i.e. the class Instrument), and the dynamic mixin does not confuse someone who reads up on the code of a certain instrument.
At the same time, the fact that such a mixin is used should probably be noted in the docs of the respective instrument implementation, so that someone who tried to develop their own device driver, found the instrument to behave erratically and then stumbled over pymeasure, would easily find where their problem was (possibly) solved within pymeasure.
Also, what I think pertains to the communication (and only the communication, of all commands, regardless of their use/function) would be kept close to the communication layer, while at the same time a user will perceive it as being put in on the level of the Instrument. This would happen by using what @bilderbuchi suggested
super().__init__(args, behaviour_override=OxfordBehaviourMixin,...), where the BehaviourMixin is passed as an argument to super() in the device's Instrument implementation, so that the actual mixin process to the Adapter class happens under the hood of the Instrument class.

[msmttchr]

Regarding the Instrument write/read/ask/values methods, I would define them as proper method and not as bypass to the Adapter class.

I'm afraid I don't fully understand. To clarify what I'm proposing (maybe you're saying the same):

Actually, I meant that ask and values are methods in the Instrument class and write and read are provided by the mixin class for the supported hardware interfaces.

[msmttchr]

My gut feeling is that last draft proposal from @bilderbuchi is preferred.

Sorry for not numbering my alternatives. I'm assuming you mean this approach "put the mixin on the Instrument subclass". Correct?

Yes, that is correct.

[bklebel]

I have added another approach, according to the last two comments by @msmttchr. I have to admit, I am now not sure I understand what you prefer in total, and I am not sure the added approach represents what you intend. If I understand you correctly, the initial idea of this issue was to move the ask and values methods from the Adapter level to the Instrument level, since they are not really "low-level" communication methods. Then, this discussion has broadened, with the problem of custom adapters being derived mostly from the VISAAdapter, while this could "shut out" users of different, non-visa Hardware Adapters, and that this means that one is implementing Instrument-specific behavior at the Adpater level, while one might want to keep this on the Instrument level. One reason for keeping this quirk-behavior at the Instrument level was that in #559 this was intuitively handled at this level, which then produced additional problems.

So, I think we can separate some things here, although I now might not include all the topics discussed in the conversation:

• where should the methods ask and values be located, at the Instrument level or the Adapter level?
• How and where should quirks which are specific to an instrument/manufacturer be handled, at the Instrument level or the Adapter level?

As I understand the conversation now, we agree that instrument specific behavior would be best packed into a Mixin class, the question is on which level it should act.

Personally, I think that ask and values could well be on the Instrument level instead of the Adapter level, although a) for simplicity I would try not to have to change it, if we do not have a real need for it (don't know whether this could break something somewhere, I do not want to guess right now), and b) it seems good to me to keep the "communication layer" apart from the "command layer", and in my head the Instrument level is reserved for the "command layer" while the Adapter level handles the "communication layer".
I assume, for @msmttchr this abstraction is different, in the sense that you have separated the "Hardware Adapter specific tools" and the "Instrument/Device specific tools", so there are two pieces of hardware, and the Adapter pertains to specifics of on while the Instrument pertains to the other.
Did I understand you correctly now? (sorry for the lengthy text....)

[msmttchr]

@bklebel thanks a lot for your effort and energy to try to shed some light in this complex subject.

Did I understand you correctly now? (sorry for the lengthy text....)

I think your understanding is correct, but allow me to further explain my point of view based on the code review and the exchange with @bilderbuchi and other people. [Of course, there is a certain degree of assumption/simplification and opinion that can be challenged]

The strategy to manage instruments in PyMeasure was designed by using two main classes:

• Adapter created to abstract instrument interfaces, intended as an hardware adapter between the controller PC and the actual instrument. [Indeed the fact that values method is implemented at Adapter level seems to contradict this interpretation]
• Instrument created to abstract instruments and uses Adapter class instance to talk to the instrument

When an instrument was implemented using the Instrument class, the adapter needed to be dynamic due to the fact that, in most cases, it is not known which hardware adapter is connected to the instrument.
This need imposed to have the Instrument read/write method to be actual "dynamic" methods that calls the Adapter class methods. I think this was done to serve two purposes:

1. To make access to read and write more easily in the Instrument commands implementation (I mean avoid to do self.adapter.read/write)
2. To allow user to access read and write methods to control the instrument at low level without mentioning the adapter, if needed.

This design choice had some implications:

• Customizing write/read method at the Instrument class level is not possible or it does not make any sense. Indeed if you change the 'write' method at Instrument class level, your changes are not used by all the other method implemented at Adapter level (e.g. ask).
• If you forbid to customize read/write method at Instrument level, then if customization is needed, the only remaining choice is at Adapter level

If you implement some part of the instrument behavior at Adapter level, then you would lose the ability to replace the adapter part with a class of your own designed for a specific hardware adapter, because some part of the instrument behavior is implemented in the Adapter.

Let's make an example, my instrument has a GPIB interface and I want to use the following hardware adapters to interface it:

• GPIB-USB-HS from NI
• GPIB-USB from Keysight
• GPIB from Prologix
• GPIB i/f from Arduino project

First of of all I need to make sure that there is an Adapter subclass for each of them and if it does not exist I can implement it by subclassing Adapter class.

Now let's assume, that my instrument has a bug/feature that requires a delay of at least 100 ms after each write command.
Now this can be considered a protocol quirk, but for sure it does not have anything to do with the adapter classes discussed above, but it is a feature of this specific instrument, so this would lead me to conclude that I need to implement it at Instrument class level.

If I do it at Instrument class level, on the other hand, I have to amend the write, but also ask, values and binary_values: not very easy to do and very error prone.

My conclusion is that there is a degree of fuzziness and interpretation on Adapter class and Instrument class roles.

My personal preference is that everything related to the instrument behavior (including protocol quirk or other stuff) is static and should be implemented at Instrument class level.
Everything related to the hardware interfaces adapters should be implemented in such a way that can be "dynamically" passed to the Instrument class.

Finally, I think your point of separating command part from communication part is worth discussing as a design choice for the instrument implementation, but in my opinion could be decided after the roles of Adapter and Instrument is defined.
[Sorry for the long post]

[BenediktBurger]

My two cents as someone new to pymeasure:
I understand, that adapter is the physical communication with the device (e.g. a wrapper around pyvisa like VISAadapter), while the Instrument contains everything which is specific to the instrument, including protocol quirks.

For the separation of communication and commands, there could be an optional Protocol intermediary containing the communication part. This Protocol could also handle frame-based communication (see #296 ), as I find it difficult to reconcile my definition of adapter (which ensures sending chars or bytes) with the formation of the chars/bytes itself (the protocol).

[BenediktBurger]

I just looked at pyvisa's RegisterBasedResource. It is useful, if you can access the memory of the device itself, but you know how to do it right.
Some devices require messages, which indicate the register, but the register is not addressed directly (see TC038D).
I'd ignore the register based approach until some device needs it or benefits truly from it.

Instead we should focus on the VISAAdapter. For frame based communication read_bytes and write_raw (pyvisa names) is necessary. Until now, only read_bytes is present.
Both methods could be included in read/write as an optional parameter (like instrumentKit) or as their own methods. I prefer the latter, because if you want to get the raw data, you might need to get/send bytes, while the normal methods use strings.
Doing this, VISAAdapter.ask could be removed (in order to default to Adapter's write+read).

EDIT: The names of those methods could be read_bytes, write_bytes and they could be present in all adapters.

[BenediktBurger]

One protocol quirk, which should be moved from the Adapter, is the "preprocess_reply", as it has nothing to do with the Adapter itself, more with the message composition (aka protocol).

Maybe even the whole "values" method should be moved, wherever the protocol is: to the Instrument or another class which is between Adapter and Instrument.

An argument for "values" in "Adapter" would be, that you could use the adapter's methods like pyvisa's query_ascii_values, but VISAAdapter.values does not use it either, probably exactly because you cannot preprocess the response.
If values remains in Adapter, self._preprocess_reply should be a protocol variable and the protocol calls Adapter.variables(preprocess_reply=self.preprocess_reply).

EDIT:
Talking more about tests and protocol, I think it is best, to move values from Adapter to Instrument. That way, values accesses the concrete instrument's read/write methods and these two methods could contain all the protocol quirks of the instrument. Things like command process, preprocess_reply etc can be easily defined there and everybody understands, how it works.

[BenediktBurger]

My proposal is:
All communication runs through the instrument's read()/write() method, which encapsulate everything special about reading or writing of the instrument. Only those two methods (at least of the generic Instrument) invoke the adapter directly, lest any default method bypasses these two methods.

This provides a single point (for each direction) of writing things and don't create confusion, why the pymeasure properties do not work, even though you change both read and write.

All other methods (ask, values, etc.) are implemented either in the base Instrument or overridden in the concrete instrument.

EDIT
TODOs from #634 :

• Remove ask from different Adapters - implement as write+read in Adapter
• Add protocol tests for an instrument with a custom Adapter, refactor that (probably)
• Search for any custom Adapters and refactor those
• logging: Roll out message-traffic logging to all Adapters, try to DRY
• Maybe(?) we can do the CMND/RESP pair in Instrument.write/read (fget/fset?) that I mentioned here immediately, and postpone the SEND/RECV (as close to the wire traffic as feasible) on Adapter/connection level for later?
• Logging timing: Also postpone until we implement the adapter logging. There is a remark in IK about this and a separate debug flag that I'm not sure is necessary, I'd like to confirm my hunch then.

(list edited by @bilderbuchi )