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

APT devices must return serial number to pick the right one #131

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

FilipDominec
Copy link

As originally referenced in #130, I suggest the ParamSet for Thorlabs motion controllers TDC001 should contain their serial number to tell these apart.

Also I noticed my controllers do not report correctly the 5th and 6th byte, causing some commands to stall.

With these two fixes, TDC001s work like charm.

Copy link
Contributor

@natezb natezb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for doing this! In addition to the little changes I've asked for, can you also make this work a little better with the generic instrument function? This should be what you need to do

  1. Add 'port' to the _INST_PARAMS_ list. (This isn't due to your changes, it was already missing)
  2. Regenerate driver_info.py (run python -m instrumental.parse_modules from the Instrumental directory, or use Instrumental/instrumental/parse_modules.py directly)
  3. Test that you can open these devices using instrument(serial=xxx), instrumental(port=xxx), and/or TDC001_APT(serial=xxx) and TDC001_APT(port=xxx)

Let me know if this doesn't work, or if you have any questions. If that all works, please add driver_info.py to your PR, alongside the other changes.

@@ -11,8 +11,9 @@
from ... import Q_, u


def list_instruments():
return [ParamSet(TDC001_APT, port=p.device) for p in comports() if (p.vid, p.pid) == (0x0403, 0xfaf0)]
def list_instruments(filter_serial_number=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter_serial_number parameter is no longer used, so we can remove it

def list_instruments():
return [ParamSet(TDC001_APT, port=p.device) for p in comports() if (p.vid, p.pid) == (0x0403, 0xfaf0)]
def list_instruments(filter_serial_number=None):
return [ParamSet(TDC001_APT, port=p.device, serialnumber=p.serial_number) for p in comports()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change serialnumber to serial? That will make it consistent with other devices.

@@ -127,7 +128,7 @@ def move_completed(self):
""" Check if the move is completed.
"""
rsp = self._ser.read(6)
if rsp == bytes([0x64, 0x04, 0x0e, 0x00, self.src | 0x80, self.dst ]):
if rsp[:4] == bytes([0x64, 0x04, 0x0e, 0x00, self.src | 0x80, self.dst ])[:4]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a short comment here (and at the other location) to indicate why we're only taking the first 4 bytes? This is the sort of info that's easy to forget when we want to change the code in the future.

@FilipDominec
Copy link
Author

OK, thanks. Meanwhile I got my repository into unusable state again and have no expert to help, so tomorrow I will clone the repository anew and submit another PR based on your notes. Programming is easy, git is hard.

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

Successfully merging this pull request may close these issues.

2 participants