pyvisa / pyvisa-py

A pure python PyVISA backend
https://pyvisa-py.readthedocs.io
MIT License
288 stars 124 forks source link

usbtmc: enable remote control on USB488 devices #241

Closed itayperl closed 4 years ago

itayperl commented 4 years ago

Thorlabs PM101 would not accept any commands without enabling REN first.

MatthieuDartiailh commented 4 years ago

Not sure why ci did not trigger. Let's try using bors

bors try

bors[bot] commented 4 years ago

try

Build failed:

itayperl commented 4 years ago

try

Build failed:

Hmm, the errors seem unrelated to my changes.

MatthieuDartiailh commented 4 years ago

I agree. I will try to fix them on master (not sure how they could end up there in the first place) and ask you to rebase once it is done. Otherwise the changes look good.

MatthieuDartiailh commented 4 years ago

bors try

bors[bot] commented 4 years ago

try

Build failed:

MatthieuDartiailh commented 4 years ago

Can you run black on your changes ? I took care of the other issue on master.

itayperl commented 4 years ago

bors try

bors[bot] commented 4 years ago

:lock: Permission denied

Existing reviewers: click here to make itayperl a reviewer

MatthieuDartiailh commented 4 years ago

Bors try

bors[bot] commented 4 years ago

try

Build succeeded:

MatthieuDartiailh commented 4 years ago

From the USBTMC 488 specification (http://sdpha2.ucsd.edu/Lab_Equip_Manuals/usbtmc_usb488_subclass_1_00.pdf) your change regarding trigger is incorrect. TRIGGER is a valid MsgID while REN control is a query (along with READ_STATUS_BYTE 128, GO_TO_LOCAL 161 and LOCAL_LOCKOUT 162). Furthermore the bit you read regarding capabilities only apply to REN, GO_TO_LOCAL and LOCAL_LOCKOUT but not TRIGGER. I would be nice to store both (rather than _cap_usb488_states, I would suggest _usb488_supports_trigger and _usb488_supports_ren and add a comment describing that it applies also to the other 2 queries). It would also be nice to store the fact that the device is usb488 (_is_usb488) since it will let us know if READ_STATUS_BYTE is supported. The relevant bits are described in the mentioned documents.

Can you take care of those changes ? Also can you add an entry to the change log ?

itayperl commented 4 years ago

You are correct about TRIGGER, my bad! I added a namedtuple to store the USB488 capabilities more cleanly.

MatthieuDartiailh commented 4 years ago

This looks good and I assume you tested locally (I do not have access to right setup et the moment). I just took the liberty to re-phrase the changelog.

bors try

bors[bot] commented 4 years ago

try

Build succeeded:

MatthieuDartiailh commented 4 years ago

All good ! Thanks for your contribution !

bors r+

bors[bot] commented 4 years ago

Build succeeded:

itayperl commented 4 years ago

Thanks @MatthieuDartiailh, really appreciate the responsiveness!

MatthieuDartiailh commented 4 years ago

Thanks ! Trying to do my best on that front.