Adapter rework

[BenediktBurger]

Reworking the instrument/adapter. This pull request just focuses on the official adapters, Instrument class and keeping the code base in a working condition.

Fixes #567, fixes #512, fixes #409, fixes #219.

Roadmap of different parts (PRs)

1. Preliminary tests of instruments/adapters in the current state Instruments and adapter protocol tests for adapter rework #665
2. This pull request with changes to the official adapters and minimum changes to the instruments to use these
3. A test code generator.
4. Pull requests for changes in individual instruments (removing custom adapters, where applicable)

Open questions

• move read/write binary_values to adapter or instrument?
• query_delay in instrument.ask or should the instrument replace ask adding its own delay method if necessary?
• remove FakeAdapter or is there any use for it, having implemented the Protocol adapter?
• Deprecate the vxi11 adapter in favour of pyvisa?
• make SerialAdapter similar to pyvisa: timeout in ms? Or both in s?

Adapter rework

General

• Write tests for instruments to be changed. (tests using devices are called test_instrument_with_device.py : add appropriate comment into documentation)
• Harmonise ask being write+read (i.e., remove from VISAAdapter), use protocol tests to confirm it works correctly
• Implement waiting in Instrument.ask. time.sleep(0) takes 0.5 µs.
• Remove all now-unnecessary ask() implementations, read and write implementations should have all (test this)
• Implement over-the-wire message-traffic logging here (protocol.py), first
• Check timing impact of non-firing logging messages: less then 0.5 µs
• Roll out message-traffic logging to all Adapters, try to DRY
• adjust tests if instruments got changed (especially those with their own adapter.
• check newly added instruments
• Include a test generator which generates tests out of device communication
• Adjust the documentation: All protocol quirks are in read/write. Timing issues in ask (defaults to write + wait + read). Remove the "custom adapters in init" examples from the "connecting.rst". It's better to adjust read than to write your own adapter. If you need a special adapter, check, whether an adapter had been handed over first (for example for protocol_tests).
• Document how Adapters work and why only the the private methods should be overridden.
• Add a remark in changes.rst

Official adapters

• prologix adapter: remove rw_delay
• Adapter: Add adapter.close
• Can pyvisa replace telnet or change to another option (telnetlib3...) as telnetlib is deprecated PEP 594
• Can Prologix be based on pyvisa?
• deprecate vxi11 in favour of pyvisa?

Instruments and special adapters

Special Adapters ensure, that they work with the modified adapters

• attocube and anc300
• danfysik and danfysik8500: should be removable, verify, that it works as expected.
• lakeshore: could be moved to pyvisa, I guess
• Oxford Adapter: move it to its own superclass between Instrument and the specific instrument itself. (later)
• Toptica (later)

Test special adapters change

• attocube
• danfysik
• lakeshore

Rewrite instruments communication (read/write...) to adapt to these changes

• anaheim.dpseries
• fwbell.fwbell5080 move to normal instrument, implementation itself is broken
• hcp.TC038
• hp.hp8116a uses read_stb, assert_trigger, clear...
• oxfordinstruments: see its own adapter
• parker.parkerGV6 move to normal instrument (using pyvisa as default), see also Cant open Parker connection #620 and the following issue Fix Parker GV6 instrument implementation #623
• activetechnologies/AWG401x rewrite write_binary_values

Verify rewritten instruments

• anaheim.dpseries
• fwbell.fwbell5080
• hcp.TC038
• hp.hp8116a
• oxfordinstruments: see its own adapter
• parker.parkerGV6
• activetechnologies/AWG401x

Small adjustments (change adapter calls to instrument calls)

• agilent/agilentB1500
• anritsu/anritsuMS9710C
• heidenhain/nd287
• anritsu/anritsuMS9710C
• hcp.TC038D
• keysight.keysightN7776C
• lakeshore.lakeshore421
• srs.sr860
• hp.hplegacyinstrument
• hp.hpsystempsu
• hp.hp3437A

Verify adjusted instruments

• agilent/agilentB1500
• anritsu/anritsuMS9710C
• heidenhain/nd287
• anritsu/anritsuMS9710C
• hcp.TC038D
• keysight.keysightN7776C
• lakeshore.lakeshore421
• srs.sr860
• hp.hplegacyinstrument
• hp.hpsystempsu
• hp.hp3437A

Check newly added / modified instruments

Clean up

• remove notes from protocol.py, adapter.py
• clean commented code from instruments transitioned to the new style

Issues and discussions

• Adapter and Instrument methods #567 Main issue
• Restructure Adapter communication methods #512 contains more connected issues
• Unify adapter classes #409
• Thread safe communication #506 Regarding thread-safety and its difficulties. Questionable whether it should be included here
• Logging traffic  #219

[bilderbuchi]

You're crazy! 👏 😁

[bilderbuchi]

I hope you have not bitten off too much here, a ultralarge PR could make reviewing quite difficult.
If you have implementation/other questions, please pipe up asap for feedback!
If you need an in-between review, or if there is a logical place where this is "complete", we can also postpone the rest of the changes to a follow-up PR. It's often easier to review 2 O(N) PRs instead of of one O(2*N).

[bilderbuchi]

re: your questions:

move read/write binary_values to adapter or instrument?

I think the read_binary and write_binary should be where read and write are. Not sure about the _values methods.

query_delay in instrument.ask or should the instrument replace ask adding its own delay method if necessary?

Not a strong opinion, but replacing base class methods (without calling them, i.e. not just wrapping them) always feels a bit suboptimal/smelly to me -- there could be additional treatments/logging/housekeeping that you would miss in this way.
As you need the query_delay between the write and the read that the ask does, it's impossible to wrap Instrument.ask in a subclass implementation.
Therefore, I think it would be preferable to implement this with an argument to Instrument.ask. A subclass could then do

def ask(self, command):
    super().ask(command, query_delay=0.1)

If that is not good for some reason, we could also use a class attribute that Instrument.ask uses if not None (but I'm hesitant to strew random attributes around, especially because users define a lot of properties on our classes)

ping @msmttchr @CasperSchippers though, I remember we discussed this before.

remove FakeAdapter or is there any use for it, having implemented the Protocol adapter?

It's used in some tests, and in SwissArmyFake and FakeInstrument, but maybe could be pulled into the latter as an internal class if the tests can be restructured.

[bilderbuchi]

re:

handle testing several times the same property/setter but with different values: keep the same name (what does pytest do?) or numerate them? Possibility to give a list of values to test for a parametrized test?

This is what pytest parametrization is made for.

[bilderbuchi]

#570 (nearly ready for merge) will also be interesting to consider for the rework -- custom read, overridden ask, prologix adapter. And it has tests (for connected device only). :-)

[BenediktBurger]

Thanks for your comments. It's still a work in progress, but I wanted to get some ideas (mainly those in the questions section), therefore I opened the pull request.
I might split the PR into several.

1. The first will be the tests, as they have to pass before the adapter rework.
2. The actual adapter rework and adjusting the individual instruments have to be done simultaneously (or into another branch), as one part will break the other one.
3. Last, but not least, the Test Generator, which requires the Adapter Logging. I'm sorry for adding it into this PR

The Test Generator, in turn defines how the Adapter have to do the logging. So, in the end, everything (besides the first tests) hang together.

handle testing several times the same property/setter but with different values: keep the same name (what does pytest do?) or numerate them? Possibility to give a list of values to test for a parametrized test?

This is what pytest parametrization is made for.

I know. It will make things a lot more complicated, if the test generator automatically notices, that it tested several times the same property and generates a parametrized test instead of individual test.

Right now the generator writes every single completed test to the output file and does not aggregate tests into one.

Let's keep the Test generator simple for this first PR, just, that it works together with the Adapters.

Regarding Deprecation Warning

Thank you for your suggestions and comments. I have not yet used those, therefore I appreciate your comments, in the meanwhile I marked somehow the deprecated parts. For that reason, the messages are not yet very descriptive (it's a draft). I focused on the implementation itself.

#570 (nearly ready for merge) will also be interesting to consider for the rework -- custom read, overridden ask, prologix adapter. And it has tests (for connected device only). :-)

One point is "rework new instruments", as I have to check those as well, which came after protocol-tests branched off master. They won't be forgotten.

[bilderbuchi]

All good, I understand that it's a draft, I just wanted to give some early feedback

Thanks for your comments. It's still a work in progress, but I wanted to get some ideas (mainly those in the questions section), therefore I opened the pull request. I might split the PR into several.

1. The first will be the tests, as they have to pass before the adapter rework.

2. The actual adapter rework and adjusting the individual instruments have to be done simultaneously (or into another branch), as one part will break the other one.

3. Last, but not least, the Test Generator, which requires the Adapter Logging. I'm sorry for adding it into this PR

The Test Generator, in turn defines how the Adapter have to do the logging. So, in the end, everything (besides the first tests) hang together.

Splitting along those lines sounds good! (and your current groundwork will already inform those different parts).
Let's see after the "dust has settled" and the content has converged somewhat if it's too large to make sense of in one go, or you split it. maybe the separation along the different files is enough.

One point is "rework new instruments", as I have to check those as well, which came after protocol-tests branched off master. They won't be forgotten.

👍

[BenediktBurger]

The official adapters and the Instrument class are ready for review.
Let us review the tests (mostly done), documentation (totally missing) and all the other instruments adjusted to these changes after we have finished the adapters/Instrument design. Reworking existing adapters proper to specific instruments and the Test-Generator will be separate Pull-Requests afterwards.

One question remains: What do we do with the read/write_binary_values?

• write_binary_values is implemented in Serial and Prologix adapters. I'd keep it there, as it is adapter specific.
• binary_values (reading them) is implemented in visa and serial adapters in basically the same way (based on write and read), we could move it to Instrument.

Some design thoughts I implemented to better understand the code:

• Only the Adapter's read/write (and their bytes companions) should be called. Everything else resides in the instrument.
• Instrument.values calls Instrument.ask, which in turn calls Instrument.write and Instrument.read with a possible delay in between. That way all read/write logic (adding stuff to written messages, extracting data from read ones) resides in read, write.
• The Adapter's read/write methods have kwargs. That way you can call for example pyvisas read_bytes with the option to stop at the term_char.
• I made the serial and telnet adapters similar to pyvisa in the sense, that they can have term_chars defined (not done on default), because some instruments implemented their own term_char logic for a SerialAdapter subclass. Due to this similarity, you can use SerialAdapter and VISAAdapter interchangeably (without considering the different timeout units of ms and s).

Thank you @bilderbuchi for your many reviews and tipps in the last pull requests. I learned a lot and produced better codes with your help.

[bilderbuchi]

The official adapters and the Instrument class are ready for review.

Great, I'll try to do this bit-by-bit, hopefully over the coming week :-)
@msmttchr it would be good to get your feedback in (especially on the prologix section, which I know is close to your heart)!
Do you also want to take a look at instrument.py and the adapters/files now, or wait until I did one pass?

Let us review the tests (mostly done), documentation (totally missing) and all the other instruments adjusted to these changes after we have finished the adapters/Instrument design.

Sounds good!

Reworking existing adapters proper to specific instruments and the Test-Generator will be separate Pull-Requests afterwards.

Sounds good!

[bilderbuchi]

One question remains: What do we do with the read/write_binary_values?

* write_binary_values is implemented in Serial and Prologix adapters. I'd keep it there, as it is adapter specific.

* binary_values (reading them) is implemented in visa and serial adapters in basically the same way (based on write and read), we could move it to Instrument.

I don't know right now, and I don't have strong intuition about it (yet), especially the *values methods. As long as we're restructuring, we should take the opportunity to make/keep the method pairs consistent in naming and abstraction level/location.

If write_binary_values resides in Adapter, read_binary_values should, too (not talking about forwarding/wrapping calls in Instrument).
Moving values() out of Adapter, but keeping [read_]binary_values() would feel weird off the cuff.

Some design thoughts I implemented to better understand the code:

* Only the Adapter's read/write (and their bytes companions) should be called. Everything else resides in the instrument.

Sounds good in principle -- separate the communication details from the "orchestration"/coordination of reads/writes.
I'm wondering how this will mesh with VISAAdapter, which calls the pyvisa write_binary_values method. Can we just not use that (like we decided against the built-in ask/query) and do something else with write_binary in perfect equivalence?

* Instrument.values calls Instrument.ask, which in turn calls Instrument.write and Instrument.read with a possible delay in between. That way all read/write logic (adding stuff to written messages, extracting data from read ones) resides in read, write.

* The Adapter's read/write methods have kwargs. That way you can call for example pyvisas read_bytes with the option to stop at the term_char.

👍

* I made the serial and telnet adapters similar to pyvisa in the sense, that they can have term_chars defined (not done on default), because some instruments implemented their own term_char logic for a SerialAdapter subclass. Due to this similarity, you can use SerialAdapter and VISAAdapter interchangeably (without considering the different timeout units of ms and s).

Interchangeability is good! Maybe at one point we can even harmonise the timeout units. :D

Thank you @bilderbuchi for your many reviews and tipps in the last pull requests. I learned a lot and produced better codes with your help.

Sure! 😃 I think the work pays off because we get higher-quality future contributions, so reviewing will be less effort!

[bilderbuchi]

I don't want to pull this into this PR to avoid increasing the already large scope even more, but if you have the time take a look how the device in #239 does frame-based communication.
It would be great to (easily) accommodate this kind of communication method in some future PR. I'm hoping that if you keep this in the back of your head this will nudge the current rework in an amenable direction. :-)

[BenediktBurger]

Note: I only looked at the files in the latest commit, I'm assuming you haven't changed anything materially in the previous ones during the force-push?

The force push before and the one now contain only a rebase onto the updated master. The last commit contains changes to the documentation and one for the Instrument, according to your comments.

[bilderbuchi]

I think we're approaching the home stretch! I'm not sure if all the "44 unresolved conversations" shown by the merge dialog are true, or if the PR machinery got confused somehow by the rebasing?

My remaining conversations/remarks should be straightforward to deal with.

[BenediktBurger]

I did not get a notification about your remarks 17 days ago...
Right now, your new comments showed me the older ones as well, which I have to address as well. Sorry for that.

[BenediktBurger]

@bilderbuchi I implemented the change requests in the last two commits. I resolved your lauding comments (great, awesome etc.) as I guess you do not need to look at them again.

[bilderbuchi]

I resolved your lauding comments (great, awesome etc.) as I guess you do not need to look at them again.

Sure! I wanted to make sure that you realise that we really appreciate this huge effort, but yes, I don't need the comments any more. :-)

[BenediktBurger]

I verified, that even without a regular output, the testcode directive works fine with expected_protocol: If I change the asserted values, the sphinx doctests fail.
I really like expected_protocol. It's so helpful finding bugs (the examples had some in the beginning).

[bilderbuchi]

I verified, that even without a regular output, the testcode directive works fine with expected_protocol: If I change the asserted values, the sphinx doctests fail. I really like expected_protocol. It's so helpful finding bugs (the examples had some in the beginning).

Perfect!

[bilderbuchi]

So, I guess now you have to go back to the top and check which boxes have been completed in the meantime? :-)

[BenediktBurger]

I verified the instruments' changes. With that, we're good to merge.

[bilderbuchi]

Thank you. I think it's time to pull the trigger to put this huge PR to rest! 🎉
Huge props to you for starting this, staying with it and seeing it through!

[bilderbuchi]

@bmoneke we missed some test warnings that are generated (because they're warnings, not errors :D), do you think we can fix them quickly, or should I file a separate issue?

 =============================== warnings summary ===============================
tests/instruments/test_instrument.py::TestWaiting::test_binary_values_calls_wait
  /usr/share/miniconda/envs/pymeasure/lib/python3.9/site-packages/pymeasure/adapters/adapter.py:226: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead
    return np.fromstring(data, dtype=dtype, **kwargs)

tests/instruments/attocube/test_anc300.py::test_stepu
tests/instruments/attocube/test_anc300.py::test_voltage
  /usr/share/miniconda/envs/pymeasure/lib/python3.9/site-packages/pymeasure/instruments/attocube/adapters.py:113: FutureWarning: Do not call `Adapter.ask`, but `Instrument.ask` instead.
    warn("Do not call `Adapter.ask`, but `Instrument.ask` instead.",

[BenediktBurger]

The first issue I already knew. For that reason I had wanted to rewrite read_binary_values, but @msmttchr wanted the old method. To keep it simple, I just copied it. I'll open an extra issue about it, because it is more than a simple fix.

The second issue is a tiny fix in the test (test_anc300.py) itself: The MockAdapter still uses self.ask (raising the warning), while the real adapter uses write and read (test_anc300.py line 56):

-        _ = self.ask('echo off')  # TODO
+        self.write('echo off')
+        _ = self.read()

[BenediktBurger]

I can do a pull request if you do not want to change just that line directly to master, @bilderbuchi .

[bilderbuchi]

It's fine, I just pushed it. Thanks!