Skip to content

AD9959 DDS Sweeper #126

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

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

carterturn
Copy link
Contributor

No description provided.

@carterturn
Copy link
Contributor Author

A result from testing in the lab: we should either remove timing mode or implement it fully. At the moment, the labscript device just sends an instruction table without timings (regardless of whether timing mode is 1 or 0), which fails. I think I am leaning towards removing it, but I could go either way.

On a related note, we should check that the number of bytes we want to send matches the bytes the Pico is ready for.

…n binary mode.

At the moment, if it fails it will simply send all zeros and throw an error.
I think this is the best option, as it prevents the device from getting locked up waiting for bytes (if we restart the blacs tab).
@carterturn
Copy link
Contributor Author

I added the "ready for ... bytes" check and an option to use an external reference clock for the labscript device. These should be tested before merging.

@Json-To-String
Copy link

Talked a tad with David and we settled on not supporting internal timing at all since the prawnblaster clockline is much better for less work. I'll remove those and try to add any remaining docs

Copy link
Contributor

@dihm dihm left a comment

Choose a reason for hiding this comment

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

Mostly minor tweaks, with two more significant things

  1. I am now noticing that we basically have a firmware limitation that there must be at least one dynamic output to use this device. I get why and it isn't totally unreasonable, I do think it could be worth allowing for all the outputs to be StaticDDS.
  2. The BLACS worker transition functions feel a bit rough. They don't quite respect the conventional boundaries (which are admittedly poorly documented) and I think transition_to_buffered has a number of edge case issues that need to be tested and fixed. I would appreciate testing confirmation that everything works as expected if you do the following things:
  • Program no outputs
  • Only use StaticDDS
  • Only Use DDS
  • That the BLACS tab updates to the final values of all outputs correctly
  • That aborts leave the device in a functional state

@carterturn
Copy link
Contributor Author

I found a rather insidious bug: if two instructions are spaced by less than the reprogramming time (10us for 4 channels), the trigger for the second instruction arrives before the DDS Sweeper has finished reprogramming the AD9959. I think the result of this is a partial update of the AD9959 parameters.

For an example of this, we were trying to use the DDS Sweeper to pulse on an AOM for 4us, 8us, and 12us. In the first two cases, the second instruction doesn't run, and the AOM never turns off.

@Json-To-String
Copy link

I've added some comments to comments (mostly for me) to make sure everything in the review got addressed. Carter and I met and talked about filling out some more docs but otherwise I think we should be looking good.

Copy link
Contributor

@dihm dihm left a comment

Choose a reason for hiding this comment

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

Few more things, but getting much closer!

In any case, I'll probably hold off on merging until Carter has had a chance to run with these changes in lab for a bit to try and catch any remaining issues.

@carterturn
Copy link
Contributor Author

set_output uses floats (and by extension, so do StaticDDSs). Is this what we want or should we add something like a seti_output to the DDS Sweeper firmware?
If we do stick with just set_output, how should the *_scale_factor device properties be handled for static DDS only devices? This is mostly relevant for connection table comparisons, right? In that case, I would be inclined to only set those when we have a dynamic DDS where we have to do scaling to get to and from SI units.

Copy link
Contributor

@dihm dihm left a comment

Choose a reason for hiding this comment

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

I like how y'all solved these outstanding issues. I have a couple more minor things to address.

Copy link
Contributor

@dihm dihm left a comment

Choose a reason for hiding this comment

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

(Hopefully) one last patch that I just took a stab at instead of pushing it off on you both.

Copy link
Contributor

@dihm dihm left a comment

Choose a reason for hiding this comment

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

I believe I have run out of things to say on this one. Going to approve. Will wait to merge until extended testing in lab is reported.

@carterturn
Copy link
Contributor Author

Mostly working so far, but I did manage to get it to crash while running an unusual sequence earlier today. I want to investigate that a bit further. It seems to have mostly affected one on a remote BLACS, so it might have been a network issue.

@carterturn
Copy link
Contributor Author

Mostly working so far, but I did manage to get it to crash while running an unusual sequence earlier today. I want to investigate that a bit further. It seems to have mostly affected one on a remote BLACS, so it might have been a network issue.

I was unable to recreate the issue, so it may have been an issue with our lab's network. I am satisfied.

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