Update usbtmc read to handle transfer_size and discard alignment bytes

[Jimmyvandenbergh]

This PR resolves an issue where the wMaxPacketSize is incorrectly reported by certain drivers.
As referenced in PR #417, wMaxPacketSize from the USB endpoints should be used. However, on Windows with the libusb1 driver, wMaxPacketSize is always reported as 1024, even for USB 2.0 devices, where it should be 512 (wMaxPacketSize = 0x200). This can be listed with the USB Device Viewer of Windows.

To address this, the current PR enhances the changes made in PR #449 by leveraging the known transfer_size value from the USBTMC header, ensuring correct data handling despite incorrect wMaxPacketSize reporting by the driver.

Additionally, this PR fixes an issue from PR #449 where alignment bytes were not being discarded. The message should only contain the number of bytes indicated by transfer_size, so the USBTMC header and any alignment bytes are now properly discarded.

• Closes # (insert issue number if relevant)
• Executed black . && isort -c . && flake8 with no errors
• The change is fully covered by automated unit tests
• Documented in docs/ as appropriate
• Added an entry to the CHANGES file

[MatthieuDartiailh]

Thanks for your contribution. The logic seems sound except for the collection of the received valid data in the received_message bytearray (see comment).

Can you also add an entry to the changelog ?

@hcoffin-vecatom @arr-ee would you mind having a look ?

[MatthieuDartiailh]

I will wait to see if @arr-ee or @hcoffin-vecatom want to comment before merging. They have contributed to usbtmc support and have more test capabilities than I do, to check the changes.

[hcoffin-vecatom]

This does seem to mess things up a little with my Siglent SDG 1062X, but I think @Jimmyvandenbergh 's changes follows the USBTMC protocol better so I think its probably for the best. It is still significantly better than before #449 where I would get different responses each time I ran the code because a full message wasn't being read. For other's reference I'll put what happens on my device:

I run

import pyvisa

rm = pyvisa.ResourceManager('@py')
res = rm.open_resource('USB0::62700::4355::SDG1XDDC801438::0::INSTR')
res.set_visa_attribute(pyvisa.constants.VI_ATTR_SUPPRESS_END_EN, True)
res.read_termination = '\n'
res.write_termination = '\n'

with res:
    res.write('*IDN?')
    print(res.read_raw())

and expect the response
b'Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.01.33R8\n'
(this is what I see if I find the firmware version by navigating through the device's menu manually)

With the changes in this PR, I instead get
b'Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.018\n'

The reason this happens is, in the first transfer, the device sets the transfer size to 52 when it should be 56. So, the device sends the two transactions (I'm including the header just for completeness)
b'\x02\x02\xfd\x004\x00\x00\x00\x00\x00\x00\x00Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.01'
b'.33R'
which in total are 56 bytes instead of the 52 bytes the device claims it should be. So, this new PR cuts it off to just
b'\x02\x02\xfd\x004\x00\x00\x00\x00\x00\x00\x00Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.01'

Then, a second transfer occurs with the following response
b'\x02\x03\xfc\x00\x06\x00\x00\x00\x01\x00\x00\x008\n\x00\x00\x00\x00\x00\x00'
which has no issues. It combines the two transfers into one message, giving the final
b'Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.018\n'

[hcoffin-vecatom]

One more note:
It seems like a lot of these devices are relying fairly heavily on the termination character being processed in ways not specified by the USBTMC protocol. It might be worth considering processing termination characters at the USBTMC level even though that isn't how the protocol is specified (or maybe add a quirky behavior for certain brands of devices).

Examples:

For my 1062X, Siglent seems to be relying on the termination character to determine which bytes might be alignment bytes. The transfer size reported by the device in the header seems to max out at 52 (i.e., the max that can be sent in one transaction), even if a transfer is multiple transactions, and is therefore pretty much meaningless.

For @arr-ee 's SDG2122X (#449 (comment)) it seems like the device never reports an EOM and instead relies on the termination character to specify that a message is complete (each transfer is still finished after a short transaction).

In issue #463 , the Rigol DS1102Z-E doesn't seem to be sending short transactions to end a transfer, but the device does put a termination character at the end of the last transaction, presumably to indicate that no more data will be sent.

Screenshot from USBTMC spec that I am using to define the terms "message", "transfer", and "transaction":

[Jimmyvandenbergh]

@hcoffin-vecatom according to table 4 or 9 of the USBTMC specification.

If bmTransferAttributes.D1 = 1, TermChar is an 8-bit value representing a termination character. If supported, the device must terminate the Bulk-IN transfer after this character is sent. If bmTransferAttributes.D1 = 0, the device must ignore this field.

so this seems that if the bmTransferAttributes is not set this field must be ignored else one needs to stop reading data when for example `\n is set as termination char.
the only thing i do not understand is the last note:

The Host must ignore EOM if the device does not send TransferSize message data bytes.

As this is part of the header and should be > 0x000000.
But maybe they are implying , when 0x000000 is set, it should read until termination char.
As looking at the header of Siglent , the bytes 4-7 are set to 0x00, and should , if above is correct ignore OEM and read until termination char / short packet?

b'\x02\x02\xfd\x004\x00\x00\x00\x00\x00\x00\x00Siglent Technologies,SDG1062X,SDG1XDDC801438,1.01.01'
b'.33R'

Looking at both message you receive then one is wxMaxPacketsize (52+12 start package = 64) and the other is the short package size (4). Which fulfills the USBTMC spec when termination char is met. So has the Siglent device has termination char enabled?

for reference for everyone: the official test measurement class specification

[hcoffin-vecatom]

Bytes 4-7 are actually 0x34 0x00 0x00 0x00 for that transaction, not 0x00 0x00 0x00 0x00, it's just printed funny because 0x34 (52 decimal) is the ascii code for the character '4' (so it prints 4\x00\x00\x00). I probably should have printed the header as pure hex values to make it more clear.

[Jimmyvandenbergh]

Got it, then everything makes sense in your comment.

With this information and following the USBTMC protocol, either one of following should happen

1. the transfer size was set accordingly (56) e.g. multiple 'transaction' should be read. Therefore in my opinion this is a 'bug' in the driver of Siglent (device side). As the bytes are send in a new message request without header.
   _Nevertheless I understand that there is a workaround desired. _

2. Or the second part should be read with a new 'transfer', which includes a new header. But then again a short package with zero bytes is expected, on transfer 1, according to spec.

[hcoffin-vecatom]

Agreed it is a bug on the Siglent device side. Your PR matches the protocol as far as I understand it. I don't think a workaround needs to be included, but I figured I would share my buggy device experience (along with the others mentioned) in case someone is trying to figure out what is going on with their own buggy device.

[arr-ee]

Between this and #463 I wonder if we should at least include a WARN when used with known “quirky” devices pointing to relevant issues since these behaviours are quite subtle and confusing, and are likely to be hit by quite a few people since both Rigol and Siglent devices in question target the lower end of the market.

I hope to find some time to look into #463 and quirks support soon; meanwhile, enormous thanks to @Jimmyvandenbergh and @hcoffin-vecatom for your time and a very educational dive into the standard!

[MatthieuDartiailh]

Thanks for the very nice discussion indeed.
I know it may not be feasible for all of you but I would be curious to know if your instruments behave properly with NI or Keysight VISA. If they don't, I am fine putting the blame on the manufacturer. If they do, I wish we could find the middle ground between the exact USBTMC spec and what manufacturer ended up doing because it just worked for major implementers.
In the meantime I will merge. There are already multiple issues talking about USBTMC so I am not sure we want a new one so feel free to keep the discussion for the time being.

[Jimmyvandenbergh]

Got some time to dive a bit further in the Rigol communication, since we have a Rigol DG1022Z at work.
What i can see when running pyvisa-py vs NI-VISA is:
NI-VISA:

• Requests more data when using their interactive tool. (by defaults 1024 bytes). the header is therefore filled with a request to send up to 1024 bytes. This then return the request *IDN?\n.
• Will wait until timeout is triggered before returning with data when 52 bytes are requested. VISA: (Hex 0x3FFF0006) The number of bytes transferred is equal to the requested input count. More data might be available.

pyvisa-py

• Requests 52 bytes. This is conform the code, but never return with data, due to the fact that the timeout per default is set to none. settings this to for example 5 seconds, this will trigger an error: (needs validation as I already altered the code)
• Correct in this case is that OEM is not set, as the message should exists out of multiple `transfers, which Rigol does support.

Therefore this is a bug in their software, when the requested bytes (including USBTMCHEADER) are equal to the wxMaxPacketSize and the requested data that should be returned is larger than (or maybe equal to) wxMaxPacketSize. (see specification below).

I have setup a test with our USBTMC device, and can see these USBTMC devices send a 0 bytes package when the data is equal to wxMaxPacketSize.
But this currently throws an error in usbtmc.py at line 114: msgid, btag, btaginverse = struct.unpack_from("BBBx", data) as data is here zero length. Please note that this is according to / allowed by specification of the USBTMC protocol:

USBTMC 2.0 specification:

The device must always terminate a Bulk-IN transfer by sending a short packet. The short packet
may be zero-length or non zero-length. The device may send extra alignment bytes (up to
wMaxPacketSize – 1) to avoid sending a zero-length packet. The alignment bytes should be 0x00-
valued, but this is not required. A device is not required to send any alignment bytes.

I think some adjustments need to be made to the code as also the header should be filled with the requested bytes and not the chucks.
If I have a solution I will make a new PR. (or an issue with a WIP branch)

[MatthieuDartiailh]

Could this be linked to the discussion in #468 ? Would you have the bandwidth to test this patch ?

[Jimmyvandenbergh]

@MatthieuDartiailh i will have a look, and also #468 where you looped me in.
Currently i have limited time to test. but will check if in parallel i can setup a Debian system for tests.

just updated this code to a working variant for Rigol DG1022Z and our own instruments in PR #470