Add services for Thorlabs motors using Kinesis

[ivalaginja]

Taking over from #170. Fixes #169. See #169 (comment) for details.

Credit for writing the hardware service goes entirely to @a-sevin.

Todo before review

• Write hardware service @a-sevin
• Write service proxy @ivalaginja
• Add simulated service @ivalaginja
• Test in software @ivalaginja
• Add source copyright to hardware service @ivalaginja
• Test on hardware for KDC101 motor @erinpougheon
• Test on hardware for TDC001 motor @erinpougheon
• Test on hardware with 2 motors at the same time @erinpougheon
• Update service doc page @erinpougheon

[ivalaginja]

@raphaelpclt could you provide a first review on this? Some of the hardware tests are outstanding but only as additional iterations.

[erinpougheon]

I went to the lab to perform tests.

I run the following tests on each motor (one KDC and one TDC):

• open the service
• tests the proxy commands (move_relative and move_absolute)
• test the service command (home())
  Everything worked as expected.

Then, I performed tests with the two motors linked to the PC (the TDC and the KDC).
First, I started both, and closed both. The test was successful, everything worked as expected.
Then, I started both, and moved both. The test was also successful.
After that, I started both motors, closed one, moved the other one and closed it. I did another tests the other way around (starting both, closing the second one, moving the first one and closing it). Both tests were successful too!

All the tests we wanted to do on both motors with the service are done and successful.

[ivalaginja]

@ehpor @raphaelpclt if any of you have some time to review this in the coming time, the PR is ready for it.

[raphaelpclt]

It looks good, with a few minor questions.

[raphaelpclt]

wait_for_completion is only used in the homing function. Don't you want to use it in set_current_position too?

[ivalaginja]

The motor itself behaves differently when homing and when moving position, which is why we didn't block it with this wait when it's just changing position. Let me remind myself of the details and then either justify our choice or follow your advice and put it in there as well.

[ivalaginja]

Ok this one is tricky. If we do not wait for completion in the set_current_position(), it intercepts the first command and executes the second command instead, and correctly. So I am fine with this behavior and don't want to add the wait_for_completion there.

However, the method get_current_position() gets called too early and puts an intermediate position on the current_position data stream, which the device reads while it is moving. Which is obviously not at all what we want.

We then tried adding the wait_for_completion() anyway, to fix the wrong values on the data stream, but in this case it just finishes running the first command. Data stream value is still wrong. And it puts command 2 in a cache apparently and runs it before the next command that is sent to the motor... also not the behavior we want, so I still vote against adding the wait_for_completion() in set_current_position().

Since this was so weird, we checked the data stream values after calling home() a couple of times and this is always fine, so it's something about the command data stream monitoring and getting the real device position before the device stops moving.

I don't actually understand what wait_for_completion() does. @a-sevin, maybe you can help out with this conundrum? What does it do and what are the values for message_id and message_type it checks against?

The minimum requirement for this fix is that the correct (final) value is put on the current_position data stream after the movement concludes.

[ivalaginja]

Maybe I should just regularly update the current position, like the Newport XPS service?

catkit2/catkit2/services/newport_xps_q8/newport_xps_q8.py

Line 191 in 6c8900a

while not self.should_shut_down:

@raphaelpclt what do you think about this?

[a-sevin]

I'm reading the tutorial labview which is not so bad. I need to check what to use it in Python:
https://www.thorlabs.com/Software/Motion%20Control/Kinesis/Kinesis-labview.pdf

This wait_for_completion() could be very simple: wait the get_current_position() stable...

[raphaelpclt]

For the get_current_position, yes I think the solution with the newport xps is what you want. Then I am not sure of what you want for the moving behavior. The fact that the moving function returns before completion sounds a bit weird to me, but maybe not dramatic.

[ivalaginja]

Considering we now get correct values on the current_position data stream within the time scales we need (1 s), I will open a follow-up issue to look into a more rigorous handling of the data stream update inside of set_current_position().

Issue: #206

[raphaelpclt]

Unless I miss something, the self.current_position datastream is initialized in open, but never updated. I guess it would have to be update in set_current_position

[ivalaginja]

Huh, you're raising a good point here. This is actually not a bug - the self.current_position data stream gets updated in the simulator service script of our thd2 simulator, much like hicat2's (both in external repos), so we're safe, every time the simulator is called to set the simulated motor position, the data stream is updated as well.

Now, why the choice has been originally done to do it this way, I don't know. I also agree that it would be more logical to update the data stream in the simulated service's set_current_position as is done in the hardware service. Maybe a new issue to open and to discuss?

[ivalaginja]

I now spun off the command monitoring on a thread and update the current position in regular updates in the main(). Works fine in simulation although I didn't check the data streams, which we'll do on hardware - @erinpougheon this can be retested on hardware, where we have to make sure that both move_absolute() and move_relative() still work ok, and then do the current position data stream tests like last time (checking that they carry the correct numbers, after homing and moving (and also in general)).

[erinpougheon]

I just went to the lab to test the changes on hardware.
When I sent a home() command to the motor, I can't send another command while the motor is not stopped. When I asked for the current_position, it gives 0 as expected.
I sent a command move_absolute(15) and then sent (before the motor stopped moving) a move_absolute(4) command. The motor stopped the first movement, and went to 4. Then, I asked for current_position and it gives what we expect (4). I did the test various times, with several values and it worked as expected everytime. When I home() the motor again, I have a current_position 0. I tried with the move_relative() command too and the current_position gives what we want.
All tests were successful.

[ivalaginja]

@raphaelpclt we are getting the expected behavior now so this is ready for a new review. I opened a follow-up issue for a closer look into the data stream synchronization with the motor move (#206).

[raphaelpclt]

Looks good!