Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-22066

Fix issues discovered in SLAC rotator testing

    XMLWordPrintable

    Details

    • Story Points:
      1
    • Sprint:
      TSSW Sprint - Oct 28 - Nov 10, TSSW Sprint - Nov 11 - Nov 22
    • Team:
      Telescope and Site

      Description

      In SLAC rotator testing today we discovered several issues that need fixing:

      • The counter field was the wrong type for Command and Header (should be uint, not ushort).
      • The rotator and hexapod CSCs have no heartbeat output. This strongly suggests we should have a base class for both CSCs.
      • The rotator commander outputs too much noise because of encoder jitter (not a surprise). This is likely an issue for the hexapod commander as well.

      Note that we still were not able to command the rotator: the commands were ignored. Fixing that is a different ticket.

      I have attached a file that shows some binary telemetry data we read from the rotator, and how the Python code parses it.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            I am also implementing DM-22104 on this ticket. Most of the fixes are in ts_hexrotcomm:

            • Command and Header structs now have the correct type for the counter field.
            • hexrotcomm.BaseCsc outputs a heartbeat event.
            • hexrotcomm.BaseCsc outputs better clearer messages when a state transition command fails.

            However I also added a bit of jitter to the current position from the MockMTRotatorController in ts_rotator
            and updated command_rotator.py to ignore small jitter in telemetry topics.

            Show
            rowen Russell Owen added a comment - I am also implementing DM-22104 on this ticket. Most of the fixes are in ts_hexrotcomm: Command and Header structs now have the correct type for the counter field. hexrotcomm.BaseCsc outputs a heartbeat event. hexrotcomm.BaseCsc outputs better clearer messages when a state transition command fails. However I also added a bit of jitter to the current position from the MockMTRotatorController in ts_rotator and updated command_rotator.py to ignore small jitter in telemetry topics.
            Hide
            rowen Russell Owen added a comment -

            If you have time to review this I would appreciate it, but if not just let me know and I'll ask somebody else. You have already looked at earlier versions of this code and may be sick of it.

            Changes I made to reduce duplication of code between ts_hexapod and ts_rotator:

            • Made BaseMockController handle all standard commands. For clarity I put the TCP/IP code into a new base class CommandTelemetryClient. I initially tried to make that an attribute of BaseMockController instead of a subclass, but the TCP/IP clients and controller are so entwined (the client needs to know a lot about the controller) that I decided inheritance was cleaner.
            • Added BaseCsc as an abstract base class for Moog CSCs.
            • Added SimpleCsc as a means of testing BaseCsc.
            • Added BaseCscTestCase as a base class for CSC unit tests. Note: I hope to find time to add a variant of this to salobj (on a different ticket).
            • Added CscCommander as a base class for command-line commander code. I am happy with the result, though some duplication remains between the commands and the help string, and in defining each command (command methods are short, but the method name and the name of the command that it calls must match or the behavior will be incorrect).

            Known issues:

            • The mock controllers have few independent unit tests (some in ts_hexrotcomm's test_command_telemetry_server.py). I did this because mock controllers are only intended to serve the CSC, and the CSC unit tests exercise all the controller code that we care about. Note that unit tests for the mock controllers would tend to look very much like the unit tests for the CSCs, though with enough differences that I doubt they could share code.
            • CscCommander has no unit tests. I am not happy about this, but feel it is arguably the best use of time, as these commanders are intended to be temporary. Nonetheless it would only cost a day or so to add such tests.
            Show
            rowen Russell Owen added a comment - If you have time to review this I would appreciate it, but if not just let me know and I'll ask somebody else. You have already looked at earlier versions of this code and may be sick of it. Changes I made to reduce duplication of code between ts_hexapod and ts_rotator: Made BaseMockController handle all standard commands. For clarity I put the TCP/IP code into a new base class CommandTelemetryClient. I initially tried to make that an attribute of BaseMockController instead of a subclass, but the TCP/IP clients and controller are so entwined (the client needs to know a lot about the controller) that I decided inheritance was cleaner. Added BaseCsc as an abstract base class for Moog CSCs. Added SimpleCsc as a means of testing BaseCsc. Added BaseCscTestCase as a base class for CSC unit tests. Note: I hope to find time to add a variant of this to salobj (on a different ticket). Added CscCommander as a base class for command-line commander code. I am happy with the result, though some duplication remains between the commands and the help string, and in defining each command (command methods are short, but the method name and the name of the command that it calls must match or the behavior will be incorrect). Known issues: The mock controllers have few independent unit tests (some in ts_hexrotcomm's test_command_telemetry_server.py). I did this because mock controllers are only intended to serve the CSC, and the CSC unit tests exercise all the controller code that we care about. Note that unit tests for the mock controllers would tend to look very much like the unit tests for the CSCs, though with enough differences that I doubt they could share code. CscCommander has no unit tests. I am not happy about this, but feel it is arguably the best use of time, as these commanders are intended to be temporary. Nonetheless it would only cost a day or so to add such tests.
            Hide
            ttsai Te-Wei Tsai added a comment -

            This task recorded the test in SLAC and the decoding of telemetry and header messages. The implementation and refactoring of hexapod and rotator code are in DM-22104. The comment describes the change of code that will happen in DM-22104. You could put me as the reviewer of DM-22104 if you want. I still need to be familiar with your code for the test in Dec actually.

            Well done!

            Show
            ttsai Te-Wei Tsai added a comment - This task recorded the test in SLAC and the decoding of telemetry and header messages. The implementation and refactoring of hexapod and rotator code are in DM-22104 . The comment describes the change of code that will happen in DM-22104 . You could put me as the reviewer of DM-22104 if you want. I still need to be familiar with your code for the test in Dec actually. Well done!
            Show
            rowen Russell Owen added a comment - Pull requests: https://github.com/lsst-ts/ts_hexrotcomm/pull/3 https://github.com/lsst-ts/ts_hexapod/pull/5 https://github.com/lsst-ts/ts_rotator/pull/7
            Hide
            ttsai Te-Wei Tsai added a comment -

            All reviews of PRs are done. The refactor of codes looks good to me! Great job!

            Show
            ttsai Te-Wei Tsai added a comment - All reviews of PRs are done. The refactor of codes looks good to me! Great job!
            Hide
            rowen Russell Owen added a comment -

            Here is Te-Wei's detailed review of ts_hexrotcomm:

            [base_csc.py]

            1. What will be the benenfit to use the metaclass in BaseCsc?

            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R51

            I checked the discussion here to try to understand the use of metaclass:
            https://stackoverflow.com/questions/17801344/understanding-metaclass-and-inheritance-in-python

            I asked this because I thought you can use the inheritance directly. You should have some reason to use the metaclass.

            2. I thought the "start_task" should be issued before any real work such as "await super().start()" when you helped me to
            integrate the ts_MTAOS with ts_salobj v5.0.RC2:

            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R148

            3. The data type is a little inconsistent:
            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R322
            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R334
            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R345

            [base_mock_controller.py]

            1. I thought your naming convension is to begin from the lower case and use the "_" instead of camel form:
            https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-08b181c9bba5841f7715e574c868af46R85

            Show
            rowen Russell Owen added a comment - Here is Te-Wei's detailed review of ts_hexrotcomm: [base_csc.py] 1. What will be the benenfit to use the metaclass in BaseCsc? https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R51 I checked the discussion here to try to understand the use of metaclass: https://stackoverflow.com/questions/17801344/understanding-metaclass-and-inheritance-in-python I asked this because I thought you can use the inheritance directly. You should have some reason to use the metaclass. 2. I thought the "start_task" should be issued before any real work such as "await super().start()" when you helped me to integrate the ts_MTAOS with ts_salobj v5.0.RC2: https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R148 3. The data type is a little inconsistent: https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R322 https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R334 https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-9884fc357e4ad38105bdf51d866033e9R345 [base_mock_controller.py] 1. I thought your naming convension is to begin from the lower case and use the "_" instead of camel form: https://github.com/lsst-ts/ts_hexrotcomm/pull/3/files#diff-08b181c9bba5841f7715e574c868af46R85
            Hide
            rowen Russell Owen added a comment -

            My response to Te-Wei's review of ts_hexrotomm:

            [base_csc.py]

            1. This use of metaclass=abc.ABCMeta is standard for the abc module.

            It is true that the abc module also includes a parent class one can inherit from, instead. But I prefer the direct technique because:

            • It makes it clear that a metaclass is being used, which means the user must be careful about multiple inheritance.
            • It avoids one level if inheritance.

            2. My pattern is as follows:

            • Add a start_task attribute to any class that has an asynchronous start method.
            • Have the object automatically start itself when constructed by having the constructor call self.start_task = asyncio.create_task(self.start()))
            • This allows a user who creates an instance of the class call await myinstance.start_task to wait until the instance has fully started.

            That is what is happening here. BaseCsc has an instance variable server that is a CommandTelemetryServer, which handles the communication between the cRIO and the CSC. BaseCsc.start is waiting for the server to finish starting.

            3. Good point. I'll shorten the long ones (I found 3 instances).

            For the record: within a package there is no need to provide the full namespace to get a working link, so just providing the class name is fine.

            [base_mock_controller.py]

            1. This is a in reference to an argument named CommandCode.

            This follows the DM convention, which is to use CamelCase for variables that hold classes. One justification is that variable naming convention matches the type the variable holds, which makes the code a bit easier to read when one creates an instance of the class. And if nothing else, it may help avoid the mistake of providing an instance when a class is required. Here is a silly toy example:

                def make_instance(argument, ClassArgument):
                    return ClassArgument(argument)
            

            Show
            rowen Russell Owen added a comment - My response to Te-Wei's review of ts_hexrotomm: [base_csc.py] 1. This use of metaclass=abc.ABCMeta is standard for the abc module. It is true that the abc module also includes a parent class one can inherit from, instead. But I prefer the direct technique because: It makes it clear that a metaclass is being used, which means the user must be careful about multiple inheritance. It avoids one level if inheritance. 2. My pattern is as follows: Add a start_task attribute to any class that has an asynchronous start method. Have the object automatically start itself when constructed by having the constructor call self.start_task = asyncio.create_task(self.start())) This allows a user who creates an instance of the class call await myinstance.start_task to wait until the instance has fully started. That is what is happening here. BaseCsc has an instance variable server that is a CommandTelemetryServer , which handles the communication between the cRIO and the CSC. BaseCsc.start is waiting for the server to finish starting. 3. Good point. I'll shorten the long ones (I found 3 instances). For the record: within a package there is no need to provide the full namespace to get a working link, so just providing the class name is fine. [base_mock_controller.py] 1. This is a in reference to an argument named CommandCode . This follows the DM convention, which is to use CamelCase for variables that hold classes. One justification is that variable naming convention matches the type the variable holds, which makes the code a bit easier to read when one creates an instance of the class. And if nothing else, it may help avoid the mistake of providing an instance when a class is required. Here is a silly toy example: def make_instance(argument, ClassArgument): return ClassArgument(argument)
            Hide
            rowen Russell Owen added a comment -

            All three packages merged to develop and master and released as v0.1.0.

            Show
            rowen Russell Owen added a comment - All three packages merged to develop and master and released as v0.1.0.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Te-Wei Tsai
              Watchers:
              Russell Owen, Te-Wei Tsai, Tiago Ribeiro
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.