More Set Commands and Binary Transfers

[carterturn]

This pull request attempts to resolve #35 and #44.

First, set_ins is split into different functions for different modes. Each of these functions is then split into a floating point to integer function (used by the set command) and an integer to SPI instruction function (used by the 'seti' command and binary programming).

Second, the seti (set-integer) command is added. This allows instructions to be programmed with integer values. The set command then has its separate delta and rate parameters combined into a single sweep_rate floating point parameter. This could be done in a way that allows for greater precision, but for the sort of testing set is likely to be used for, it may be fine as is.

Finally, the setb (set-binary) command is added. This allows instructions to be programmed in a large block without ascii encoding. The number of instructions is sent first, allowing the Pi Pico to check if there is enough memory available.

[carterturn]

I have tested set and seti in all modes, but setb has only been tested for single step mode. There, it allows filling Pi Pico RAM in <0.3s (which I think corresponds to a ~3x higher transmission rate than PrawnDO).

Documentation is not up to date yet. I think I should also refactor this a bit. It might make sense to move processing for the set, seti, and setb commands into their own functions and see if there is a way to reduce serial command error checking code duplication, as dds-sweeper.c is becoming a bit long.

[dihm]

Apologies for being slow @carterturn. I have finally had a moment to look at this. Overall I think this is a huge leap forward. The process is fairly easy to follow (or at least as easy as it could be, the logic for all the modes is pretty deep) and I greatly appreciate that higher level abstraction functions (like set) re-use all the code from the lower level ones, which should make debugging and testing quite a bit easier.

Mostly to ensure I am following along, I'm going to try to document the call chains:

• set: highest level of abstraction, user sets inputs with floats
  • single step mode
    • get_asf, get_ftw, get_pow: converts floats to Amp scale factor, freq. tuning word, phase offset word
    • calls lower level set_single_step_ins and set_time to move to memory
  • sweep modes
    • parse_amp_sweep_ins, parse_freq_sweep_ins, parse_phase_sweep_ins
• seti: intermediate level of abstraction, user sets tuning words, but in ascii
  • single step mode
    • set_single_step_ins, set_time: puts tuning words into instruction memory
  • sweep modes
    • set_amp_sweep_ins, set_freq_sweep_ins, set_phase_sweep_ins: puts tuning words and sweep words in memory
• setb: lowest level of abstraction, user pipes binary data directly into memory
  • Same instructions as seti, but directly copied off the buffer

I'm happy to follow your lead on refactoring, since you are the one mucking around and know what the pain points are. I agree that some refactoring is probably warranted. Personally, I'd consider moving the the set_xxxxx_ins, parse_xxxxx_ins, and set_time commands into a new file, though I understand if that is too annoying since the instruction memory pointer would need to be tracked differently. In any case, it does seem like much of the setb logic could be delegated to a similar set of functions to help shorten the code there.

Finally, I may be much mistaken, but I believe the set and seti sweep modes ignore the time parameter in all cases.

[dihm]

Also, I am pondering how to be more helpful. I suspect getting into the weeds of the firmware is not going to be all that productive, and likely result in longer delays as I try to keep up with you.

I'm thinking it might be better for me to focus on setting up our simultaneous amp/freq/phase test rig and developing out a fuller suite of much more automated tests. I'd start with the tests.ipynb in the repo and work from there. If agreeable, I'd probably only have to lean on you for initial code snippets for creating the binary commands and such.

[carterturn]

Thank you for the feedback so far.

Those are the intended workings of the call chains, glad that makes sense (I should still add a few more comments so it does not have to be re-derived). You are correct that I neglected the set_time call for the sweeps for set and seti; I will get that fixed soon (it should be trivial).

I think the largest functional refactor I would consider doing at this point would be to parameterize and then combine the serial parsing code. However, this seems more complex for no benefit besides reducing the amount of code to read through, and I am not sure how much that could happen given that human readable strings must be generated. Given that, it probably makes sense to keep the serial parsing as it is and move some of the parsing/setting functions into a different file. Perhaps it would even work well to move instructions into its own file (with the associated "parsing" and "setting" functions), then add "getting" functions to retrieve instructions? This might also improve future edit-ability by making the scope of instructions clear.

More automated testing would be very beneficial! I have a BLACS worker for stepping mode here: https://github.com/carterturn/zwierlein_labscript_user_devices/blob/ad9959-eval/AD9959EvalDDS/blacs_workers.py. I think it should be fairly similar for the sweeps (though that is actually entirely untested).

It would probably be good to also add a "get" command for testing. I'll do that too in the next few days.

[dihm]

@carterturn I have finally found a small amount of time to set up a test rig. I am hitting a perplexing bug trying to use the internal timing for testing.

send('reset')
send('mode 0 1')
send('setchannels 1')

print(send('set 0 0 100000000 1 0 500000'))
print(send('set 4 1')) # this works fine

send('reset')
send('mode 0 1')
send('setchannels 1')

print(send('set 4 1'))  # this fails
print(send('set 0 0 100000000 1 0 500000'))

The failure produces the invalid command string 'Missing Argument - expected: set <channel:int> <addr:int> <frequency:double> <amplitude:double> <phase:double> (<time:int>)\nok\n'

This matters because I'm trying to send the table with a binary write, then use the send command to set the stop instruction, which doesn't work.

Of course, maybe this isn't the correct method and I should figure out how to send the stop instruction as part of the binary write. I've tried setting all zero instructions at the end of the table, but I don't appear to be getting anything out. Maybe because I don't understand how to actually format the instructions to include timing information for multiple channels in the first place?

Best guess is that I am bumping up against a bunch of edge case issues all at once, which isn't surprising given how many things this firmware can possibly support and a general lack of detail documentation.

In any case, I am thoroughly confused and going on vacation for 1.5 weeks so not much I can do about it now. When I get back, I'll try to scratch out some more time to actually set up all the testing scripts. In general, I'd like to set up labscript-agnostic ones that leverage the internal timing and manual programming of instructions, then labscript tests which use external timing and labscript programming. In both cases, I'd like to automatically capture scope traces that demonstrate functionality since there are going to be a fair number of things to test and manually going through them all feels painful.

Ideally, once I get into the thick of this, I'm going to find bugs that need fixing. But I may not be well equipped to do that myself. Is there a time in the near-ish future where you would be available to iterate though things quickly?

[carterturn]

That bug turned out to be a combination of two bugs: an improper argument list check and lack of serial buffer clearing. It should work now.

At the moment, it will not be possible to set stop or repeat commands through the binary interface, as it does not have any way of sending metadata. I think it would be roughly the same amount of total data transmitted to include 2 bits of metadata (stop/repeat) for each command or to send a single non-binary set/seti at the end. I think the latter is simpler to encode/decode, so I would prefer that method.

I will put substantially more time into the documentation this week; I can see why the stop command is hard to determine.

That sounds like a good plan for testing. How quick iterations would you like to aim for? I can quite easily keep a closer eye on this and respond to issues within ~6 hours; it would also be possible to schedule something more real time if that would work better.

[dihm]

Awesome! Thanks for fixing that. It no doubt would have taken me a while to find that buffer issue.

I agree that insisting on the stop/repeat commands being sent with set/seti is the best course of action. It's much simpler to support and barely adds any overhead.

Documentation help would be appreciated. Just looking at the code, it isn't obvious to me that internal timing works for anything but channel 0. Maybe I'm missing something, but my understanding is that the updates for all channels have to by synchronous, so there is only one true timing per step, but the code will attach a timing argument to each channel command. If that's true, I'm not entirely sure how to fully support internal timing on all channels easily and am tempted to relegate the feature to a separate firmware.

Finally, my hope is for iterations to be fairly quick once basic testing functionality is working. But that will take a bit of work on my end. I also have a new researcher that started yesterday (@Json-To-String) who I'm hoping to spin up to take on my backlog of labscript development tasks and I think this project is a decent place to start. So it may be a week or two until he is up to speed and ready. In any case, I think 6 hour turn around is more than good enough for now. Once we are up to speed, I think it would be good to set up a weekly 30min meeting to get a little real-time feedback and get this out the door (and off our plates) quickly.

[carterturn]

Good, I will not change the stop/repeat behavior for now.

At the moment, and for past versions, I think the program expects to send an SPI command for every channel at each (internal or external) time point. However, those SPI commands could be empty (though this is not exactly properly supported, as an SPI command with all zeros might change the SPI mode). Furthermore, it is designed to not necessarily send profile pin triggers at every time point. Thus, it should be possible to start a sweep on one channel, wait part of the sweep, then start another sweep on another channel while the first sweep is still in progress.
A caveat to all this is: there is not a way to zero the instructions array, so once a sweep is written to a certain channel/address, it will stay there. It would be fairly straightforward to add a set [addr] [channel] null command special case, but I will think a bit more about how to nicely handle this with setb.

[Json-To-String]

wow, incredible catch, I think my idea that it was "working the first time" might be wrong if this was the issue. the signals clear so quickly after the screen draws for the first time that I must be missing the behavior - i'll try to get this tested soon