Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solution to fix usbtmc frame partitioning issues on Linux. #468

Open
jyfrau opened this issue Nov 18, 2024 · 10 comments
Open

solution to fix usbtmc frame partitioning issues on Linux. #468

jyfrau opened this issue Nov 18, 2024 · 10 comments

Comments

@jyfrau
Copy link

jyfrau commented Nov 18, 2024

Hello,

I am sharing with you a solution to fix the usbtmc frame partitioning issues that are not handled properly under Linux.

I am using pyvisa and pyvisa-py to communicate with Siglent devices (SSA3032X and SVA1032X). On Linux it does not work. I get the message "Unexpected MsgID format. Consider updating the device's firmware. See #20".

The problem comes from the fact that usbtmc frames are split into 64-byte packets. A problem that does not appear under Windows, nor on a Linux virtual machine on a Windows host (which therefore goes through the Windows driver...).

Although this partitioning management problem does not come from pyvisa-py, I found a solution to add a processing of these packets in the file "…/site-packages/pyvisa/usbtmc.c", in the read() function of the USBTMC class :
when receiving the usbtmc frame (resp = raw_read(…)), I read the data length field directly in the usbtmc header and I receive the missing packets (resp += raw_read(…)) as long as the usbtmc frame is not the expected length.

It takes just 3 lines of code (between "### DEB PATCH" and "### END PATCH"):

try:
resp = raw_read(recv_chunk + header_size + max_padding)

### DEB PATCH                                                             # processing if partitioned frame :
usbtmc_size = header_size + int.from_bytes(resp[4:8], byteorder='little') # calculating the number of bytes expected
while len(resp) < usbtmc_size:                                            # as long as the frame is incomplete...
    resp += raw_read(recv_chunk + header_size + max_padding)              # ...we get a packet
### END PATCH

response = BulkInMessage.from_bytes(resp)

except (usb.core.USBError, ValueError):
# Abort failed Bulk-IN operation.
self._abort_bulk_in(self._btag)
raise

received.extend(response.data)

This partitioned frame handling in usbtmc.c works very well. I tested it with the Siglent SSA3032X and SVA1032X, as well as with a Keysight EDUX1002A (which does not partition its frames). I did this under Linux and under Windows, it works fine.

These 3 additional lines of code remain transparent if they are not needed but provide a great service in the event of partitioning poorly managed by the lower layers.

Although it is not the role of pyvisa-py to do this processing, do you think it is possible to integrate it into a future update of the pyvisa-py module?

Best regard

The modified read method of the USBTMC class:
USBTMC read

@jyfrau
Copy link
Author

jyfrau commented Nov 19, 2024

Hello,
In order to detail this problem of cutting into 64-byte packets of usbtmc frames by Siglent devices, here are two examples of usbtmc frames received with the Siglent SSA3032X device.

Example 1 :
1 - In the usbtmc.py file, I deactivate my patch and display the received frame and its length.
EX1-log

2 - I send the following commands to the SSA3032X.
EX1-cmd

3 - I look at the results of print("resp_size :", len(resp)) and print(resp)
EX1-out

For *IDN?, I receive a first packet of 64 bytes, with the header of 12 bytes.
The length field b’5\x00\x00\x00’ indicates 53 bytes of data.
The usbtmc frame of 12+53 = 65 bytes is sent in two packets:

  • The first packet is swapped to 64 bytes
  • The second packet is 4 bytes: the last byte of data (b’\n’) and the 3 padding bytes

For the screenshot command:
The length field b’ \x00P\x00\x00’ indicates 20480 bytes of data. This is indeed the value of chunk_size.
The usbtmc frame is sent in several packets of 64 bytes.
We can clearly see in this first packet the 12 header bytes followed by the beginning of the image which begins with ‘BM’.
The next packet of 64 bytes will crash the communication.

Exemple 2 :
1 - In the usbtmc.py file, I activate my patch and display the received frame and its length.
EX2-patch log

2 - I send the same commands to the SSA3032X.
EX1-cmd

3 - I look at the results of print("resp_size :", len(resp)) and print(resp)
EX2-out

For *IDN?, the received usbtmc frame is correct: it is indeed 68 bytes long:
12 header + 53 data + 3 padding

For the screenshot command, the received usbtmc frame is correct: it is indeed 20492 bytes long:
12 header +20480 data + 0 padding
The other usbtmc frames containing the rest of the image are also sent correctly.
At the end, the received image is reconstructed correctly.

@MatthieuDartiailh
Copy link
Member

@jyfrau thanks for the detailed explanation.

It looks somewhat similar to the patch that landed in #465, can you test with latest main and let us know if this fixes your problem or not ?

@jyfrau
Copy link
Author

jyfrau commented Nov 19, 2024

After reading #465 I realized I was still on 0.7.1, sorry.
But it doesn't work with version 0.7.2 either

After update:
pyusb 1.2.1
PyVISA 1.14.1
PyVISA-py 0.7.1 => 0.7.2

I did some tests with version 0.7.2:

1 - If I execute:
rep = inst.query("*IDN?")
print("rep size : ", len(rep))
print(rep)
inst.write(":HCOPy:SDUMp:DATA?")
image = inst.read_raw()
print("image size : ", len(image))
print(image[:len(image)])

I get:
.../python3.9/site-packages/pyvisa_py/protocols/usbtmc.py:116:
UserWarning: Unexpected MsgID format. Consider updating the device's firmware.
See https://github.com/pyvisa/pyvisa-py/issues/20Expected message id was 2, got 48.
warnings.warn(

rep size : 54
Siglent Technologies,SSA3032X,SSA3XLBD2R1647,1.3.9.10

image size : 2
b'\x00\x00'

I have the correct answer to "*IDN?", it is indeed 54 bytes.
However, after the screenshot command I get 2 zero bytes, probably the end of the padding of the answer to "*IDN?".
And then the script crashes.

2 - If I run only the screenshot:
inst.write(":HCOPy:SDUMp:DATA?")
image = inst.read_raw()
print("image size : ", len(image))
print(image[:len(image)])

I get:

.../lib64/python3.9/site-packages/pyvisa_py/protocols/usbtmc.py:116:
UserWarning: Unexpected MsgID format. Consider updating the device's firmware.
See https://github.com/pyvisa/pyvisa-py/issues/20Expected message id was 2, got 0.
warnings.warn(

image size : 56
b'BM6 \x1c\x00\x00\x00\x00\x006\x00\x00\x00(\x00\x00\x00\x00\x04\x00\x00X\x02\x00\x00\x01\x00\x18\x00\x00\x00\x00\x00\x00 \x1c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff'

I only get the first 56 bytes of the image (out of the expected 1843200 bytes).
And then the script crashes.

3 - If I test version 0.7.2 on the Keysight EDUX1002A which does not partition its frames: the operation is correct.

4 - If I add my patch in version 0.7.2:

  • the EDUX1002A works correctly
  • The SSA3032X crashes.

Version 0.7.2 seems less stable than 0.7.1 in case of partitioning of usbtmc frames.

@MatthieuDartiailh
Copy link
Member

0.7.2 does not contain the fix I mentionned.

Can you test installing the current main branch pip install git+https://github.com/pyvisa/pyvisa-py ?

@jyfrau
Copy link
Author

jyfrau commented Nov 21, 2024

Hello,

I haven't been able to do any yet "pip install git+https://github.com/pyvisa/pyvisa-py" under a Linux machine (python 3.9 not compatible).

But I did some tests with PyVISA-py 0.7.3.dev34+g4a77dd0 under Windows.
Unfortunately this does not work with the Siglent SSA3032X.
As with version 0.7.2, changes made before reading the frame in USBTMC.read() cause the exchange to crash.

To check the integrity of the usbtmc frames I put the instruction "print(resp)" in the USBTMC.read() class
print
All my versions of pyvisa-py (0.7.1, 0.7.2 and 0.7.3.dev34+g4a77dd0) have this "print(resp)".

Version 0.7.1 works fine.
V71
For "*IDN?", the response has the correct format with <header + data + padding>. The data length field is correct (53 bytes).
For the response to the screenshot ":HCOPy:SDUMp:DATA?", everything is correct too. The data length field is 20480. (I have of course only put the beginning of the first frame of 12+20480 bytes.)
At the end of the exchanges the image is correctly retrieved.

Version 0.7.2 crashes.
V72
For the response to "*IDN?", the size has changed from 53 to 52 (while the final '\n' is indeed part of the frame data). And there is a second frame with a data length field of 1 !!!
For the response to the screenshot, the first frame has a data field of 53 (instead of 20480). The transfer will take 10 ms instead of a few seconds and the received image is incorrect.

Version 0.7.3.dev34+g4a77dd0 also crashes.
VGT
The problems are the same as with version 0.7.2

In conclusion, I'm sticking with version 0.7.1 for now.

@MatthieuDartiailh
Copy link
Member

What changed between 0.7.1 and 0.7.2 is that we now used the MaxPacketSize as reported by the USB endpoints instead of 2048 which was hardcoded. This was done to fix en issue with a Rigol scope whose size 64 and that truly could not manage larger transfer.

Reading your first message, I understand that your patch is sufficient to recover valid data (although slowly I assume). Is that so ?
0.7.3 contains the following logic which is equivalent to your patch:

resp = raw_read(recv_chunk + header_size + max_padding)
                response = BulkInMessage.from_bytes(resp)
                received_transfer.extend(response.data)
                while (
                    len(resp) == self.usb_recv_ep.wMaxPacketSize
                    or len(received_transfer) < response.transfer_size
                ):
                    # USBTMC Section 3.3 specifies that the first usb packet
                    # must contain the header. the remaining packets do not need
                    # the header the message is finished when a "short packet"
                    # is sent (one whose length is less than wMaxPacketSize)
                    # wMaxPacketSize may be incorrectly reported by certain drivers.
                    # Therefore, continue reading until the transfer_size is reached.
                    resp = raw_read(recv_chunk + header_size + max_padding)
                    received_transfer.extend(resp)

So if your patch works I would expect 0.7.3 to also work. But the fact it is slow will remain.
We could add a way to override the reported maxPacketSize for instruments such as yours that report a size that is smaller than the reality of what they can handle.

@jyfrau
Copy link
Author

jyfrau commented Nov 22, 2024

My patch has no impact on speed. A SSA3032X screenshot is as fast under Windows "without patch " as under Linux "with patch". Retrieving a screenshot is done in 2 to 3 seconds. But the patch only works with V0.7.1

On the other hand I don't understand V0.7.3, I may be wrong but the use of "wMaxPacketSize" instead of "RECV_CHUNK = 1024**2" seems to me to be an error:
USBTMC read

(1)
wMaxPacketSize is the max length of DATA (transaction level), while recv_chunk is relative to the length of BULK IN USBTMC message data bytes (transfer level).
BULKINmessage

recv_chunk must be equal to or greater than BULK IN message size (transfer level). The value RECV_CHUNK = 1024**2 was a good solution. This allowed to work even if chunk_size is modified in the VISA layer (it is set by default to 20480 bytes), and it is this one that is used for the size of the BULK IN message.

In summary: the USBTMC message (here our response to a SCPI command in VISA) is split into several transfer BULK IN messages, which in turn are split into “USB transactions”.

(2)
resp
resp must retrieve a complete BULK IN transfer : header + message + alignement bytes

response
BulkInMessage.from_bytes(resp) must process a full BULK IK transfer

A treatment with wMaxPacketSize is transaction level, it must remain below, in the usb layer.
Or at most in a patch between these two lines ("resp=..." and "response=...") if the transaction packets were not correctly assembled.

@MatthieuDartiailh
Copy link
Member

@Jimmyvandenbergh could you join this conversation. You and @jyfrau have spent far more time than me going though the USBTMC standard.

@Jimmyvandenbergh
Copy link
Contributor

Jimmyvandenbergh commented Nov 22, 2024

I think that both in 0.7.2 and 0.7.3 some details are causing this problem.

  1. as @jyfrau pointed out the line: recv_chunck = self.usb_recv_ep.wMaxPacketSize - header is not correct. this should be the requested bytes by the User. According to spec the device may return fewer bytes. Therefore we want to request for example the hardcoded 2048 bytes or whatever other number the user wants to get back from the device. see PR Support partial usbtmc reads and request the size amount of bytes from the device #470

  2. The max_padding depends on the wMaxPacketSize so it is not fixed 511 I removed this in PR Support partial usbtmc reads and request the size amount of bytes from the device #470 so the requested raw_read() matched the wMaxPacketSize

  3. as @jyfrau also indicates we want to read the transfer_bytes that the device wants to send back. this was fixed in PR Update usbtmc read to handle transfer_size and discard alignment bytes #465. but still had some issues. hence PR Support partial usbtmc reads and request the size amount of bytes from the device #470

@jyfrau are you able to test PR #470 with your device, i tested with a Rigol for *IDN? and tested multiple Admesy devices with raw data and arbitrary block reads. these work fine.

@Jimmyvandenbergh
Copy link
Contributor

Jimmyvandenbergh commented Nov 22, 2024

please note that i have used wMaxPacketSize as during debugging sometimes the devices may hang when the line was raw_read(recv + header_size + max_padding) and did not understand why, therefore i changed it to wMaxPacketSize

I know in C you normally would use a known buffer size and request that amount of bytes from libusb. But here also multiple read may be performed as libusb does not return the transfer_bytes from the device in 1 read.

so maybe there is a speed improvement to be made for large data returns in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants