Add unpark MTDome logic for MTCS #161

iglesu · 2024-09-06T22:30:19Z

Add unpark MTDome logic for MTCS and related tests

- Implemented unpark logic in MTCS to move the dome from PARKED state.
- Added mock support for dome events in MTCSAsyncMock.
- Included unit tests for unpark_dome

tribeiro

I some some questions regarding the use of azEnabled event, why not rely on the azMotion like we did for parking?

Also, please, make sure all methods and attributes are snake_case.

tribeiro · 2024-09-23T15:52:14Z

python/lsst/ts/observatory/control/maintel/mtcs.py

+ timeout=self.fast_timeout
+ )
+
+ az_delta = 0.1 # A small move to un-park the Dome


Please make this a class attribute instead.

shall we also add the
unpark_velocity = 0.0 # The Dome defaults to a crawl velocity

as an attribute?

as per @tribeiro comment:
velocity will always be zero, no matter what.. you can even ignore that parameter and don't set it as the default is zero.

tribeiro · 2024-09-23T15:53:58Z

python/lsst/ts/observatory/control/maintel/mtcs.py

@@ -25,6 +25,7 @@
 import copy
 import enum
 import logging
+import time


Please, don't use the time module, you already have timing functionality with the lsst.ts.utils module. You can user utils.current_tai().

I removed the azEnabled check

tribeiro · 2024-09-23T15:54:31Z

python/lsst/ts/observatory/control/maintel/mtcs.py

@@ -632,6 +633,52 @@ def dome_ready(az_motion: salobj.type_hints.BaseMsgType) -> bool:
 f"Dome state: {MTDome.MotionState(az_motion.state).name}, inPosition: {az_motion.inPosition}"
 )

+ async def wait_for_dome_azEnabled(self, timeout: float) -> None:


This method name should be snake_case, please rename it to wait_for_dome_az_enabled.

However, do you really need this method? won't the one you included for parking (wait_for_dome_state) do the work?

azEnabled check removed

tribeiro · 2024-09-23T15:57:06Z

python/lsst/ts/observatory/control/maintel/mtcs.py

+ )
+
+ # The cmd_MoveAz should issue a azEnabled event set to ENABLED
+ await self.wait_for_dome_azEnabled(timeout=self.park_dome_timeout)


So you really need to look for this event? Why not rely on the wait_for_dome_state?

on the azEnabled event, I'm following the advice given by you and Wouter on the issue: https://rubinobs.atlassian.net/browse/DM-45608?focusedCommentId=523258 specifically:

Also check the azMotion, and azEnabled events to verify UNPARKED status.

but happy to do otherwise.

as per @tribeiro comment:
I think the comments was more on the lines of use one or the other..

in light of that I'm removing the azEnabled check

tribeiro · 2024-09-23T15:57:46Z

python/lsst/ts/observatory/control/mock/mtcs_async_mock.py

@@ -121,10 +121,14 @@ async def setup_types(self) -> None:
 )

 # MTDome Motion PARKED state. State is set by the mocked park cmd.
- self._mtdome_evt_azMotion_state = types.SimpleNamespace(
+ self._mtdome_evt_azMotion = types.SimpleNamespace(


Please, use snake_case for attributes.

tribeiro · 2024-09-23T15:57:57Z

python/lsst/ts/observatory/control/mock/mtcs_async_mock.py

 state=MTDome.MotionState.UNDETERMINED, inPosition=False
 )

+ self._mtdome_evt_azEnabled = types.SimpleNamespace(


snake_case

I'm removing the azEnabled check

tribeiro · 2024-09-23T23:08:22Z

python/lsst/ts/observatory/control/maintel/mtcs.py

@@ -148,6 +148,9 @@ def __init__(
 self._dome_az_in_position: typing.Union[None, asyncio.Event] = None
 self._dome_el_in_positio: typing.Union[None, asyncio.Event] = None

+ self.az_delta = 0.1 # A small move to un-park the Dome


so, instead of relying on the comment (that is only really here) to specify what the attribute is, use a more expressive name. Something like;

# Small offset (in degrees) applied to the dome to unpark it. self.dome_az_unpark_offset = 0.1

Would be better.

tribeiro · 2024-09-23T23:08:39Z

python/lsst/ts/observatory/control/maintel/mtcs.py

@@ -148,6 +148,9 @@ def __init__(
 self._dome_az_in_position: typing.Union[None, asyncio.Event] = None
 self._dome_el_in_positio: typing.Union[None, asyncio.Event] = None

+ self.az_delta = 0.1 # A small move to un-park the Dome
+ self.dome_unpark_velocity = 0.0 # A small move to un-park the Dome


This is not necessary so please, consider removing it.

tribeiro · 2024-09-23T23:09:26Z

python/lsst/ts/observatory/control/maintel/mtcs.py

+ message="All components need to be enabled for un-parking the Dome."
+ )
+
+ self.log.info("Checking if the dome is currently PARKED.")


Maybe debug level instead?

tribeiro · 2024-09-23T23:12:10Z

python/lsst/ts/observatory/control/maintel/mtcs.py

+ self.rem.mtdome.evt_azMotion.flush()
+
+ # We don't specify the dome velocity as it defaults to a
+ # `crawl` which is what we want.


This is not correctly stated. This should read:

# We don't specify the dome velocity as it defaults to # zero, which is what we want.

tribeiro

looks good!

- Implemented unpark logic in MTCS to move the dome from PARKED state. - Added mock support for dome events in MTCSAsyncMock. - Included unit tests for unpark_dome

iglesu requested a review from tribeiro September 6, 2024 22:30

iglesu force-pushed the tickets/DM-45610 branch from f4d15cc to 82633bf Compare September 6, 2024 22:32

iglesu marked this pull request as draft September 10, 2024 17:32

iglesu force-pushed the tickets/DM-45610 branch from 82633bf to 0e97a9e Compare September 16, 2024 17:00

iglesu marked this pull request as ready for review September 18, 2024 21:45

iglesu force-pushed the tickets/DM-45610 branch 3 times, most recently from 0bed32e to 276a77f Compare September 20, 2024 20:23

tribeiro requested changes Sep 23, 2024

View reviewed changes

iglesu force-pushed the tickets/DM-45610 branch from 9c00a65 to 31ad5f6 Compare September 23, 2024 23:02

tribeiro reviewed Sep 23, 2024

View reviewed changes

tribeiro approved these changes Sep 24, 2024

View reviewed changes

iglesu force-pushed the tickets/DM-45610 branch from f946283 to a207e7c Compare September 24, 2024 15:33

iglesu added 2 commits September 26, 2024 09:44

Add unpark logic for MTDome and related tests

4de5d94

- Implemented unpark logic in MTCS to move the dome from PARKED state. - Added mock support for dome events in MTCSAsyncMock. - Included unit tests for unpark_dome

Add news fragment

2baec9b

iglesu force-pushed the tickets/DM-45610 branch from d4f501c to 2baec9b Compare September 26, 2024 16:45

iglesu merged commit fa702c9 into develop Sep 26, 2024
5 checks passed

iglesu deleted the tickets/DM-45610 branch September 26, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unpark MTDome logic for MTCS #161

Add unpark MTDome logic for MTCS #161

iglesu commented Sep 6, 2024

tribeiro left a comment

tribeiro Sep 23, 2024

iglesu Sep 23, 2024

iglesu Sep 23, 2024

tribeiro Sep 23, 2024

iglesu Sep 23, 2024

tribeiro Sep 23, 2024

iglesu Sep 23, 2024

tribeiro Sep 23, 2024

iglesu Sep 23, 2024

iglesu Sep 23, 2024 •

edited

Loading

tribeiro Sep 23, 2024

tribeiro Sep 23, 2024

iglesu Sep 23, 2024

tribeiro Sep 23, 2024

tribeiro Sep 23, 2024

tribeiro Sep 23, 2024

tribeiro Sep 23, 2024

tribeiro left a comment

Add unpark MTDome logic for MTCS #161

Add unpark MTDome logic for MTCS #161

Conversation

iglesu commented Sep 6, 2024

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

iglesu Sep 23, 2024 • 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

iglesu Sep 23, 2024 •

edited

Loading