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

New feature: WaitForExternalTrigger #937

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

conorhughmcfadden
Copy link
Collaborator

Put this feature in your feature list to wait for an external digital trigger before moving on to the next block.

Example feature list which worked:

[{"name": PrepareNextChannel,},({"name": WaitForExternalTrigger,"args": ("/PCIe-6738/PFI4",-1,),},({"name": MoveToNextPositionInMultiPositionTable,},{"name": ZStackAcquisition,"args": (True,True,),},{"name": WaitToContinue,},{"name": LoopByCount,"args": ("experiment.MicroscopeState.multiposition_count",),},),{"name": LoopByCount,"args": (5,),},),]

Option to set a timeout to break the loop if no trigger is incoming.

Tested with a function generator hooked up to PFI4 and it seemed to work well.

- Feature which waits for an external trigger to continue on to the next feature block
- Useful when combined with LoopByCount where each iteration depends on some external event
@conorhughmcfadden
Copy link
Collaborator Author

I found a bug...

In the proposed feature list using multiposition above, the stage will not go back to the first position after the first iteration. It just repeatedly stacks the last position.

I'll investigate why and report back.

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 30 lines in your changes missing coverage. Please review.

Project coverage is 53.08%. Comparing base (4b37039) to head (cc2e20c).

Files Patch % Lines
src/navigate/model/devices/daq/ni.py 0.00% 18 Missing ⚠️
src/navigate/model/features/common_features.py 20.00% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #937      +/-   ##
===========================================
- Coverage    53.18%   53.08%   -0.10%     
===========================================
  Files          177      177              
  Lines        19020    19052      +32     
===========================================
- Hits         10115    10114       -1     
- Misses        8905     8938      +33     
Flag Coverage Δ
unittests 53.08% <9.09%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- current_idx is initialized in the constructor, which is only called when you first run the feature
- Need to reset it to zero if using LoopByCount in for example, a time series
@conorhughmcfadden
Copy link
Collaborator Author

I found a bug...

In the proposed feature list using multiposition above, the stage will not go back to the first position after the first iteration. It just repeatedly stacks the last position.

I'll investigate why and report back.

I found the issue! There appears to have been a bug in MoveToNextPositionInMultiPositionTable. At least when I ran this feature list without my new triggering block, just a normal loop, I got the same issue:

[({"name": PrepareNextChannel, },({"name": MoveToNextPositionInMultiPositionTable, },{"name": ZStackAcquisition, "args": (False,False,"z-stack",),},{"name": WaitToContinue, },{"name": LoopByCount, "args": ("experiment.MicroscopeState.multiposition_count",),},),{"name": LoopByCount, "args": (3,),},),]

It turns out current_idx needs to be reset in the signal_func in order for it to work with LoopByCount. Making this bug fix allows it to return to the original position using a nested loop. I tested it with my WaitForExternalTrigger and it works properly now!

@AdvancedImagingUTSW
Copy link
Collaborator

This is a great addition. Before we merge, I think it would be a good idea to have the calls to the data acquisition card go through the device abstraction layer. This way, should we add additional ways to receive external signals (e.g., Tiger Controller, TriggerScope), it will be more modular.


# Create a digital input task and wait until either a trigger is detected,
# or the timeout is exceeded. If timeout < 0, wait forever...
self.task = nidaqmx.Task('WaitDigitalEdge')
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.task = self.model.active_microscope.daq.create_digital_edge()...

This way if we add a different daq in the future, from another manufacturer, we do not have to rewrite the feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The task is moved to daq_ni.py. Please test the code.

@@ -624,6 +695,11 @@ def signal_func(self):
self.model.move_stage(abs_pos_dict, wait_until_done=True)

self.model.logger.debug("MoveToNextPositionInMultiPosition: move done")

# Make sure to go back to the beginning if using LoopByCount
if self.current_idx == self.position_count:
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to @annie-xd-wang, the multi position feature does not reset the index for the position in the table after it loops. Might be better to update there. She will take a look at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@annie-xd-wang will adjust the code, and then @conorhughmcfadden can test it again on his system, and I will test it on one of our rigs too. If all goes well, we will merge it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@conorhughmcfadden has fixed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested it on my system and Annie's changes work well! There was a minor bug fix but just a typo. I tested the following feature:

[({"name": WaitForExternalTrigger,"args": ("/PCIe-6738/PFI4",-1,),},{"name": PrepareNextChannel,},({"name": MoveToNextPositionInMultiPositionTable,},{"name": ZStackAcquisition,"args": (False,False,"z-stack",),},{"name": WaitToContinue,},{"name": LoopByCount,"args": ("experiment.MicroscopeState.multiposition_count",),},),{"name": LoopByCount,"args": (3,),},),]

Works the same as before!

annie-xd-wang
annie-xd-wang previously approved these changes Jul 15, 2024
@AdvancedImagingUTSW AdvancedImagingUTSW merged commit 8cee25b into develop Jul 15, 2024
1 check passed
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.

3 participants