DM-39206 - Power on ATCalSys #116

isotuela · 2023-05-19T19:38:22Z

No description provided.

tribeiro

Here some initial comments.

tribeiro · 2023-06-05T16:44:09Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        #     self.log.info(f"Lamp didn't turn on after {self.lamp_warm_up_time} s")
+
+    async def wait_for_lamp_to_warm_up(self):
+        await self.white_light_source.evt_lampState.flush()


flush is a regular method not a coroutine. You should drop the await.

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

tribeiro · 2023-06-05T16:46:15Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+            )
+            if lamp_state.basicState == ATWhiteLight.LampBasicState.WARMUP:
+                warmup_time_left = (
+                    lamp_state.warmupEndTime - lamp_state.private_rcvStamp


I think you probably want to use private_sndStamp instead of private_rcvStamp. Difference should be small but still.

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

tribeiro · 2023-06-05T16:48:58Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+                self.log.info(
+                    f"White Light Lamp is on after a warm up of {warm_up_elapsed_time} s"
+                )
+        else:


Is this correct? if the status is not WARMUP nor ON it will fail?

Also, if it is a fail, shouldn't you raise an exception or something?

tribeiro · 2023-06-05T16:49:35Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+                    "Time Left for lamp warmup: {} min.".format(warmup_time_left / 60.0)
+                )
+                await asyncio.sleep(self.track_lamp_warmup)
+            elif lamp_state.basicState == ATWhiteLight.LampBasicState.ON:


I think you could drop this elif and just do this after the while loop has ended (it saves you from doing a comparison).

I have changed the code a little more.

Sorry, not sure I understand the logic here. Do you need to break in this condition? it seems like it is going to continue in the loop until is time out.

However, see my suggestion for simplifying this loop.

tribeiro

Here some comments for you to consider... I haven't been able to read through it all yet, sorry..

tribeiro · 2023-06-08T20:36:00Z

python/lsst/ts/standardscripts/auxtel/calibrations/__init__.py

+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+


Missing the final phrase:

# along with this program. If not, see <https://www.gnu.org/licenses/>.

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

tribeiro · 2023-06-08T20:37:49Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+
+from lsst.ts import salobj
+
+track_lamp_warmup = 60


If this is supposed to be a constant, please, make is all caps,

TRACK_LAMP_WARMUP = 60

But maybe best to have this as a class attribute instead, e.g.,. move it to the __init__ method and make it self.track_lamp_warmup.

tribeiro · 2023-06-08T20:38:08Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+    index : `int`
+        Index of Script SAL component.
+
+    Notes


If you don't have any Notes, you can remove this session.

tribeiro · 2023-06-08T20:39:49Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        self.timeout_chiller_cool_down = 60 * 10
+        self.chiller_temp_tolerance_relative = 0.1
+
+        if self.white_light_source is None:


I think you need to move all this code from here until line 86 to the configure method. Or maybe make a setup_remotes coroutine and await it in configure.

I tried moving it to __init__ because it didn't configure properly if it was in the configure method, and all unittest were failing quickly.

tribeiro · 2023-06-08T20:44:12Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+                log=self.log
+            )
+
+        asyncio.gather(


You need to await this otherwise nothing happens.. Once you move this to the configure method, you will be able to do:

await asyncio.gather(...)

tribeiro · 2023-06-08T20:45:25Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        #         domain=self.domain, name="ATMonochromator"
+        #     )
+
+        self.config = config


Above you are creating individual attributes to get the values from config and then here you just store the entire thing. I think pick one method. Either set them up individually or get the full thing, but not both.

tribeiro · 2023-06-08T20:45:53Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+
+    async def run(self):
+        """Run script."""
+        #await self.assert_components_enabled()


Please, remove commented code or uncomment.

Uncommented.

tribeiro · 2023-06-08T20:47:29Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        await self.white_light_source.cmd_startChiller.set(timeout=self.timeout_chiller_cool_down)
+
+    async def wait_for_chiller_temp_within_tolerance(self):
+        start_time_chill_time = time.time()


time is repeated here.. maybe rename it start_chill_time?

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

tribeiro · 2023-06-13T15:03:28Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+
+        Raises
+        ------
+        salobj.ExpectedError :


Usually we only document the exceptions that the method itself raises. This one in particular is raised by another method that does the configuration validation before calling configure. You should probably remove this here.

tribeiro

Here another round of comments..

tribeiro · 2023-06-13T15:04:42Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+            self.white_light_source = salobj.Remote(
+                domain=self.domain,
+                name="ATWhiteLight",
+                log=self.log,


Remote does not take log as a parameter.

tribeiro · 2023-06-13T15:04:58Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+            self.monochromator = salobj.Remote(
+                domain=self.domain,
+                name="ATMonochromator",
+                log=self.log,


Remote does not take log as a parameter.

tribeiro · 2023-06-29T16:11:10Z

doc/version_history.rst

@@ -32,6 +32,8 @@ v1.22.0

 * Add new ``MoveP2P`` maintel script.

+* Add new ``auxtel/calibrations/power_on_atcalsys.py`` script, unit test and executable to set up and turn on the ATCalSys white light. 


Version 1.22.0 was already released, please make this a new release 1.25.0

tribeiro · 2023-06-29T16:13:54Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+            )
+
+    async def open_white_light_shutter(self):
+        await self.white_light_source.cmd_openShutter.set_start()


Please, add a timeout to the command, otherwise it can stop here forever. Also, since you are not passing in any parameter, use, start instead, e.g,:

await self.white_light_source.cmd_openShutter.start(timeout=self.timeout_open_shutter)

and you need to define self.timeout_open_shutter in the __init__ method.

I have put 5 minutes timeout, does that sound right?

tribeiro · 2023-06-29T16:15:06Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+
+    async def open_white_light_shutter(self):
+        await self.white_light_source.cmd_openShutter.set_start()
+        # Q? Should we include a check that the shutter is open?


I think this command only finishes when the shutter is open, and fails if the shutter doesn't open.. So think it is ok to leave it as is, e.g., no need to check.

tribeiro · 2023-06-29T16:20:43Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+                    "Time Left for lamp warmup: {} min.".format(warmup_time_left / 60.0)
+                )
+                await asyncio.sleep(self.track_lamp_warmup)
+            elif lamp_state.basicState == ATWhiteLight.LampBasicState.ON:


Sorry, not sure I understand the logic here. Do you need to break in this condition? it seems like it is going to continue in the loop until is time out.

However, see my suggestion for simplifying this loop.

tribeiro · 2023-06-29T16:25:56Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        )
+
+    async def wait_for_lamp_to_warm_up(self):
+        self.white_light_source.evt_lampState.flush()


I think it is best to flush just before you send the command. Flushing here can create a race condition where the event is published between the time the command is sent and the time to get here, in which case you loose the event.

I would actually suggest changing the loop a little bit.

Consider something like this:

lamp_state = await self.white_light_source.evt_lampState.aget( timeout=self.timeout_lamp_warm_up ) # gets the most recent data # Now enters the loop with the stop condition being that the lamp is ON while lamp_state.basicState != ATWhiteLight.LampBasicState.ON: lamp_state = await self.white_light_source.evt_lampState.next( flush=False, timeout=self.timeout_lamp_warm_up ) self.log.info(f"Lamp state: {ATWhiteLight.LampBasicState(lamp_state.basicState)!r}.")

Good suggestion.

tribeiro · 2023-06-29T16:29:25Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+        )
+
+    async def get_monochromator_parameters(self):
+        tmp1 = await self.monochromator.evt_selectedGrating.aget()


Please, add a timeout for all these aget statements, otherwise it can block here for ever.

Also you can probably do this in "in parallel" using asyncio.gather:

return await asyncio.gather( self.monochromator.evt_selectedGrating.aget(timeout=self.std_timeout), self.monochromator.evt_wavelength.aget(timeout=self.std_timeout), self.monochromator.evt_entrySlitWidth.aget(timeout=self.std_timeout), self.monochromator.evt_exitSlitWidth.aget(timeout=self.std_timeout), )

Thanks for the suggestion with the asynctio.gather

tribeiro · 2023-06-29T16:30:10Z

python/lsst/ts/standardscripts/auxtel/calibrations/power_on_atcalsys.py

+            if salobj.State(summary_state.summaryState) != salobj.State(
+                salobj.State.ENABLED
+            ):
+                raise Exception(f"{comp} is not ENABLED")


Please, raise RuntimeError instead of a bare exception.

isotuela marked this pull request as draft May 19, 2023 19:40

isotuela requested a review from tribeiro May 19, 2023 19:40

isotuela force-pushed the tickets/DM-39206 branch 3 times, most recently from 6f1c821 to bb330bd Compare May 19, 2023 21:42

isotuela force-pushed the tickets/DM-39206 branch from 1e81124 to 56946d6 Compare June 1, 2023 14:49

isotuela changed the title ~~DM-39206 - First draft~~ DM-39206 - Power on ATCalSys Jun 5, 2023

tribeiro reviewed Jun 5, 2023

View reviewed changes

isotuela force-pushed the tickets/DM-39206 branch from 5575a22 to 6127f47 Compare June 7, 2023 18:26

tribeiro reviewed Jun 8, 2023

View reviewed changes

isotuela force-pushed the tickets/DM-39206 branch 2 times, most recently from c7cff59 to e35a0d3 Compare June 12, 2023 13:20

isotuela requested a review from tribeiro June 12, 2023 13:22

isotuela force-pushed the tickets/DM-39206 branch from e35a0d3 to 6ee7a0c Compare June 12, 2023 13:40

tribeiro reviewed Jun 13, 2023

View reviewed changes

isotuela force-pushed the tickets/DM-39206 branch 3 times, most recently from d1feb7b to 1930f20 Compare June 14, 2023 02:51

isotuela marked this pull request as ready for review June 14, 2023 02:59

isotuela requested a review from tribeiro June 14, 2023 02:59

tribeiro reviewed Jun 29, 2023

View reviewed changes

isotuela force-pushed the tickets/DM-39206 branch from 8fc0495 to 1053af4 Compare July 3, 2023 15:37

isotuela force-pushed the tickets/DM-39206 branch from a3e7e31 to bc03365 Compare July 10, 2023 19:34

isotuela closed this Jul 10, 2023

isotuela force-pushed the tickets/DM-39206 branch from bc03365 to f005fd1 Compare July 10, 2023 20:21

isotuela deleted the tickets/DM-39206 branch July 10, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-39206 - Power on ATCalSys #116

DM-39206 - Power on ATCalSys #116

isotuela commented May 19, 2023

tribeiro left a comment

tribeiro Jun 5, 2023

tribeiro Jun 5, 2023

tribeiro Jun 5, 2023

tribeiro Jun 5, 2023

isotuela Jun 9, 2023

tribeiro Jun 29, 2023

tribeiro left a comment

tribeiro Jun 8, 2023

tribeiro Jun 8, 2023

tribeiro Jun 8, 2023

tribeiro Jun 8, 2023

isotuela Jun 9, 2023 •

edited

Loading

tribeiro Jun 8, 2023

tribeiro Jun 8, 2023

isotuela Jun 9, 2023

tribeiro Jun 8, 2023

isotuela Jun 9, 2023

tribeiro Jun 8, 2023

tribeiro Jun 13, 2023

tribeiro left a comment

tribeiro Jun 13, 2023

tribeiro Jun 13, 2023

tribeiro Jun 29, 2023

tribeiro Jun 29, 2023

isotuela Jul 3, 2023

tribeiro Jun 29, 2023

tribeiro Jun 29, 2023

tribeiro Jun 29, 2023

isotuela Jul 3, 2023

tribeiro Jun 29, 2023

isotuela Jul 3, 2023

tribeiro Jun 29, 2023

		@@ -32,6 +32,8 @@ v1.22.0

		* Add new ``MoveP2P`` maintel script.

		* Add new ``auxtel/calibrations/power_on_atcalsys.py`` script, unit test and executable to set up and turn on the ATCalSys white light.

DM-39206 - Power on ATCalSys #116

DM-39206 - Power on ATCalSys #116

Conversation

isotuela commented May 19, 2023

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isotuela Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isotuela Jun 9, 2023 •

edited

Loading