I added a detailedState event to ts_xml (and associated enums to ts_idl) because it is crucial for users to know what these substates are. (If we decide to go with the vendor's CSC code we can modify it to output the detailed state, but I hope it doesn't come to that.)

Pull requests:

• https://github.com/lsst-ts/ts_rotator/pull/5
• https://github.com/lsst-ts/ts_hexrotcomm/pull/2
• https://github.com/lsst-ts/ts_xml/pull/149
• https://github.com/lsst-ts/ts_idl/pull/7

Before merging this, but after testing at SLAC clean up the XML as follows (this gets rid of a horrible hack at the expense of temporarily commenting out enumerations that we won't be using anyway; we can restore the enumerations once SAL 4 is released):

• in ts_xml comment out the enumerations for controllerState and in the commented-out code assign values starting at 0 and update the documentation to eliminate any reference to incrementing by 1.
• in ts_xml add a commented-out enumeration for controllerState.applicationStatus; this is a mask with hex values. Test it with ts_sal 5 prerelease. Refer to it instead of documenting in the field itself.
• in ts_idl update the Rotator enums accordingly (0-based instead of 1-based, and add a mask enum for controllerState.applicationStatus).
• in ts_rotator stop incrementing controllerState fields by 1.
• In ts_rotator eliminate the local copies of the ts_idl enums and only use the ts_idl enums.

Te-Wei Tsai's review of the ts_xml changes:
1. What is the controller here:
https://github.com/lsst-ts/ts_xml/pull/149/commits/31586bd2290a25ac4c62a4a0d7fdfa05a92e43f1#diff-22b2e780cdda9ef95b69ba5c0a508e33R7
 
The newCSC/middleware or wrapper in cRIO?
 
2. I feel this part should be in the configuration file instead of the SAL:
https://github.com/lsst-ts/ts_xml/pull/149/commits/31586bd2290a25ac4c62a4a0d7fdfa05a92e43f1#diff-22b2e780cdda9ef95b69ba5c0a508e33R26-R39
 
I prefer the configuration file because it is easier to change in the future. And the wrapper in cRIO can read that configuration file as well. I think to let the wrapper to have the idea of DDS/ SAL may not be a good idea.
 
3. Same question as 1, I do not know what is this controller:
https://github.com/lsst-ts/ts_xml/pull/149/commits/31586bd2290a25ac4c62a4a0d7fdfa05a92e43f1#diff-22b2e780cdda9ef95b69ba5c0a508e33R46
 
4. I do not understand why the value should add 1:
https://github.com/lsst-ts/ts_xml/pull/149/commits/31586bd2290a25ac4c62a4a0d7fdfa05a92e43f1#diff-22b2e780cdda9ef95b69ba5c0a508e33R51
 
https://github.com/lsst-ts/ts_xml/pull/149/commits/31586bd2290a25ac4c62a4a0d7fdfa05a92e43f1#diff-22b2e780cdda9ef95b69ba5c0a508e33R65
 
5. Please help to say "Not used"
https://github.com/lsst-ts/ts_xml/pull/149/commits/c3c4e45fee085549ed2795b3f6d07fde084927b3#diff-0a1ef441d0e102ad1c38e3b2f2a81ec9R59
 
6. Please help to clarify this part is inconsistent with LTS-160 (we need to update that document in a latter time):
https://github.com/lsst-ts/ts_xml/pull/149/commits/c3c4e45fee085549ed2795b3f6d07fde084927b3#diff-0a1ef441d0e102ad1c38e3b2f2a81ec9R157
 
7. When I did the test, I did not need to issue the stop command. Will the description here make the user feel the stop command is needed?
https://github.com/lsst-ts/ts_xml/pull/149/commits/c3c4e45fee085549ed2795b3f6d07fde084927b3#diff-0a1ef441d0e102ad1c38e3b2f2a81ec9R197
 
8. I think it is better to point out the duration unit is sec:
https://github.com/lsst-ts/ts_xml/pull/149/commits/c3c4e45fee085549ed2795b3f6d07fde084927b3#diff-0a1ef441d0e102ad1c38e3b2f2a81ec9R227
 
9. The same question as in 2, I prefer the configuration file:
https://github.com/lsst-ts/ts_xml/pull/149/commits/335be3122ab50c0e55519a86e77f24a87b69bcd7#diff-af62cb06e57b4259b951ad1b1bb2c354R26-R39
 
10. Same question as in 1, what is the controller here?
https://github.com/lsst-ts/ts_xml/pull/149/commits/335be3122ab50c0e55519a86e77f24a87b69bcd7#diff-af62cb06e57b4259b951ad1b1bb2c354R46
 

1 3 10: The low level controller is the controller that runs in the cRIO. I find the terms "middleware" and "wrapper" very confusing. Assuming we adopt my new CSC (I am planning for success) then we will only use the vendor's low level (cRIO) code and their engineering user interface.

2, 9: The ApplicationStatus enum matches a set of hard-coded constants (with the same names) in the cRIO code. I provide them to make it easy for a user to understand the applicationStatus values. I do feel that putting them in a configuration file would be helpful. The vendor's CSC ("wrapper") code has no use for these values.

4: Good catch. I originally was adding 1 to some fields so I could provide enumerations in the XML file. But Dave Mills kindly agreed to allow a new style of enumeration with specified values. This will appear in SAL 4.1 (and I have already tested it). So I removed the +1, added the new style enumerations, but commented them out for now. I updated some of the descriptions but clearly missed others.

5. Good idea. I will do that.

6. I would rather not reference obsolete documentation in the XML. We may not remember to update it.

7. The word "stop" was not intended to be a command name. I have tried to clarify by using "halt". I also took the opportunity to mention the enabled substates in hopes this would further clarify what is going on.

8. I prefer to only describe units using the Units tag. I am strongly opposed to duplicating information because it can easily go out of sync.

All updated code look good! Great job!

Te-Wei Tsai's review of ts_hexrotcomm:
 
1. When I read of book of "Code Complete 2", the author suggested the number of internal attributes is better to be <= 9.
https://github.com/lsst-ts/ts_hexrotcomm/pull/2/files#diff-5225dfeecfc8bb326dc6f2f5943aa9a8R67-R96
 
Although this is pretty personal, I felt there were too many attributes already to follow.
 
2. Is there a way to separate try loop? I felt the indentation is not good:
https://github.com/lsst-ts/ts_hexrotcomm/pull/2/files#diff-5225dfeecfc8bb326dc6f2f5943aa9a8R197-R212
 
But this may be fine.
 

1) An excellent point.

There is a lot of duplication between the code for the command socket server and telemetry socket server. I refactored the code as follows:

• Added a new class OneClientServer which encapsulates a TCP/IP socket server that accepts a single client connection.
• Renamed Server to CommandTelemetryServer to reduce potential confusion between this class, asyncio socket servers and OneClientServer.

I also looked at the command and telemetry client code in BaseMockController and decided that client code is simple enough that the current code is probably better than trying to refactor that.

2) This is a reference to the read_telemetry_and_config method of Server.

I agree it is a bit nested. I looked into extracting the main reading code into a separate method. Unfortunately that makes it tricky to handle the case of an unrecognized frame ID, so I decided it was best to leave the code as is.

Te-Wei Tsai's review of ts_rotator:
<ts_rotator>
 
1. Does this mean we want to restrict the speed can not be 0? I think it might be better say "assert speed >= 0" if it does not hurt.
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R46
 
2. For the time.time(), how about to use the time.perf_counter() instead?
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R50
 
This is because I read this article recently:
https://www.webucator.com/blog/2015/08/python-clocks-explained/
 
I assume you want to consider the delta time in the future because I saw the self._start_time and self._end_time. But maybe I am wrong.
 
It looks like I am wrong when I read here:
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R76
 
You estimate the duration time. However, this estimation depends on how accurate you want it. Maybe the consideration of acceleration is needed if you need the higher accuracy.
 
3. I think use the time to estimate the actuator is moving or not might be dangeraous. But it looks like this is just a mock class. Maybe this is fine:
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R96
 
4. When I played with the real hardware, the positionSet command is needed before the move command.
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R153
 
5. I feel it is easier to understand by just saying: self.encoder_resolution = 200000
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R159
 
6. Does it really matter that we do not want to import the logging library in the initail beginning of script?
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R185
 
Although I know we could import the library in any time....
 
7. I feel there are too many indentation of if loops already.
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-e6ee538b3659b987982aa4710ebeaed8R241-R322
 
I should say it might be hard to debug after 1 months even for the original developer. But it may be fine. I know some AI program in the past did the similar thing.
 
8. What is "_n" in "self._tracking_started_n"?
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-03362fafdd1aac9f841e4c5f118184f8R96
 
9 In my memory, after I used the SAL to clearError, I did not use the GUI to put the rotator into the DDS mode. But the rotator was still controllable by SAL. But maybe I am wrong in my memory.
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-03362fafdd1aac9f841e4c5f118184f8R246-R248
 
10. Although I recorded two times of "clearError" command are needed with the Rotator, I also recorded that Scott needed to do something between these two commands. Scott and I saw that the rotator will not clear the error if we just send two times of clearError commands without doing something in the hardware part.
 
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-03362fafdd1aac9f841e4c5f118184f8R256-R257
 
https://confluence.lsstcorp.org/pages/viewpage.action?pageId=118589798
 
11. The acceleration can be 0 althought it might not make sense:
https://github.com/lsst-ts/ts_rotator/pull/5/files#diff-03362fafdd1aac9f841e4c5f118184f8R264
 

1 and 11): It is not safe to use a velocity or acceleration limit of 0 because moves will never complete. I don't know if the cRIO accepts 0 but I'd rather not find out.

2) Good suggestion. I will use time.monotonic instead of time.time.

As to the accuracy of the model: it would be better if I used the actuator from ts_ATMCSSimulator, which handles slewing and tracking, but I filed a separate ticket for that and I'm not sure we need it.

3) Yes, it's just a simulator.

4) Thank you. I will update the comment.

5) I respectfully disagree. I love the new support for underlines in integer contants. I admit it may take time for folks to become used to them.

6) Good catch. That is leftover debugging code. I will remove it.

7) I agree that the MockMTRotatorController run_command method is messy and difficult to understand.

I refactored the code as follows:

• Added a method for each command. This method is responsible for checking initial conditions.
• Added a lookup table that ties commands to those new methods.

I think it looks a lot better now.

8) The "_n" means it is a count. I will rename it to _tracking_started_telemetry_counter.

9) I assume you are correct and remove anything that says clearError goes to OFFLINE/PUBLISH_ONLY.

10) Based on your notes it appears that you used the interlock system to put the rotator into a FAULT state. I think it likely that the extra operation you had to perform was related to the interlock system.

My changes are extensive enough that I have asked Te-Wei to have another look.

The refactored code looks good and is easy to understand! Great job!

Thank you for the helpful review. It made a big difference.