search

slaclab / pysmurf

Other
2 stars 9 forks source link

No check on drain power supplies before set_amp_defaults in S.setup() #724

Closed dpdutcher closed 1 year ago

dpdutcher commented 1 year ago

https://github.com/slaclab/pysmurf/blob/d7328818f4d2a25d9a8871ef9ceccfa643f55e41/python/pysmurf/client/base/smurf_control.py#L644-L646

According to the set_amp_defaults doc string, it assumes the drain power supplies are disabled, but there is no check or command here to ensure that is the case. Running S.setup() with amps that are already powered can have undesirable effects. Recommend the drain power supplies are disabled prior to this call to set_amp_defaults.

dpdutcher commented 1 year ago

@swh76 Here's something on the pysmurf front that should be a relatively easy fix. I don't think it will hamper things in the way SAT-1 testing is proceeding with Jack at the helm, but it did confuse LATR folks quite a bit.

swh76 commented 1 year ago

Thanks @dpdutcher for reminder. I'll plan to address this too in upcoming release.

swh76 commented 1 year ago

A couple of questions on this one.

1) Why is setup being run with amps that are already powered? FYI setup must only be run once per board power cycle. Or is the issue that folks are using set_amp_defaults independently of setup (which we should make work without causing any problems)? FYI there is a software reg that records if a system has already been configured (AMCc:SmurfApplication:SystemConfigured) but unfortunately because it's a software reg (ie, not defined in fw), it's not persistent if the rogue server is restarted. So we could add a check for that in setup but it wouldn't prevent rerunning setup if the rogue server has been restarted. Maybe I will do that anyway, with an override flag so users have to force it to rerun setup if the system has been configured but the rogue server hasn't been restarted.

2) Looking at set_amp_defaults it's not obvious to me why there'd be an issue rerunning this function if the amps are already powered on. Can you say more about the problems they were seeing? For the production SO cards, the function runs this logic;

https://github.com/slaclab/pysmurf/blob/54f3e9527b8f83221e05fda005421d62211a6f99/python/pysmurf/client/command/smurf_command.py#L4556

the issue might be the now deprecated set_rtm_slow_dac_enable(dac_num,0x2) function calls (low level DAC enable is now handled in setup by the call to set_defaults_pv, which must not be run twice).

I'll put this on an oscilloscope and see what it's doing, and confirm that removing those set_rtm_slow_dac_enable calls doesn't break things (and hopefully fixes this issue).

dpdutcher commented 1 year ago

Sorry it took me forever to respond. I think the issue boiled down to two things: (1) The amplifier default values in the pysmurf config are wrong (2) setup being run multiple times per board power cycle. For (1), SO uses another layer of config file that sodetlib reads from and writes to. The pysmurf config never gets written to except by a user modifying it by hand. This does not happen all that often, and things like amplifier biases are especially unlikely to get updated. For (2), it is news to me that setup "must" only be run once per board power cycle. If a board were misbehaving, sometimes a call to S.setup() would be a way to try to fix it that occasionally worked. This does not happen often anymore. In the particular case that prompted this issue, setup was being called by an SO-side function that should probably not be running setup, so I'll request that change there.

In any case, I just tried this function again with a pysmurf config that did have the correct default values, and it worked as expected. The "undesirable effects" only happen if the defaults are way off. So, I think this can be closed.

swh76 commented 1 year ago

@dpdutcher reopening mainly because need to remove the deprecated set_rtm_slow_dac_enable(dac_num,0x2) calls here and elsewhere. After we've removed those let's close.