simonsobs / sorunlib

High level library for running observatory operations using OCS.
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

wiregrid.py: add configurable motor current, zenith tag, and increase reference rotation prior to calibration #154

Closed sadachi5 closed 3 months ago

sadachi5 commented 5 months ago

I changed the three points for wiregrid.py:

  1. Change the El requirement: 50 deg --> 60 deg
    • Because the nominal elevation value was changed to 60 deg.
  2. Add loose_el_check argument in the calibration() function.
    • In order to loosen the elevation requirement for the commissioning test. Especially, in the commissioning, we often want to perform the calibration at zenith.
    • The requirements in loose mode: > 59.5 deg
  3. Change time period for the continuous rotation before the calibration because the rotation speed can be slow sometimes.
    • 5sec --> 10 sec
sadachi5 commented 5 months ago

Hi Brian, Thanks for your comment.

As for safety, the dangerous elevation for the grid loader is less than 50 deg. If the elevation is lower than that, the linear actuator is getting steeper, and falling due to gravity could occur.

I agree with your idea. We don't need the loose_el_check argument if we don't require el=60. Instead of El=60 deg check, I added the wg_nominal_el tag to distinguish a correct-elevation run easily. How do you feel about this strategy?

sadachi5 commented 5 months ago

@BrianJKoopman As you know in socs repo, we need to modify the KIKUSUI current value. We want to change it from 3A to 4A for SATp3. For satp1, we want to keep it and I want it to change the current for each satp. Could you fix the implementation to get the satp name in L106?

BrianJKoopman commented 5 months ago

@BrianJKoopman As you know in socs repo, we need to modify the KIKUSUI current value. We want to change it from 3A to 4A for SATp3. For satp1, we want to keep it and I want it to change the current for each satp. Could you fix the implementation to get the satp name in L106?

Hi @sadachi5, sorry for the slow reply, been preparing for SPIE. This implementation encodes configuration information into the library. I'd rather add a wiregrid current configuration field to the configuration file and pull from there. I need to think a bit about how I want to do that, will try to get to that within the next week or so, likely I will restructure the configuration file a bit to separate the agent instance-id's from device specific configuration like this current.

I agree with your idea. We don't need the loose_el_check argument if we don't require el=60. Instead of El=60 deg check, I added the wg_nominal_el tag to distinguish a correct-elevation run easily. How do you feel about this strategy?

Great, thanks for updating that. I'm okay with the tag, though I wonder if it's necessary? If "nominal" elevation changes again, then the tag would seem untrustworthy compared to also loading the elevation for inspection during analysis. But if it helps with analysis, again, fine to keep.

sadachi5 commented 3 months ago

Hi @BrianJKoopman,

I missed your last message. As for the current setting, I'm fine that the current for each SAT is managed in the configuration file. But, unfortunately, I don't know about the configuration file. Could you implement that function in this script and the configuration files?

For the el tag, I understand your concern. I changed the tagging procedure. We only tagged for el=90, which often happens during the commissioning and el=90 could change the data behavior compared to that at el=60 deg. So, I want to tag it so as not to miss the configuration of each data in the analysis.

sadachi5 commented 3 months ago

Hi @BrianJKoopman

We want to test this code next week (if we can, we want to do that on Sunday). To approve the El limit change as soon as possible, I commented out the settings for the current temporary. Please go ahead and merge as soon as possible.