Pull request: https://github.com/lsst-ts/ts_hexapod/pull/3

Note that the changes to ts_xml and ts_idl are on tickets/DM-21694 and ts_xml has been merged to develop.

I had tickets/DM-21778 branches for those packages but have since deleted them.

Code is implemented based on the ticket description. Great job!

Te-Wei Tsai's review:

1. The desciption here (line 46-48) could be explained more for the reason of the exchange of key and value. It is a little misleading when I read it:
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-267186c7c9d800ff16df86883e84a7e5R46-R48

2. It might be good to explain the index 1 or 2 means the camera or M2 hexapod here (line 77):
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-267186c7c9d800ff16df86883e84a7e5R77

3. In the GUI, if there is no error and the user issues the "clearError" command, the system is still contronllable and not in the Fault state. I am pretty sure for the middleware by vendor has the same idea. However, the code here will put the CSC into the Fault state (line 289-290), is it a good idea?
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-267186c7c9d800ff16df86883e84a7e5R289-R290
Or do I have misunderstood the code? The salobj just record the "ExpectedError" and will not put the system into the Fault state.

4. Same question as 3, will the system be put into the Fault state (line 482-483)?
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-267186c7c9d800ff16df86883e84a7e5R482-R483

5. I saw the index's explanation here (line 636). This replies the question 2. But I think the future's developer/ maintainer does not want to read this statement to understand it.
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-267186c7c9d800ff16df86883e84a7e5R636

[mock_controller.py]

1. Same question 2 in hexapod_csc.py (line 44):
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-3eff6648ada2ac75c09c6e8ba651ea62R44

2. The MockMTHexapodController has the base_angle0 attribute. And this pivot is the input argument of hexapod instantiation argument (line 114). If someone update the base_angle0 value of the MockMTHexapodController instance, will the internal state (attributes) of hexapod change as well? The same questions for the arguments needed in the instantiation of simple_hexapod.SimpleHexapod.make_zigzag_model as well.
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-3eff6648ada2ac75c09c6e8ba651ea62R114

[utils.py]

1. If you import the numpy already, why do you still need the math (line 24)?
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-1fe6a08da4d9305d9b23debf785a8728R24

2. It might be good to clarify the rotation is clockwise or counter-clockwise about the y-axis (line 150). My intuition is the latter one actually.
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-1fe6a08da4d9305d9b23debf785a8728R151

3. Same question above (line 171):
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-1fe6a08da4d9305d9b23debf785a8728R171

4. Same as above (line 147):
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-1fe6a08da4d9305d9b23debf785a8728R147

[test_simple_hexapod.py]

1. If the asynicio test framework is used, what is the benefits to use the numpy's as well (line 231, 232)?
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-eb44c135b450727ebda0d6f1887f2663R231-R232

2. I am pretty sure the numpy has the similar funtion of atan2 (line 308):
https://github.com/lsst-ts/ts_hexapod/pull/3/files#diff-eb44c135b450727ebda0d6f1887f2663R308

I am not sure I understand the benefit to use the numpy and math libraries in the same time. Performance concern? Or the developer's personal trend?

Thank you for the helpful review. Here are my comments.

[rotator_csc.py]

1) I have attempted to clarify.

2) Great suggestion. I have changed this to:

    index : `SalIndex` or `int`

        SAL index; see `SalIndex` for the allowed values.

3) and 4) Rejecting a command does not change the state of the CSC.

For example the CSC rejects the clearError command if the current state is not FAULT,
but if the CSC was Enabled it remains Enabled.

5) Agreed.

[mock_controller.py]

1) You are absolutely right. I made the same change here.

2) I am not sure I understand your question, but I hope this will help:

base_angle0 is an argument for SimpleHexapod.make_zigzag_model, which is a way to construct a SimpleHexapod. It specifies the "clocking" of the zigzag arrangement of actuators. It is not saved anywhere so there is no opportunity for a user to even try to change to change it later.

Once the mock controller builds a SimpleHexapod it cannot be changed.

By the way I reluctant to call it a "pivot", since the pivot point refers to something else (that the user can change).

[utils.py]

1) I prefer to use math for scalar math and numpy for arrays and other sequences. I think it makes the intent of the code clearer.

2) OK. I will clarify for all three functions. For example:

def rot_about_y(xyzpos, ang):

    """Rotate a 3-d position about the y axis.

    Positive rotation is from from z to x (the usual right-hand rule).

[test_simple_hexapod.py]

1) numpy offers nice functions for testing if one sequence is similar to another and unittest does not.

I am not sure what the asyncio test framework has to do with this, so I may have misunderstood your question. If so, please ask again.

2) (Another case of using math for scalars.)

Merged to develop