Initial addition of reflector #868

JosephParsons · 2024-06-07T19:12:44Z

No description provided.

couger01

This is missing some major pieces for ts_xml.

Need to add a Reflector in sal_interfaces/SALSubsystems.xml
Need to add the Reflector section towncrier.toml
Need to add Reflector into testutils.py

couger01 · 2024-06-11T18:32:49Z

doc/news/dm-43456.feature.rst

This should be under interface_changes and renamed to DM-43456.reflector.rst.

couger01 · 2024-06-11T18:41:55Z

python/lsst/ts/xml/data/sal_interfaces/Reflector/Reflector_Events.xml

+  <SALEvent>
+    <Subsystem>Reflector</Subsystem>
+    <EFDB_Topic>Reflector_logevent_reflectorState</EFDB_Topic>
+    <Description>Reflector state change</Description>
+    <item>
+      <EFDB_Name>reflectorState</EFDB_Name>
+      <Description>Reflector state; a ReflectorState enum.</Description>
+      <IDL_Type>int</IDL_Type>
+      <Units>unitless</Units>
+      <Count>1</Count>
+    </item>
+  </SALEvent>


I think this event should be replaced with two events that indicate that the reflector is on or off. The error state that is here should be superceded by the fault state since there isn't a second discrete device that also indicates state.

couger01 · 2024-06-11T18:42:37Z

python/lsst/ts/xml/data/sal_interfaces/Reflector/Reflector_Commands.xml

+  <SALCommand>
+    <Subsystem>Reflector</Subsystem>
+    <EFDB_Topic>Reflector_command_switchOn</EFDB_Topic>
+    <Description>Switch on reflector.</Description>
+  </SALCommand>
+  <SALCommand>
+    <Subsystem>Reflector</Subsystem>
+    <EFDB_Topic>Reflector_command_switchOff</EFDB_Topic>
+    <Description>Switch off reflector.</Description>
+  </SALCommand>
+</SALCommandSet>


Suggested change

<SALCommand>

<Subsystem>Reflector</Subsystem>

<EFDB_Topic>Reflector_command_switchOn</EFDB_Topic>

<Description>Switch on reflector.</Description>

</SALCommand>

<SALCommand>

<Subsystem>Reflector</Subsystem>

<EFDB_Topic>Reflector_command_switchOff</EFDB_Topic>

<Description>Switch off reflector.</Description>

</SALCommand>

</SALCommandSet>

<SALCommand>

<Subsystem>Reflector</Subsystem>

<EFDB_Topic>Reflector_command_switch</EFDB_Topic>

<Description>Switch on/off reflector.</Description>

<item>

<EFDB_Name>toggle</EFDB_Name>

<Description>Switch on/off the reflector</Description>

<IDL_Type>bool</IDL_Type>

<Count>1</Count>

</item>

</SALCommand>

</SALCommandSet>

I think this could just be one command with a boolean toggle value for switching on the reflector and and switching off the reflector.

tribeiro

Please create one command to open and one command to close the reflector.

tribeiro · 2024-07-01T20:09:44Z

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Commands.xml

+<SALCommandSet xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://lsst-sal.tuc.noao.edu/schema/SALCommandSet.xsd">
+  <SALCommand>
+    <Subsystem>MTReflector</Subsystem>
+    <EFDB_Topic>MTReflector_command_toggle</EFDB_Topic>


Please, create an open and a close command instead of a toggle command.

Please remove the toggle command.

couger01

There should be two more states added if possible for opening and closing.

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Events.xml

couger01

Remove the toggle command, it's not necessary with the open and close commands.
Add two states for opening and closing if possible.

couger01 · 2024-07-08T15:07:57Z

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Commands.xml

+<SALCommandSet xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://lsst-sal.tuc.noao.edu/schema/SALCommandSet.xsd">
+  <SALCommand>
+    <Subsystem>MTReflector</Subsystem>
+    <EFDB_Topic>MTReflector_command_toggle</EFDB_Topic>


Please remove the toggle command.

couger01 · 2024-07-08T15:09:21Z

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Events.xml

+    MTReflectorStatus_Connected=0,
+    MTReflectorStatus_Disconnected=1,
+    MTReflectorStatus_Unknown=2,
+    MTReflectorStatus_Connection_Error=3


Please add states for opening and closing.

couger01 · 2024-07-08T15:09:46Z

python/lsst/ts/xml/enums/MTReflector.py

+    CONNECTED = 0
+    DISCONNECTED = 1
+    UNKNOWN = 2
+    CONNECTION_ERROR = 3


Please add states for opening and closing.

tribeiro

I have a couple more comments to this PR.

tribeiro · 2024-07-08T18:00:27Z

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Events.xml

+  </SALEvent>
+  <SALEvent>
+    <Subsystem>MTReflector</Subsystem>
+    <EFDB_Topic>MTReflector_logevent_reflectorOpen</EFDB_Topic>


I really don't think having different events for open and close is a good idea. You already have an event that indicates the status, so that is probably good enough. Please, remove these 2 events.

tribeiro · 2024-07-08T18:01:10Z

python/lsst/ts/xml/enums/MTReflector.py

+
+    Attributes
+    ----------
+


I think you can remove this empty line here.

tribeiro

Looks good!

couger01

LGTM.

JosephParsons self-assigned this Jun 7, 2024

JosephParsons requested a review from couger01 June 7, 2024 20:25

JosephParsons force-pushed the tickets/DM-43456 branch from 91d293b to 0e3689b Compare June 11, 2024 15:39

couger01 requested changes Jun 11, 2024

View reviewed changes

JosephParsons force-pushed the tickets/DM-43456 branch from 268b725 to 7da7064 Compare June 28, 2024 12:55

tribeiro requested changes Jul 1, 2024

View reviewed changes

couger01 requested changes Jul 1, 2024

View reviewed changes

python/lsst/ts/xml/data/sal_interfaces/MTReflector/MTReflector_Events.xml Show resolved Hide resolved

JosephParsons force-pushed the tickets/DM-43456 branch from e5d781f to 8fe66fa Compare July 8, 2024 14:53

couger01 requested changes Jul 8, 2024

View reviewed changes

JosephParsons force-pushed the tickets/DM-43456 branch from 8fe66fa to 2bd0d4a Compare July 8, 2024 15:14

tribeiro reviewed Jul 8, 2024

View reviewed changes

Initial addition of MTreflector

bd1177f

JosephParsons force-pushed the tickets/DM-43456 branch from 2bd0d4a to bd1177f Compare July 8, 2024 21:05

tribeiro approved these changes Jul 8, 2024

View reviewed changes

couger01 approved these changes Jul 9, 2024

View reviewed changes

JosephParsons merged commit e2a97b8 into develop Jul 9, 2024
5 of 6 checks passed

JosephParsons deleted the tickets/DM-43456 branch July 9, 2024 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial addition of reflector #868

Initial addition of reflector #868

JosephParsons commented Jun 7, 2024

couger01 left a comment

couger01 Jun 11, 2024

couger01 Jun 11, 2024

couger01 Jun 11, 2024

tribeiro left a comment

tribeiro Jul 1, 2024

couger01 Jul 8, 2024

couger01 left a comment •

edited

Loading

couger01 left a comment

couger01 Jul 8, 2024

couger01 Jul 8, 2024

couger01 Jul 8, 2024

tribeiro left a comment

tribeiro Jul 8, 2024

tribeiro Jul 8, 2024

tribeiro left a comment

couger01 left a comment

Initial addition of reflector #868

Initial addition of reflector #868

Conversation

JosephParsons commented Jun 7, 2024

couger01 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

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

couger01 left a comment • edited Loading

Choose a reason for hiding this comment

couger01 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

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

tribeiro left a comment

Choose a reason for hiding this comment

couger01 left a comment

Choose a reason for hiding this comment

couger01 left a comment •

edited

Loading