Add read until terminator support for ip instrument #971

jenshnielsen · 2018-02-19T09:40:17Z

Hopefully fix stability issues that we have been seeing with the AMI 430 magnet

The idea is to ensure that we always recv until we get read termination char. The current implementation assumes that one recv is always enough to get all the data. That is probably true in almost all cases but might be wrong under heavily congested network traffic.

Still need to check the Terminator for the Mercury. Judging from the manual there may not be one.
Currently it is set to None

sohailc · 2018-02-19T11:44:04Z

qcodes/instrument/ip.py

+            if partresult == b'':
+                log.warning("Got empty response from Socket recv() "
+                            "Connection broken.")
+            elif (partresult[-rtl:] == self.read_terminator or


I would use "endswith" here:
https://www.tutorialspoint.com/python/string_endswith.htm

Agreed that is slightly more clean

sohailc · 2018-02-19T11:50:17Z

Should we not make the read_terminator default to terminator? I think we should always call recv until the read terminator has been seen.

jenshnielsen · 2018-02-19T12:59:06Z

I agree. I wanted to test with other instruments before doing that but just doing a single recv without any checks is fundamentally wrong

codecov · 2018-02-19T12:59:11Z

Codecov Report

Merging #971 (a6219b7) into master (18e9d4e) will increase coverage by 1.60%.
The diff coverage is 16.00%.

❗ Current head a6219b7 differs from pull request most recent head 5df574b. Consider uploading reports for the commit 5df574b to get more accurate results

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
+ Coverage   65.56%   67.16%   +1.60%     
==========================================
  Files         227      145      -82     
  Lines       30626    17972   -12654     
==========================================
- Hits        20080    12071    -8009     
+ Misses      10546     5901    -4645

sohailc · 2018-02-19T13:17:36Z

Other things I am concerned about:

How do we prevent getting stuck in an infinite loop? Maybe we should have a timeout?
How do we know the terminator string is at the end of the string we get from recv?

Maybe the following link can help us: http://code.activestate.com/recipes/408859-socketrecv-three-ways-to-turn-it-into-recvall/

jenshnielsen · 2018-02-19T13:24:30Z

We already set a timeout so the first call that times out will raise a TimeoutError. We should probably catch and handle that better.
As long as we send commands one by one I think we should always get a buffer that ends with a terminator but if we send more than one command before reading then you are right it can be anywhere in the stream.

sohailc · 2018-02-19T13:38:05Z

I think we should be careful and not assume we are reading before another command is send. As is done in the example code I linked to:

We should see if the terminator marker is in the string
If the termination marker is more then one character, it can be broken up in two read actions.
If the termination marker is half way down the string, what do we do with the part of the string after the marker? Its part of a response to another querry so it should be stored in a buffer somewhere and appended to the string of the next recv command.

All of this will complicate our code dramaticaly so I am wondering if we really should do all of this. I am leaning towards a "yes" though as it will make our code more robust.

MatthieuDartiailh · 2018-02-19T13:46:18Z

This kind of issues is also addressed in pyvisa-py (tcpip-socket backend). You may be interested in giving it a look.

jenshnielsen · 2018-02-19T13:50:07Z

In general we should strongly consider if we really need this IP class. I would expect we can and I think we should replace both the magnet drivers with visa drivers that connects to visa over sockets.

sohailc · 2018-02-19T13:53:47Z

I agree! We do not want to duplicate efforts

jenshnielsen · 2018-02-20T08:52:53Z

This now adds a comment suggesting to use sockets over visa.
Takes into account that the EOL may be split over more than one recv
Clips the EOL and any residual text from the recv string
Warns if any text follows the EOL. If that is the case we are doing something wrong

jenshnielsen · 2018-06-20T08:17:41Z

@WilliamHPNielsen would be useful to look at this in the context of the mercury

WilliamHPNielsen · 2018-06-20T09:23:18Z

I'll test it with the old (current) IPInstrument MercuryiPS driver.

WilliamHPNielsen

From what I could see playing around with the MercuryiPS, this works well.

WilliamHPNielsen · 2018-06-25T13:45:35Z

@sohailc, do you see any reason to not merge this guy?

sohailc · 2018-06-25T13:51:06Z

Well I still don't like the way partial messages are treated. I think there is a more robust way to do this and one which does not throw away anything. Here is an example of how I would do this: https://gist.github.com/sohailc/a0fcbfff5359f5fc4cc1ad9f2e0707ba

jenshnielsen · 2018-06-25T14:20:49Z

@sohailc imho this is of little consequence. If we ever read more than one reply it is likely due to a non empty output buffer even before whatever write command we did. Most likely the output of the last recv is the answer that you actually want. In principle we need to prevent this from ever happening as the reply is likely to be wrong no matter what.

Honestly perhaps we should just raise and close the connection in that case.

sohailc · 2018-06-25T14:49:46Z

Jens wrote: "If we ever read more than one reply it is likely due to a non empty output buffer even before whatever write command we did."

I'm sorry Jens, but I just don't agree. I think we need to write our code and flexible and general as possible and not include any assumptions like the one you are making. Keep in mind that this modification will not only affect the MercuryiPS driver but all IPInstruments. If we receive jibberish from the Mercury power controller the proper location to raise and close the connection is in the instrument driver, not in the base IPInstrument class which I think we should program as general as possible.

I you guys disagree feel free to merge this PR, but it is just not the way I would do things.

jenshnielsen · 2018-06-25T15:08:56Z

Ok Sohail I will be more explicit. I think your solution will result in wrong results in all relevant cases for all our IP Instruments. I have identified a real use case where it will result in incorrect results. This applies to all IPInstruments and not specifically the mercury. I cannot see any use cases where it does anything useful. If you can identify such a use case I am happy to reconsider

Try looking at the _recv function and how it is used. It is only used after a send command. Each send command must result in a single reply or no reply. If it does not the whole class is fundamentally broken.

sohailc · 2018-06-25T15:26:09Z

I have not tested my particular implementation with any instruments. I was trying to explain that there should be more robust ways of dealing with instrument replies. I am sure you are right that my solution will result in wrong results, but can you share your reasoning? Which use cases have you identified?

As for your question of when my solution would be useful, what about if a reply to a _recv command is so long that an instrument sends it in two or more chunks? We have never seen that but we cannot altogether rule it out, especially for older instruments.

sohailc · 2018-06-25T15:42:10Z

You know what, I think you are right. The case that a reply is send in two chucks is already taken care of in this PR. I am approving this PR

sohailc · 2018-02-19T11:47:55Z

qcodes/instrument/ip.py

@@ -183,7 +215,7 @@ def ask_raw(self, cmd):
    def __del__(self):
        self.close()

-    def snapshot_base(self, update=False):
+    def snapshot_base(self, update=False, params_to_skip_update=None):


Please add this arg in the docstring with a description what "params_to_skip_update" means

sohailc · 2018-02-21T14:33:22Z

qcodes/instrument/ip.py

@@ -25,6 +29,11 @@ class IPInstrument(Instrument):

        terminator (str): Character(s) to terminate each send. Default '\n'.

+        read_terminator: Character(s) to look for at the end of each read.


I wonder if the read_terminator should not default to terminator

sohailc · 2018-02-21T15:57:13Z

qcodes/instrument/ip.py

+            rtloc = result.find(self.read_terminator)
+            if rtloc >= 0:
+                if rtloc + rtl < len(result):
+                    log.warning("Multiple (partial) results received. "


Maybe we can have a look at this to see how we can handle receiving multiple partial messages: https://gist.github.com/sohailc/1a5db7bb260f36b94b552e630ea844f6

A little better:
https://gist.github.com/sohailc/a0fcbfff5359f5fc4cc1ad9f2e0707ba

jenshnielsen · 2018-06-25T15:54:18Z

Here is a real world example for an instrument with two parameters volt and curr

The output buffer on the instrument contains a reply to a command that has never been read i.e. "1.0e1V\n". This is not read due to a crash at an earlier run.
They you ask the instrument CURR?
The _recv will then get"1.0e1V\n" or perhaps "1.0e1V\n5.0A\nfollowing the loop construct. In the first case we will get the wrong result in any case with both solutions. In the second case you will return1.0e1Vand keep5.0A` in the internal buffer and return that on the next command.

It's not about 2 or more chunks that's already handled correctly we are in any case reading until a termination char in this PR. It is only about two or more termination characters within one message. And as far as I can see your solution will still not handle that hypothetical situation correctly. If a command to the instrument returns two partial results separated by a newline char the ask command will still only return the first part and the next one will be left in the internal buffer of the Instrument for the next ask command giving an incorrect result here.

jenshnielsen · 2018-06-25T15:55:53Z

@sohailc Just saw your message glad that we understand each other. This still need real world testing before merging can be considdered. And I think we should look into better handling of Interrupts within the _recv command. I suspect these are a source on the kind of inconsistency that I talk about

jenshnielsen · 2018-06-25T16:10:05Z

Actually my solution does not handle it any better. I stand by what I said earlier. I think we should raise.
Quoting the zen of Python In the face of ambiguity, refuse the temptation to guess.

sohailc · 2018-06-25T16:16:18Z

Also, I am not raising issues dealing with multiprocessing/threading, but technically I think our solution might be thread unsafe (right?). I am not objecting because for now QCoDeS is single process anyway. But what if two processes call _recv and the internal instrument buffer contains two responses?

I suppose we can deal with these kind of issues when we implement multiprocessing in the weeks/months to come?

jenshnielsen · 2018-06-25T16:32:07Z

I suspect that no instrument is thread safe see #621 for an example of what can happen in a Visa instrument. We should have a good system, locks or otherwise as part of a multiprocessing architecture to ensure that an instrument can only be called from one thread

WilliamHPNielsen · 2018-06-26T07:44:24Z

@jenshnielsen said:

This still needs real world testing before merging can be considered.

As you saw, I did some preliminary testing (basically: it doesn't crash upon init, and commands are sent and read). I'll be happy to try and reproduce an actual fail case and see how the code lives up. What would you consider sufficient testing with an actual instrument?

jenshnielsen · 2018-06-26T07:57:37Z

The one thing that i suspect still needs improving is the behavior when doing interrupts in the middle of get and set commands.

I think it's hard to reproduce the issue that this aims to fix in a real world scenario. It is so far pure speculation that a recv may only return a partial command. (But according to the specs it is possible) But I suspect this may happen with high volumes of network traffic perhaps coupled with high cpu utilization

jenshnielsen · 2022-11-15T13:21:13Z

We are generally moving away from the ip instrument so lets drop this

jenshnielsen added 2 commits February 19, 2018 10:00

Add read until terminator support for ip instrument

2600632

use the right read terminator for ami 430

0ea8dd7

sohailc self-assigned this Feb 19, 2018

jenshnielsen assigned jenshnielsen and unassigned sohailc Feb 19, 2018

sohailc reviewed Feb 19, 2018

View reviewed changes

Merge branch 'master' into ip_recv_eol

e68840c

jenshnielsen added 2 commits February 20, 2018 09:49

Try to make ip recv more robust

71cf4e1

Add note suggesting to use VISA over IP instrument

24da263

Merge branch 'master' into ip_recv_eol

87bbd6b

Merge branch 'master' into ip_recv_eol

ac12675

WilliamHPNielsen approved these changes Jun 25, 2018

View reviewed changes

sohailc approved these changes Jun 25, 2018

View reviewed changes

astafan8 and others added 2 commits September 14, 2019 03:48

Merge branch 'master' into ip_recv_eol

a6219b7

Merge branch 'master' into ip_recv_eol

5df574b

jenshnielsen closed this Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add read until terminator support for ip instrument #971

Add read until terminator support for ip instrument #971

jenshnielsen commented Feb 19, 2018

sohailc Feb 19, 2018

jenshnielsen Feb 19, 2018

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

codecov bot commented Feb 19, 2018 •

edited

Loading

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

sohailc commented Feb 19, 2018

MatthieuDartiailh commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 20, 2018

jenshnielsen commented Jun 20, 2018

WilliamHPNielsen commented Jun 20, 2018

WilliamHPNielsen left a comment

WilliamHPNielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

sohailc commented Jun 25, 2018

sohailc Feb 19, 2018

sohailc Feb 21, 2018

sohailc Feb 21, 2018

sohailc Feb 21, 2018

jenshnielsen commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018 •

edited

Loading

jenshnielsen commented Jun 25, 2018

WilliamHPNielsen commented Jun 26, 2018

jenshnielsen commented Jun 26, 2018

jenshnielsen commented Nov 15, 2022

		@@ -25,6 +29,11 @@ class IPInstrument(Instrument):

		terminator (str): Character(s) to terminate each send. Default '\n'.

		read_terminator: Character(s) to look for at the end of each read.

Add read until terminator support for ip instrument #971

Add read until terminator support for ip instrument #971

Conversation

jenshnielsen commented Feb 19, 2018

sohailc Feb 19, 2018

Choose a reason for hiding this comment

jenshnielsen Feb 19, 2018

Choose a reason for hiding this comment

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

codecov bot commented Feb 19, 2018 • edited Loading

Codecov Report

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

sohailc commented Feb 19, 2018

MatthieuDartiailh commented Feb 19, 2018

jenshnielsen commented Feb 19, 2018

sohailc commented Feb 19, 2018

jenshnielsen commented Feb 20, 2018

jenshnielsen commented Jun 20, 2018

WilliamHPNielsen commented Jun 20, 2018

WilliamHPNielsen left a comment

Choose a reason for hiding this comment

WilliamHPNielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018

sohailc commented Jun 25, 2018

sohailc Feb 19, 2018

Choose a reason for hiding this comment

sohailc Feb 21, 2018

Choose a reason for hiding this comment

sohailc Feb 21, 2018

Choose a reason for hiding this comment

sohailc Feb 21, 2018

Choose a reason for hiding this comment

jenshnielsen commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

jenshnielsen commented Jun 25, 2018

sohailc commented Jun 25, 2018 • edited Loading

jenshnielsen commented Jun 25, 2018

WilliamHPNielsen commented Jun 26, 2018

jenshnielsen commented Jun 26, 2018

jenshnielsen commented Nov 15, 2022

codecov bot commented Feb 19, 2018 •

edited

Loading

sohailc commented Jun 25, 2018 •

edited

Loading