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.

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.

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!

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

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

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)

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