Add fixed tones support

[tristpinsm]

This is a draft with a proposed implementation for supporting fixed tones. If this looks good, I can proceed to testing on the SLAC system and then polish things up / add docstrings.

Right now it relies on a new function in pysmurf (slaclab/pysmurf#796) because I thought that would be a more complete implementation, but if it's preferable to keep these changes within sodetlib I can move it into this PR.

[jlashner]

Hi Tristan, this looks like a good start! A few comments:

• I think (band, channel) are not enough to determine fixed tone location, and we should also include the frequency or the freq relative to subband center. I kind of think just specifying and saving the (frequency, band) or (frequency, AMC) information is easiest because from there you can get the band and channel info.
• I think the "setup_fixed_tones" function should also figure out the optimal frequency of the fixed tones, since we don't expect users to know this. I imagine it should take some behavioral parameters like fixed_tones_per_band and min_fixed_tone_spacing and then just figure out the frequencies based on that, and save them to the device cfg.
• The defaults for the device cfg are recorded here. Anything we add to the device cfg should be added here, and then in functions we can assume these keys will always be present in the cfg. We can make it so the fixed_tones entry is an Optional[dict], and only set to a value after the setup function is run.

One more thing is that recently I've found type hinting and static type checking to be very helpful for code readability and catching bugs early. For instance, it would be a lot clearer if the freq arg of setup_fixed_tones was hinted as List[float], so its clearer that it should be a list and you can't pass a single value. I'm not going to require this for this PR since the rest of sodetlib doesn't follow it, but thought I'd point it out because I think its really nice!

[tristpinsm]

Thanks for the rapid feedback! That all makes sense and I'll make those changes

[msilvafe]

We probably also need an automated procedure to find the frequencies to use based on gaps where resonances don't exist from the tunefiles or find_freq results.

[msilvafe]

I'm not sure this will work since you need to run tracking_setup after turning on fixed tones.

[jlashner]

I think we should just figure out how to enable fixed tones without tracking setup... I imagine there's just a register or two that needs to be set.

[tristpinsm]

I had looked through tracking_setup and it wasn't clear to me what would be missing, but in any case I will have to go through all of these steps with testing and I should find out then.

[tristpinsm]

I added a turn_on_fixed_tones call to relock_tracking_setup so that if it disables these channels they should be restored

[jlashner]

I think we still need an additional enable_fixed_tones cfg entry which allows us to disable fixed tones without removing the fixed-tone dictionary.

[tristpinsm]

I just pushed a new draft addressing some of the suggestions so far:

• Now it sets the channel frequency offsets
• These are saved along with the channels in the config
• I cobbled together an algorithm for picking fixed tone frequencies, but I guess we should consider it a placeholder until I can do some actual testing

After realising that you need the subband frequency offsets, I moved all the steps into the sodetlib functions, as I didn't see an obvious series of steps to do this within the existing pysmurf functions.

[jlashner]

Sorry should've mentioned this, there is already an algorithm in pysmurf that we tend to use when doing this manually:
https://github.com/slaclab/pysmurf/blob/1ff66325bf174f975c241b5cf6922f378dd58b20/python/pysmurf/client/util/smurf_util.py#L4361

[jlashner]

We may want to make something custom though that is based on an existing tunefile and doesn't need to rescan.

[tristpinsm]

Sorry should've mentioned this, there is already an algorithm in pysmurf that we tend to use when doing this manually: https://github.com/slaclab/pysmurf/blob/1ff66325bf174f975c241b5cf6922f378dd58b20/python/pysmurf/client/util/smurf_util.py#L4361

Oh man, I hadn't seen that! I'll take a look and see how it could fit in.

[tristpinsm]

I tried to address the suggestions so far and did some testing on the SLAC UFM today. I think functionally things are working now, and next I will polish up the code and commit history and add docstrings.

[jlashner]

Thanks for the changes tristan! I have a few more simple requests and then I'm happy to merge (assuming its been tested)!

[jlashner]

I think we still need an additional enable_fixed_tones cfg entry which allows us to disable fixed tones without removing the fixed-tone dictionary.

[tristpinsm]

Thanks for the changes tristan! I have a few more simple requests and then I'm happy to merge (assuming its been tested)!

Thanks for the suggestions -- I'll make those changes. I've tested the individual functions, but not in a full OCS context with agents running. But I did want to try to get something like that working.

[tpsatt]

Thanks for all this work @tristpinsm! Wondering if it's worth adding a call to turn_on_fixed_tones in uxm_relock that way this change can fit nicely into our current schedules?

Also should feedback be disabled in tracking.setup_tracking_paramters too in case relock_tracking_params doesn't get called?

[tristpinsm]

Thanks for all this work @tristpinsm! Wondering if it's worth adding a call to turn_on_fixed_tones in uxm_relock that way this change can fit nicely into our current schedules?

The way it is now, it is called in stream_g3_on in order to guarantee the fixed tones are set during data-taking. Are you thinking of scenarios where fixed tones would need to be set but we are not streaming data? Like an interactive session or something?

Also should feedback be disabled in tracking.setup_tracking_paramters too in case relock_tracking_params doesn't get called?

relock_tracking_params is called as the last step of setup_tracking_params so I think this is covered.

It doesn't look like I got around to changing the code for finding fixed tone channels to use existing functions -- I can take another look at that.

[tpsatt]

Ah I'd missed both of these points. Thanks @tristpinsm! Looks good on my end to rebase and merge

[msilvafe]

Ah I'd missed both of these points. Thanks @tristpinsm! Looks good on my end to rebase and merge

Let me take one last look over if that's ok @tpsatt and @tristpinsm.

[msilvafe]

This looks good, I think we should fix the broken sphinx docs build also but probably not a requirement here. I think you'll need to re-request review from @jlashner and have him approve (hopefully he doesn't charge us his google rate!)

[jlashner]

I'm gonna approve so that you don't have to wait on me to merge, this mostly looks good!

One potential problem I see is that "turn_on_fixed_tones" can disable the feedback on active channels. This might be an issue, since the fixed-tone array in the device cfg is not reset when a new tune is taken. So its possible that a new tune is taken, the channel assignment of the new tune is slightly different so it conflicts with existing set of fixed tones, and then the turn_on_fixed_tone function overwrites real channels.

You may want to make it so that a set of fixed tones is tied to a specific tune-file, and if that tune-file is changed the fixed-tones need to be reset or they won't apply.

That'll be $10 :)

[tristpinsm]

thanks for approving and the suggestion @jlashner !

I've implemented that now, so when we're ready to test @msilvafe we should make sure to pull this commit.