Skip to content

modify enable/disable definition for Trigger #93

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

Closed

Conversation

achuang2718
Copy link

I added additional logic to modify how enable() and disable() are defined for a Trigger.

To illustrate, here is a use case:

  • my TriggerableDevice is triggered on a rising edge
  • the digital output being used to trigger the device has a breakout board which, for whatever reason, inverts its output.

So I would then instantiate my Trigger as Trigger(..., inverted=True, trigger_edge_type='rising') so that it matches with my TriggerableDevice.

This seems to make sense, if this was the intended use case of the inverted argument. This is less misleading than using trigger_edge_type='falling' for both Trigger and TriggerableDevice, which is a (working) workaround. However, merging this would break the workaround.

@dihm dihm added the bug Something isn't working label Apr 13, 2023
@dihm
Copy link
Contributor

dihm commented Apr 13, 2023

@achuang2718 Thanks for submitting this PR! This is a curious edge case, but we should still fix it since the workaround really doesn't make much sense. It seems pretty obvious to me that Trigger was not written with inverted DigitalOut in mind so making it aware is an overall good.

If you don't mind, could you update the docstring for Trigger to explicitly mention that the inverted kwarg is respected and has the same functionality as DigitalOut? That should help any who may be using the workaround to track things down.

@philipstarkey
Copy link
Member

As the pulseblaster defines it's trigger edge type as falling, does this break the use of pusleblasters as secondary pseudoclocks? Really the pulseblaster has an inverted trigger input so it sounds like that might be a case of the workaround that you think will break if this PR is merged?

@philipstarkey
Copy link
Member

I've logged this as #109 and will close this PR due to inactivity. It sounds like something that needs to be fixed, but we need to ensure we aren't breaking existing usage. If anyone would like to pick this up again, PRs are welcome as always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants