See https://confluence.lsstcorp.org/display/LTS/MT+Rotator+and+Hexapod+Notes

I created a new package ts_pymoog to contain base classes for TCP/IP communication – both a server that runs in the CSC and a client for simulating the low level controller code in the cRIOs.

Pull request: https://github.com/lsst-ts/ts_pymoog/pull/1

To see how this is used see DM-21694, especially the pull request https://github.com/lsst-ts/ts_rotator/pull/5

The code in PR looks good and fulfill the ticket description. Great job done!

Many thanks to Te-Wei Tsai for the following detailed review (with permission):

1. I am confused for the package name: ts_pymoog. Moog code contains (1) middleware, (2) wrapper, and (3) LabVIEW GUI. It looks like you try to do something like ts_ctrl_crio_moog.

2. This sentence is misleading:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-752cfbb96342b26a63548167f8d66defR12
In Moog's code, it is three (not "each") cRIO controllers communicates over TCP/IP with a Commandable SAL Component (CSC = middleware).

3. I am not sure we should use the word of "simulate" or "emulate" here:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-752cfbb96342b26a63548167f8d66defR15
The emulator has the meaning to mimic the hardware behavior that this PR does not look like trying to do that. For example, Android emulator gives certain behavior of hardware phone already:
https://developer.android.com/studio/run/emulator
But when I reviewed your code, if you only want to consider the behavior of TCP/IP client, it did look like a TCP/IP client "emulator".
I do see some simulation of the behavior here:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-cf112886b18d8fd2b0df1c3d7c312112R88-R96
If this is the case, maybe the term of emulate is good here.

4. For the middleware by Moog, you never get the feedback from cRIO for "rejected command":
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-752cfbb96342b26a63548167f8d66defR29

5. Moog provides the CSC, not "CSCs". There is only one middleware:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-752cfbb96342b26a63548167f8d66defR37

6. Moog only provides a single "CSC" (commandable SAL commonent), which is "middleware" here. The "wrapper" in Moog's documentation means the code deployed in the cRIO.
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-752cfbb96342b26a63548167f8d66defR38
PS. I have no real opinion to use the terms of middleware, wrapper, or CSC here. I just feel if we do not want to be consistent with the terminology, the future's developers/ maintainers will be super-confused. I stick to the documentation of Moog at this moment. If you prefer to use your own terminologies, we need to discuss how to update the documentation from Moog. Otherwise, it will be a big trouble in the future.

7. Do you add this salobj to the table file as the required package?
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR29
I do not see that actually:
https://github.com/lsst-ts/ts_pymoog/blob/tickets/DM-21467/ups/ts_pymoog.table

8. I do not understand the meaning of header here:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR75

9. I thought the self.cmd_reader need to be closed as the self.cmd_writer. But it looks like no thread for the self.cmd_reader. Is this correct?
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR127
The same question for the self.tel_reader:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR129
PS. I thought I understand you meaning here:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR164-R165
Do you want to consider to add the comments on: self.cmd_writer.close() to say the self.cmd_reader will be closed as well?

10. What is "coros"?
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR143

11. Do you want to explain the header details in the docstring:
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-30051fa9dcb1d0e34839888ba150974fR215
Android DevelopersAndroid Developers
Run apps on the Android Emulator | Android Developers
The Android Emulator simulates Android devices on your computer so that you can test your application on a variety of devices and Android API levels without needing to have each physical device.(20 kB)

12. Where is the input: "reader" to be used?
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-550eff8130d42c87e273101c19320703R73

13. I know the message is a c structure (struct):
https://github.com/lsst-ts/ts_pymoog/pull/1/files#diff-a9c28ca716bb24706e5dac2c9569dcbaR27
But I thought it could be named like the cmd_msg to abstract the input to give more readability. But this is personal preference.

14. It looks like you try to use async (single thread) to simulate the behavior of two threads. Will we have the performance issue?

1. I agree with you; I do not like the name ts_pymoog. However I would like to avoid underscores in package names implies sub-package namespaces (at least the way we do things), for example ts_ctrl_crio_moog implies lsst.ts.ctrl.crio.moog.

After discussion with several people I have renamed the package to ts_hexrotcomm.

2, 5 and 6: In my opinion the Moog "wrapper" code implements three different CSCs. Each has its own states and SAL topics. It is an internal detail that these CSCs are all running in the same process. The only clue for the user is that if one brings down the "wrapper" all three CSCs go away.

I have tried to update my documentation to make that clearer.

3. I have decided to go with "mock" everywhere. I think "emulate" is also a reasonable term, but CSCs have a simulation mode, so I prefer to restrict the number of terms to "simulate" and "mock". I could get rid of mock as well, but it is common and I prefer BaseMockController to BaseControllerSimulator or BaseSimulatedController.

4. I did not understand your point so I asked offline. Here is my understanding now:

The current CSCs (middleware) always say that acknowledge a command as succeeded even if it is rejected. They output the rejectedCommand event to reject a command.

These details are out of scope for this package, but I will say that I find this behavior undesirable. My plan for my rotator and hexapod CSCs to properly reject commands when they can do so.

7. Good catch. I changed to the code to not use salobj, as it seems silly to require the package just for this.

8. Excellent point. I added a code comment that I hope will explain what is going on.

9. I initially thought that readers should be closed, but it turns out that asyncio socket readers cannot be closed; they have no close method.

10. "coros" means "coroutines". I will rename the variable to coroutines. (Note that "coro" is a common abbreviation in Python asyncio examples, but even so, I hope this makes the code easier to read).

11. I hope it be clearer now that I have clarified item 8.

12. Excellent point and I see that the doc string for set_command_writer is wrong, as well. I updated the doc strings to document the parameters and explain why the arguments are ignored. For the record these are called by asyncio.start_server which provides both a reader and a writer.

13. Good point. I tried changing struct to cstruct everywhere but I felt it hampered readability. I am not keen to add the underscore, so I plan to stick with struct despite some reservations.

14. I think asyncio can easily support these data rates. A timing test in salobj shows that one process can both produce and consume DDS messages at a rate of over 5000 messages/second. I would expect TCP/IP to do even better, though perhaps the use of ctypes to encode and decode the data might be a factor (I have not timed that).

Although it is possible to run higher data rates for debugging, the standard rate is 20 Hz and I feel if the CSCs can reliably keep up with that then we should be in fine shape.

Could you please have another look? The pull request is now: https://github.com/lsst-ts/ts_hexrotcomm/pull/1

This looks good to me now! Great job!

The PR is updated based on the discussion between Russell and my. The code and documentation looks good now!

Merged to develop.