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

added option to parse through TS or DAC #318

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

added option to parse through TS or DAC #318

wants to merge 3 commits into from

Conversation

edyoshikun
Copy link
Contributor

There was no need to enumerate the devices as this reads the State0 config properties and looks for devices with TS. As long as the channels have been set properly, the current implementation works well.

Additionally, there was no current need to add another dropbox for the DAC device, but perhaps in the future if users are are using DACs other than the triggerscope the if/else case should be changed to look for just DAC instead.

Tested this on Mantis. Fixes #312

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Merging #318 (a73a48e) into main (96dca01) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##            main    #318      +/-   ##
========================================
- Coverage   7.23%   7.22%   -0.01%     
========================================
  Files         25      25              
  Lines       4439    4440       +1     
========================================
  Hits         321     321              
- Misses      4118    4119       +1     
Files Changed Coverage Δ
recOrder/calib/Calibration.py 0.00% <0.00%> (ø)
recOrder/plugin/main_widget.py 0.00% <0.00%> (ø)

@deprecated-napari-hub-preview-bot
Copy link

deprecated-napari-hub-preview-bot bot commented Feb 14, 2023

Preview page for your plugin is ready here:
https://preview.napari-hub.org/mehta-lab/recOrder/318
Updated: 2023-07-31T21:29:15.439404

Copy link
Contributor

@ziw-liu ziw-liu left a comment

Choose a reason for hiding this comment

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

Code looks good. I just realized that the TriggerScope/DAC features are not documented at all. Maybe for another issue...

@talonchandler
Copy link
Collaborator

Thanks @edyoshikun.

TriggerScope/DAC features are not documented at all

Thanks @ziw-liu. I'll move this to a separate issue.

@talonchandler talonchandler marked this pull request as draft February 16, 2023 22:45
@edyoshikun
Copy link
Contributor Author

I realized this will fail if the name of the Triggerscope is back to to 'TS' since the calibration.py in recOrder uses the name. Fixing this

Copy link
Contributor

@ieivanov ieivanov left a comment

Choose a reason for hiding this comment

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

This is not a permanent solution still. We should allow the users to name their DAQ device arbitrarily, and potentially use an Arduino or NI DAQ instead of a TriggerScope. DACs are registered as SignalIODevice. We should check for that using mmc. getDeviceType. Here is the SignalIODevice API: https://github.com/micro-manager/mmCoreAndDevices/blob/77ccd434cb84b86892b5544d1bf17aafa57cacdd/MMDevice/MMDevice.h#L881

@talonchandler
Copy link
Collaborator

@edyoshikun will revisit this in preparation for polarization experiments on mantis.

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.

[BUG] LCs not recognized when using multiple Triggerscopes
5 participants