labscript-suite / labscript

The 𝗹𝗮𝗯𝘀𝗰𝗿𝗶𝗽𝘁 library provides a translation from expressive Python code to low-level hardware instructions.
http://labscriptsuite.org
BSD 2-Clause "Simplified" License
9 stars 48 forks source link

modify enable/disable definition for Trigger #93

Closed achuang2718 closed 7 months ago

achuang2718 commented 2 years ago

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

To illustrate, here is a use case:

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 commented 1 year ago

@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 commented 1 year ago

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 commented 7 months ago

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.