Fixed bug in USBTMC code which caused issues when packets were longer than max usb packet size #449
Conversation
…BTMC class to correctly handle messages longer than the usb packet size. Created USBIntstrSession read to handle case where an incomplete message is read (i.e., before a term character is reached)
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #449 +/- ##
==========================================
- Coverage 24.48% 24.47% -0.01%
==========================================
Files 23 23
Lines 3488 3485 -3
Branches 485 480 -5
==========================================
- Hits 854 853 -1
- Misses 2617 2626 +9
+ Partials 17 6 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi — thank you for the PR! Would you mind providing some context regarding what prompted you to make the change? Any specific instrument/command pattern that were having issues and are fixed by this? This information might help with revisiting existing USBTMC-related issues. Thanks! |
I am working with a Siglent SDG1062X function generator. I tried querying the waveform parameters using 'C1:BSWV?' (see https://siglentna.com/USA_website_2014/Documents/Program_Material/SDG_ProgrammingGuide_PG_E03B.pdf section 3.4), but I was getting issues where I would only get a partial response back (first 52 bytes, which is one usb packet without the 12 byte BulkInMessage header), and future queries would return garbage (the remaining packets partially misinterpreted as part of the BulkInMessage header information). The code in this PR fixes the issue for me, but I haven't been able to test it on other devices, so I'm not sure if part of this is a quirk of this particular device. |
|
Amazing, thank you so much! This PR might be related to a bunch of existing issues around USBTMC connectivity with Siglent and Rigol instruments (that are known to be... quirky): off the top of the issues list, #414, #436, #318, and #311 seem like they might be caused by the same problem. Once again, thank you for taking the time to dig into the issue and open a PR. |
Happy to help! Took a look at the issues you mentioned. #414 and #436 look like the same issue I was having. #318 could be, but has a slightly different symptom than what I was seeing; in the USBTMC spec they say that the "short message" can be 0 bytes, which sounds kind of like the symptom they describe. #311 definitely won't be fixed by this PR since I didn't change any of the write code. I didn't look into the spec for bulkout at all so I'm not sure if it has the same type of issue. |
There was a problem hiding this comment.
Thanks your contribution ! USBTMC is complicated and all fix improving support are more than welcome.
I left a comment that I think is worth addressing here. Can you have a look @hcoffin-vecatom ?
In addition to that if you can add a releasenotes entry it would be great.
pyvisa_py/usb.py
Outdated
| return self._read( | ||
| _usb_reader, | ||
| count, | ||
| lambda current: False, # USBTMC can return partial message (i.e., |
There was a problem hiding this comment.
Here the end indicator checker does not care about the termination character, only about the fact that the underlying hardware signaled it is done transmitting a message. For GPIB, this is done by checking the state of hardware line, for USB it is always trivially true since by construction a BulkIn message is complete when received. Using this kind of notification is more efficient than checking for term chars.
The _read implementation takes care of ensuring we read till a term char if we were requested to do so. Actually it is at this layer that support for suppress_end is implemented so I would appreciate if while removing this implementation, you could also get rid of the error forbidding the use of suppress_end.
There was a problem hiding this comment.
Thanks for the comment. I didn't fully understand what _read was meant to do. I think I have fixed the issue and also allowed suppress_end_en to be True in my lastest commit.
…uppress_end_en==True
for more information, see https://pre-commit.ci
I think I have made the relevant changes. I'm not sure if I did the releasenotes correctly as I haven't done that before, so feel free to change it if desired. |
|
Also here is the code I used to test it in case someone else needs it: With 'res.set_visa_attribute(pyvisa.constants.VI_ATTR_SUPPRESS_END_EN, True)' it outputs With 'res.set_visa_attribute(pyvisa.constants.VI_ATTR_SUPPRESS_END_EN, False)' it outputs |
|
Wow thanks for doing the fixes so fast !!! For the suppress_end option, can you run your test with NI or Keysight VISA to see if the output is the same ? |
Unfortunately, I have not been unable to correctly install either of those libraries on my computer (running Linux Mint w/ Ubuntu 21.2). I tried installing NI-VISA before looking into changing the code in pyvisa-py and was unable to get it to even see any device let alone query them. I just tried installing Keysight's version, but I'm mostly getting errors (to be fair to Keysight, they say my version of Linux isn't supported during the installation). which (besides the seg fault) is what I got with pyvisa-py backend before any of the updates in this pull request. Trying to run the test script again without replugging the device results in. If I run it with suppress_end set to True, I always get So I would not trust that suppress_end is working correctly unless someone else can verify. |
|
@arr-ee would be able to test the suppress_end behavior ? |
|
I should be able to in the next couple of days, will update soon |
|
Testing with my SDG2122X: Test code:import sys
import pyvisa
pyvisa.log_to_screen()
rm = pyvisa.ResourceManager('@py')
i = rm.open_resource('USB0::62700::60984::SDG2XCA4162836::0::INSTR')
i.set_visa_attribute(pyvisa.constants.VI_ATTR_SUPPRESS_END_EN, sys.argv[1] == '1')
print(f"IDN: {i.query('*IDN?')}")
i.write('C1:BSWV?')
print(f"C1:BSWV?: {i.read_raw()}")pyvisa 1.14.1, pyvisa-py 0.7.2:Repeated executions of the script lead to different results, with previous responses showing up in place of up to date ones, e.g.:
pyvisa 1.14.1, pyvisa-py 0.7.3.dev26+gc632e68:Same code hangs on the first read, not reacting to Ctrl-C. When process is killed, attempts to write to the device timeout, and even physical controls do not react to anything (I suspected it to be "remote" mode, but hard-reboot and running the script using the released version does not lead to UI lock-up). Adding read/write termination does not change the behaviour. I also tried running the same code with R&S VISA, and both through pyvisa and through the bundled tester utility I've been getting Attached are packet captures for released code (before), new code running after released (after), new code running after instrument reboot (after_clean), and rsvisa (rsvisa) |
|
However, I do see progress when running a slightly changed test code against SDS2104X Plus: Code change: print(f"IDN: {i.query('*IDN?')}")
i.write(':WAVEFORM:SOURCE C1')
i.write(':WAVEFORM:DATA?')
print(f":WAVEFORM:DATA?: {i.read_raw()}")Released code gets the same issues with old results, but that also leads to errors. R&S VISA just throws I/O errors on raw read. Updated code (after several tried to get rid of old replies in the instrument buffer) works every time. |
|
I am a bit worried by the very hard freeze you report in you first test but the rest seems like clear win. |
|
According to a quick glance at the packet capture, version from the PR attempts to read repeatedly (?), and responses from the device get more and more jumbled until it hangs. I have not looked into USB packet internals much before, so it might take me a minute. @hcoffin-vecatom let me know if there is any info you might need in the meantime, I have aforementioned Siglent devices, some supposedly better behaving ones (R&S/HPAK), and some potentially broken-in-different-ways (hi GW Instek and possibly Tenma) that I'll try to test against. Another thing I can eventually try is updating the firmware on the SDG — it's several minor versions behind, although it's not old old; and testing it against a windows box with Siglent's drivers to get a "reference implementation" (assuming it works). |
|
Took me a bit to parse those log files, but it looks like your SDG2122X has some even weirder behavior than my SDG1062X. Notice the x01 on the last packet. That represents that this usbtmc message (which could be multiple usb packets) should be the last (the eom logic in usbtmc.py). When you do the same query on your device (looking at the after_clean capture you sent), you never receive an eom (in picture marked below, I have underlined where that should be in red). The device sends a new line character (underlined in green), which I guess is meant to tell the computer to stop reading more packets, but that is not how usbtmc is specified. It then proceeds to just keep copy and pasting its id over and over. To be honest, I have no idea how to handle that in a reasonable way. I guess you could set a relatively short timeout and just get rid of all the extra copies it sends, but that seems quite hacky. Only other option I can see is to parse out termination characters in the USBTMC.read function, which again seems wrong. |
|
Fascinating. @hcoffin-vecatom thank you for looking into the captures right away. I’ll try to run more tests in the next couple of days to hopefully get a better understanding of what’s happening, but ultimately I think this PR should be OK to merge since my instrument’s behaviour appears to be broken-broken. If no one minds I suggest postponing merge until early next week just in case any of further tests point to an explanation/workaround. |
Thought of one more small thing to try over the weekend. I'm guessing you have already tried this, but if you haven't, can you try setting the termination character to '\n' before reading/writing to SDG2122X. It looks like, at least for writing to the device, you are terminating with '\r\n' in that log. The SDG programming manual specifies '\n' as the termination, so I'm not sure what to expect if it isn't. |
|
Yes, I have noticed that you were changing termination settings, and tried that as well with no change in behaviour IIRC. I will try it again to make sure I have not missed a combination of factors that actually works. |
|
Thank you for your patience, took me a minute. I only got to run linux-based tests since windows machine is out of reach for this instrument's USB right now, but tl;dr PR is fine, Siglent just being Siglent. Firmware update from Details:
Apologies for not getting to the bottom of this, but at least it appears to be safe(r) to merge now. |
|
I updated the README and will merge as soon as the CIs are green. |
|
Thanks @hcoffin-vecatom for the patch and @arr-ee for testing !!! |
|
Huge shout to @hcoffin-vecatom for the fix and explanations during testing! |
Following sec. 3.3 of "Universal Serial Bus Test and Measurement Class Specification (USBTMC)" Rev 1.0, updated the read function of USBTMC in protocols/usbtmc.py . Read now reads from usb until a "short packet" has been read (see specification), and only expects a header on the first packet received. In usb.py, added separate read function to USBInstrSession which is nearly identical to USBSessions.read, but has "lambda current: False" instead of "lambda current: True", to allow for usbtmc device to return multiple packets before a termination character and to remove trailing alignment bytes which might be sent by device.
black . && isort -c . && flake8with no errors