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

Service docs work (Steiger) #224

Merged
merged 15 commits into from
Jul 16, 2024
Merged

Conversation

steigersg
Copy link
Contributor

@steigersg steigersg commented Jul 9, 2024

This is part of the team effort to fill out the documentation pages in the services module of catkit2 (see issue #190 for more information).

service docs included in this PR:

  • bmc_dm
  • nkt_superk
  • FLIR camera

@steigersg steigersg self-assigned this Jul 9, 2024
@steigersg steigersg mentioned this pull request Jul 9, 2024
21 tasks
@steigersg
Copy link
Contributor Author

I would appreciate a second opinion on the driver information to make sure that information is accurate.

@steigersg steigersg marked this pull request as ready for review July 10, 2024 15:58
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

This all looks right to me! I cannot helo for the FLIR drivers, but I did leave some comments on the BMC service doc.

@@ -1,14 +1,57 @@
Boston Deformable Mirror
========================
This service operates a Boston Micromachines MEMS DM. The following Boston DMs have been tested with catkit2 thus far:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a service for a pair of Boston DMs controlled by the same driver. Since this can be a limitation in some cases, @ehpor and I started working on a new service that is more flexible in this regard in #156. While this will not alter this service here, it might be good to explain that it is for a pair of the same type of Boston DMs, with the same number of actuators, controlled by a single driver.

I will make sure to highlight the respective changes when I write the service doc page in #156.


Properties
----------
``channels``: List of command channel names (strings).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup!


``total_surface``: Map of the total amplitude of each DM actuator (meters).

``channel_name``: Name of the DM channel being commanded.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the DAC we used ```channels[channel_name]``: The command per virtual channel, identified by channel name.` to try to make it come through that this is a dict holding several data streams - maybe you could adapt a similar solution here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's a good idea to make the syntax consistent - updated.

Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot for this work! I'll leave it up to you if you want to hold off the merge until you get confirmation on the FLIR driver info or if you want to go ahead.

Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

While working on #156, I thought of a couple more clarifications that would help to have in the docs here. Sorry to throw another round of comments at you after having already approved this.

Datastreams
-----------
``total_voltage``: Map of the total voltage applied to each actuator of the DM.

``total_surface``: Map of the total amplitude of each DM actuator (meters).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just had a look while working on that other PR, and I am pretty sure that the units of the data frames on the total_surface data stream are in nanometers. Could you please confirm this and if it is true, update this comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, and maybe we could avoid the word "map" here? Emiel initiated a refactor in #123 to be a little stricter on the differentiation between "DM commands" and "DM maps", and even if this does not apply to this service here, it might help to stick to that in the docstrings too. Maybe something like "Array of the total amplitude..." would work? Same for the total_voltage data stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! Yes, just confirmed the units are nm and so updated that and also changed "map" to "array"


``total_surface``: Map of the total amplitude of each DM actuator (meters).

``channels[channel_name]``: The command per virtual channel, identified by channel name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the same note, we should probably specify that the commands on the data streams for the individual channels are in units of nm surface.

ivalaginja
ivalaginja previously approved these changes Jul 15, 2024
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Good by me! Again back to you to tell when you're done with the FLIR docs.

@steigersg
Copy link
Contributor Author

Good by me! Again back to you to tell when you're done with the FLIR docs.

On second thought I edited the page the FLIR docs pointed to since I found a more descriptive one. I am happy with it now though.

@steigersg steigersg merged commit 5c2f7a9 into develop Jul 16, 2024
6 checks passed
@steigersg steigersg deleted the feature/add_service_docs_steiger branch July 16, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants