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

Improvements to APT #167

Merged
merged 28 commits into from
Feb 10, 2019
Merged

Improvements to APT #167

merged 28 commits into from
Feb 10, 2019

Conversation

cgranade
Copy link
Contributor

This PR (developed in collaboration w/ @crazy4pi314) fixes the Thorlabs APT classes for Python 3 support, and adds new scale factor tables, including for the recently released KDC-101 driver. Also included is a fix for terminators on serial communicators, and a new motion_timeout on APT for slow-moving motor drivers. These changes have been tested some on hardware, but we still need to add better tests (coverage decrease, plus a lack of testing on APT before), hence the WIP tag. As before, I wanted to open the PR early to get design feedback before finishing all tests. Thanks!

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage decreased (-0.1%) to 84.521% when pulling 11822f0 on feature-apt-tables into bc4fdbb on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 84.806% when pulling f493ef6 on feature-apt-tables into bc4fdbb on master.

@scasagrande
Copy link
Contributor

master is the target again :)

@scasagrande scasagrande changed the base branch from master to develop April 20, 2017 13:42
@cgranade
Copy link
Contributor Author

The tests are currently failing as the new APT tests in particular refer to the hw_info instance of struct.Struct that I had started using to refactor out parsing of HW_GET_INFO packets before I got frustrated and worked on #169 instead. I've tried to address with 2acbfc1 by using struct.Struct for now and checking that at least HW_GET_INFO is correctly turned into APT metadata.

@cgranade
Copy link
Contributor Author

cgranade commented Jul 12, 2017

OK, something went seriously wrong with trying to extend the loopback communicator to deal with empty terminators. I'll try to fix it ASAP, sorry for leaving the PR in a broken state. In the meantime, I've merged develop back in to see if that fixes the ruamel.yaml installation errors that are breaking CI builds.

@cgranade
Copy link
Contributor Author

I didn't anticipate that there'd be a significant number of calls of the form comm.read_raw(0), such that I didn't properly handle the size == 0 case. That said, tests now pass locally, so hopefully it should work on Travis as well.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.667% when pulling 5bc5a6e on feature-apt-tables into 215a6d0 on develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.667% when pulling 5bc5a6e on feature-apt-tables into 215a6d0 on develop.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage increased (+0.7%) to 85.919% when pulling e2e59f1 on feature-apt-tables into cd571bc on develop.

@tobiasgehring
Copy link

thanks for all the work. I just tried it out (using python 3.5) and found a couple of issues on my linux machine using a K10CR1:

a = instruments.thorlabs.APTMotorController.open_serial('/dev/ttyUSB0', baud=115200)

  • Some (?) commands take about 3s, which is the serial timeout I believe:
%timeit a.channel[0].position
1 loop, best of 3: 3.01 s per loop
  • a.channel[0].status_bits

AttributeError Traceback (most recent call last)
in ()
----> 1 a.channel[0].status_bits

InstrumentKit/instruments/thorlabs/thorlabsapt.py in status_bits(self)
595 # The documentation claims there are 14 data bytes, but it seems
596 # there are sometimes some extra random ones...
--> 597 resp_data = self._apt.querypacket(pkt).data[:14]
598 # ch_ident, position, enc_count, status_bits
599 _, _, _, status_bits = struct.unpack(

AttributeError: 'NoneType' object has no attribute 'data'

  • that's probably me, but I don't understand how to set e.g. max and min velocities.

@cgranade
Copy link
Contributor Author

@tobiasgehring: I've been testing on a KDC-101, so I can't comment on the K10CR1 in particular. Unfortunately, the way the APT devices set min and max velocities depends a lot on the particular model of controller and motor, such that this PR by no means is intended to finish supporting all APT devices. As for the status bits, as noted by the comment in your quoted code snipppet, the documentation provided by ThorLabs on how the status bits work is in contradiction with what we've seen in the past from actual devices. Currently, my and @crazy4pi314's experimentation with the KDC-101 has been focused on getting the absolute move commands working on the new controller and motor models that have been added since the last improvements to APT support. This has been somewhat complicated by driver issues with the new FTDI chip and driver combination that ThorLabs seems to be shipping (hence #176).

Long story short, we haven't gotten status bits working yet as it that is beyond the scope of this PR and PRs #176, #169.

@tobiasgehring
Copy link

@cgranade: Thanks for your answer. I'm fine with this being beyond this PR, no doubt, I just wanted to share my observations. I'm wondering, did you have a look at thorpy? This works very well with my stage and is using the same protocol. Did you try it with your stage? At least for mine it solves problems with the status bits so it might give you some inspiration.

@scasagrande
Copy link
Contributor

Resurrecting this PR, I think its in a decent-enough state to be merged in now.

While I understand that the changes to the Thorlabs instruments may be incomplete, the comments in #188 indicate that they do help. This PR also includes additional unit tests to help cover a few use cases for these few Thorlabs APT methods that are now known-functional.

And since I know that @cgranade and @crazy4pi314 have moved on from their jobs when this work was originally being worked on, I'm afraid we won't get a chance to work on this for a while. I might at some point be able to borrow some of this equipment from work, but I don't know what the timeline on that would be. It's better at this point to get it merged in since its still net-positive.

@scasagrande scasagrande changed the title [WIP] Improvements to APT Improvements to APT Feb 7, 2019
@scasagrande scasagrande merged commit ca5fb15 into develop Feb 10, 2019
@scasagrande scasagrande deleted the feature-apt-tables branch February 10, 2019 00:10
scasagrande added a commit that referenced this pull request Feb 19, 2019
* Drop support for Python 3.4 (#200)

* Fix issue #197 (#201)

Where checking if value is in enum will raise value error in Py38

* Improvements to APT (#167)

* Added driver/motor tables for T/KDC001 APT devs.

* Moved TODO comment to avoid pylint error.

* Misc Py3k changes for ThorLabs APT

* motion_timeout for APT motor cmds, fix scale factor

* ThorLabsAPT: Example of new config support.

* More pylint fixes

* Fix for line continuation convention.

* Rearranged imports into standard order.

* Added an APT test. Not working yet.

* Fix linting issues

* New handling in loopback for empty terminator.

* struct.Struct for contents of hw_info packets

* Support for specifying expected apt pkt sizes

* Fixes to APT and APT tests

* Missed a conflict marker.

* Fixed bug due to `if size` falling through on size == 0.

* Removed trailing whitespace.

* Locked requirements.txt; see #174.

* Remove numpy version pinning in requirements.txt

* Add tests to cover additional loopback comm behaviour

* Make pylint happy

* Revert changes to size=0 behaviour in loopback comm

* Fix bug with SCPIFunctionGenerator.function, add tests (#202)

* Fix bug with SCPIFunctionGenerator.function, add tests

* Fix linting

* Fix Agilent 33220a, add tests (#203)

* Function Generator single/multi-channel consistency (#206)

* Function Generator single/multi-channel consistency

* Fix Py27 linting issue

* Fix linting issue

* Add tests for FunctionGenerator abstract instrument

* Adding support for the minghe mhs5200 (#150)

* added mhs5200

* added minghe function generator

* added absolute_import

* fixed scaling on frequency

* switched to abstract instrument class

* fixed a few docstrings

* after testing with device

* Minghe MHS5200 - Add instrument to docs

* isolating changes from cc1 test station:

* Revert "isolating changes from cc1 test station:"

This reverts commit 87b8dec.

* reverting changes and fixing duty cycle

* Update for new FunctionGenerator multichannel consistency update

* Docstring modifications (#207)
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.

4 participants