Recent GatedClocks-bugfix merge breaks (at least) Novatech DDS9m timing #51
Comments
|
Original comment by Philip Starkey (Bitbucket: philipstarkey, GitHub: philipstarkey). I don’t believe this is actually a timing bug. I think that my fix for a timing bug has identified a region of your code that was not technically safe (although no doubt worked). The issue I fixed was ensuring that a clock tick that updated a Novatech was never cut short (to ensure it is always 100us long). It’s not that the Novatech is enforcing it’s clock limit on the digital flag, but that the interaction between the clockline for the novatech and the clockline for the digital flag must be taken into account when determining the set of instructions for the pseudoclock. In your example in the other thread, you are attempting to change a digital flag 40us after a call to update the Novatech. As the pseudoclock is common between the Novatech and the device controlling the digital gate, this necessitates that the Novatech clock pulse is only 40us long (20us high, 20us low), which is a violation of it’s specified clock rate. Now, in the case of the Novatech, because the pulse only needs to be high for 100ns, and low for 100us, the previous code worked for you even if the pulse was shortened (because it was held low for the appropriate length of time when the clockline was not is use for subsequent instructions). But that’s a very device specific example of something that happened to work, even though we were violating our own rules. Other devices are not so forgiving, and this fix was to identify (and prohibit) timing issues that were causing instructions to be missed on the G99 apparatus. The only ways to work around this are awful to implement:
The preferred solution would be to put your Novatechs on a separate pseudoclock. But I realise that may not be feasible for everyone. I’m obviously not going to be able to work on this any time soon, so I assume someone else will be making the changes, but my preference would be to not remove these extra checks because they serve a purpose ensuring the ‘gated clocks’ feature does not bypass the specified (symmetric) clock rate of a device. |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). Thanks Phil for the detailed explanation. This only makes sense to me though if the pseudoclock is common between the novatech table-advance line and the digital gate. But the digital gate is on an NI card, which I believe is on a different pseudoclock. I am pretty sure, in fact, that the novatech table-advance clock line is a single-purpose pseudoclock - ie the pseudoclock goes straight from a pulseblaster flag to the DDS9m and nowhere else. But I will check this in the lab tomorrow. |
|
Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey). The last commit of your connection table (8 months ago) shows the NI cards and novatech on separate clock lines (digital flags) but not pseudoclocks (pulseblasters). Gated clocks was the best we could do given that the pulseblasters flags could not be independently controlled with separate instructions, but it is unfortunately not equivalent to separate pseudoclocks. |
|
Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington). The two devices don't share a 'clockline', but they do share a 'pseudoclock' if they are connected to the same PulseBlaster. I am not completely following the problem (I will read through it more carefully later), but I wonder if it's fair to say that perhaps the novatech's min update interval is simply not 100us when using symmetric clock ticks. Would it make sense to set it to ~200us so that it can survive no matter what other devices on the pseudoclock are doing? This could be overridable so that when using asymmetric clock tick (which are implemented and work fine for PulseBlasters), one could set a shorter min update interval. Or the Novatech could inspect the pulse width duration of its pseudoclock and work out its min update interval based on this (this would require all pseudoclocks to declare their pulse width duration - which could be the string 'symmetric'. Perhaps the new error checking code should only run if pulse widths are symmetric. I'll read more closely to make sure I understand what's going on, but something like the above seems like it could be the right direction for a solution. |
|
Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey). I don't think it's as simple as that because you need to check whether the digital low length was achieved because the clockline was gated for one or more instructions on another clockline. Assymetric pseudoclocks pulses wouldn't actually fix Lincoln's ‘bug’, but widespread support for them throughout the labscript processing chain might allow you to not raise an error because you can tell that the clockline is held low for long enough. And yes, technically the Novatech clocklimit should be halved for symmetric clock pulses if you go by the technical specs. @shjohnst and I have discussed it though and we were reluctant to change it given it seems to work…it will break a lot of people's code if we do change it. |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). Yes, wow. I had no idea our NI clock was constrained by the dopey novatech, but I guess it is. Both are on PB0. I suppose nothing moves very fast on our NI AOs and DOs. Just coils etc. Our analog inputs (NI) and alazartech analog inputs are off PB1, so I guess they tick faster. I don’t have the stomach to move our NI card to a different PB and will just sit on the older rev for now. |
|
Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey). It’s only constrained within 100us of Novatech instructions though, so you might have fast things in your labscript file that work because they are far away (temporally) from Novatech commands. |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). So a low-fuss solution would be to move the novatech enable line to a flag on PB1…? Do we support PB flags as DOs or only as pseudoclocks? |
|
Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey). Pulseblasters support flags as DOs or clocklines. But it would have to be the NovaTech clockline that moved to PB1, not the enable line (which could stay on PB0) |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). But PB1 is running some things that need to be clocked fast at present… so surely the enable line should go there, and the novatech clock line should stay on PB0 where we can tolerate things being slowed down by it? |
|
Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey). Sorry, I thought you were talking about the table enable line, not the DDS enable line. Yes, if you only have an issue with one DDS enable line being used too close to Novatech instructions, then moving it is a good idea. DigitalOuts (including DDS enable lines) can be connected to pulseblasters by using the |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). Great, thanks Phil (and Chris!) for the super support here. I’ll try this tomorrow and if it works with on the current labscript revision I’ll upvert it to tip and test it with the gated clocks patch. |
|
Original comment by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner). No joy. It wasn’t just the novatech enable line that was ticking too fast, there was also a camera trigger on another NI DO. Not enough spare PB flags to move all of them as I think our PB is limited to 4 flags due to a firmware ‘upgrade’ a long time ago. I then thought about putting the NI trigger onto PB1. But labscript didn’t like this, because WaitsMonitor (using a NI DO attached to a ctr) has to be on the master PB (PB0). Final thought was to put novatech table clock onto PB1 and make PB1 ‘slow’. The other flags run off it could be made slow without too much pain. But the DDSes on PB1 need to be fast as they run rf pulses. And I suspect DDS ‘tick rate’ is also limited to slowest clockline? Can’t see how to make this ‘feature’ run in spinor lab… |
|
Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington). The commit 5618445 is backed out in all JQI/NIST labs, as shots reliably break with that change. Perhaps they unreliably break without the change, but unfortunately it looks like the cure is worse than the disease. We haven't stamped labscript with a release since then, so the issue isn't being experienced by everyone, but I think we shouldn't push out a release with this commit in it. How do you feel about it being reverted @philipstarkey? I don't know what the fix will be for the original issue, but I think we should back out the change even if we don't know what the alternative is. |
|
Original comment by Philip Starkey (Bitbucket: philipstarkey, GitHub: philipstarkey). What about just changing the added exception to instead print a critical warning in the terminal so it doesn't prevent compilation? Then we could work on a better long term solution which addresses the issue for the NovaTech (which is the only device having difficulty with this change AFAIK) I'm reluctant to revert entirely because we were bitten by the bug this patch corrected and that bug manifests itself as missed instructions which is not something you want to hide (and is very hard to track down). |
|
Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington). Yeah, that sounds like a good idea! Better than removing it entirely. You want to do it, or should I? It might be better as a print to stderr rather than using the |
|
Original comment by Philip Starkey (Bitbucket: philipstarkey, GitHub: philipstarkey). You had better do it I think. I’m swamped with work at the moment and am also behind with getting the BitBucket-GitHub exporter going (it’s coming along quite nicely, but still a few key things to do!) |
|
Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington). No worries, will do! |
…to specify a minimum clock high time in order to allow asymmetric clock ticks when combined with gated clocks on a pseudoclock. `minimum_clock_high_time` defaults to half of the minimum time between `IntermediateDevice` instructions. It be backwards compatible with previous versions of labscript devices. Fixes labscript-suite#51. I also discovered a bug where the check against the next all change time did not capture the last change time on the clock line because the stop time had not yet been added. This is now fixed. There were also some issues with various error messages. I've fixed those too and moved to use f strings so they're more readable. The whole `Pseudoclock.collect_change_times` method has also been reformatted in line with how `black` would format it (since I was working on it anyway). Some commented out code was also removed here too.
commit 82f2b7e Merge: 43d9901 727caf1 Author: David Meyer <[email protected]> Date: Fri Aug 5 16:14:04 2022 -0400 Merge pull request #94 from dihm/update-workflow Bring workflow up to date with sandbox commit 727caf1 Author: David Meyer <[email protected]> Date: Wed Aug 3 13:22:55 2022 -0400 Bring workflow up to date with sandbox commit 43d9901 Merge: b67bc30 c48a1f2 Author: David Meyer <[email protected]> Date: Thu Jun 2 10:36:50 2022 -0400 Merge pull request #92 from dihm/fix_docs_build Bump sphinx pin and update intersphinx links commit c48a1f2 Author: David Meyer <[email protected]> Date: Thu Jun 2 10:33:45 2022 -0400 Bump sphinx pin and update intersphinx links commit b67bc30 Merge: efa479a d554160 Author: Chris Billington <[email protected]> Date: Wed Jun 1 09:38:08 2022 +1000 Merge pull request #90 from ispielma/NoHG Change default of save_hg_info to commit d554160 Author: Ian Spielman <[email protected]> Date: Tue May 31 13:45:02 2022 -0400 Change default of save_hg_info to commit efa479a Merge: de6fb95 a9888d5 Author: David Meyer <[email protected]> Date: Mon May 30 07:29:54 2022 -0400 Merge pull request #88 from philipstarkey/philipstarkey/issue51 Introduce a new attribute for `IntermediateDevice` defining the minimum clock high time commit de6fb95 Merge: 763a0f1 aadd476 Author: David Meyer <[email protected]> Date: Sat Feb 5 06:01:08 2022 -0500 Merge pull request #87 from dihm/docs_mock_imports Mock `labscript_utils` to prevent RTD from trying to launch zprocess. commit a9888d5 Author: Phil Starkey <[email protected]> Date: Sat Feb 5 20:31:56 2022 +1100 Introduce a new attribute for `IntermediateDevice`s that allows them to specify a minimum clock high time in order to allow asymmetric clock ticks when combined with gated clocks on a pseudoclock. `minimum_clock_high_time` defaults to half of the minimum time between `IntermediateDevice` instructions. It be backwards compatible with previous versions of labscript devices. Fixes #51. I also discovered a bug where the check against the next all change time did not capture the last change time on the clock line because the stop time had not yet been added. This is now fixed. There were also some issues with various error messages. I've fixed those too and moved to use f strings so they're more readable. The whole `Pseudoclock.collect_change_times` method has also been reformatted in line with how `black` would format it (since I was working on it anyway). Some commented out code was also removed here too. commit aadd476 Author: David Meyer <[email protected]> Date: Fri Feb 4 08:51:54 2022 -0500 Re-attempt to mock `labscript_utils` import so autosummary can work. commit 755e562 Author: David Meyer <[email protected]> Date: Fri Feb 4 08:40:06 2022 -0500 Reset pins, use importlib_metadata to get version in `conf.py`. commit d5750bb Author: David Meyer <[email protected]> Date: Thu Feb 3 14:36:46 2022 -0500 Remove zprocess pins and pin labscript-utils to last known working version. commit a5b195d Author: David Meyer <[email protected]> Date: Thu Feb 3 14:29:19 2022 -0500 Back to last known working zprocess version. commit 9294f98 Author: David Meyer <[email protected]> Date: Thu Feb 3 14:27:16 2022 -0500 Change pin back on more. commit d9b0cd0 Author: David Meyer <[email protected]> Date: Thu Feb 3 14:24:39 2022 -0500 Remove mock, test which version of zprocess is causing change by temporarily pinning it. commit 60d21fa Author: David Meyer <[email protected]> Date: Thu Feb 3 14:03:01 2022 -0500 Mock `labscript_utils` to prevent RTD from trying to launch zprocess.
Changed default behaviour of exponential ramps when "truncation" keyword argument is not set. Approved-by: Chris Billington
Original report (archived issue) by Lincoln Turner (Bitbucket: lincolndturner, GitHub: lincolnturner).
Marked as critical because it’s in mainline and a timing issue in hardware many of us use.
I initially reported this as labscript_devices issue #35 (labscript-suite/labscript-devices#35) thinking it was Novatech DDS9m specific…
…
So I reverted to
f4405d8a8ad5aedacf37ae2dc282d529a0b9b21f, removing the GatedClocks-bugfix merge.Then all is well!
Shots compile. And better, they run and produce nice fluoroscence images!
And indeed @philip Starkey 's commit
56184458fbfdf04e5f3da6c9e19267c4e1d796b4does seem to edit code in and around this error message…See closed issue linked above for details…
The text was updated successfully, but these errors were encountered: