Skip to content
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

Add fixed tones support #440

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

Add fixed tones support #440

wants to merge 5 commits into from

Conversation

tristpinsm
Copy link

@tristpinsm tristpinsm commented Sep 9, 2024

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
Copy link
Collaborator

jlashner commented Sep 9, 2024

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
Copy link
Author

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

@@ -363,6 +363,35 @@ def setup_tune(S, cfg, bands, show_plots=False, update_cfg=True):
return True, summary


def setup_fixed_tones(S, cfg, freq, tone_power=10, update_cfg=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

# ensure fixed tones are on
for band in S.bands:
if "fixed_tones" in cfg.dev.bands[band]:
uxm_setup.turn_on_fixed_tones(S, cfg, band)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@tristpinsm
Copy link
Author

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
Copy link
Author

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.

@tristpinsm tristpinsm marked this pull request as ready for review October 2, 2024 21:19
Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

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

sodetlib/operations/tracking.py Outdated Show resolved Hide resolved
sodetlib/operations/uxm_setup.py Outdated Show resolved Hide resolved
sodetlib/operations/uxm_setup.py Outdated Show resolved Hide resolved
sodetlib/operations/uxm_setup.py Outdated Show resolved Hide resolved
sodetlib/operations/uxm_setup.py Outdated Show resolved Hide resolved
# ensure fixed tones are on
for band in S.bands:
if "fixed_tones" in cfg.dev.bands[band]:
uxm_setup.turn_on_fixed_tones(S, cfg, band)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Author

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.

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