Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: None
-
Labels:None
-
Story Points:3
-
Epic Link:
-
Sprint:TSSW Sprint - Dec 9 - Dec 20, TSSW Sprint - Dec 23 - Jan 3, TSSW Sprint - Jan 06 - Jan 17
-
Team:Telescope and Site
Description
The XML for Rotator and Hexapod has some misfeatures I would like to fix if and when we are sure that we are writing our own CSCs. (Actually we could easily fix these even if we keep the vendor's code):
- The Hexapod inPosition event has no inPosition field, so there is no way to indicate "not in position". Note that the Rotator inPosition event is fine. I suggest also adding an array of 6 booleans to indicate if each actuator is in position, perhaps named actuatorInPosition.
- The Rotator tracking and trackLost events have no boolean value so cannot be output properly. They should be combined into a single event with a boolean flag.
- Some telemetry is not output. I will not output them yet, but keep them in mind for the future, should we decide we need them:
- copley_fault_status_register; note that similar latching registers are output.
- input_pin_states
- Several events are sent only when something (such as tracking, or a problem) starts, but not when it ends. We should send the event when the condition starts and when it ends, with suitable data (e.g. a boolean that is true if the condition is present).
- Most events and telemetry topics have a timestamp field. That is a waste of space because it duplicates private_sndStamp.
- The telemetry topic names are capitalized and so do not meet our current standards.
These are at least partly fixed in DM-21694:
- There is no detailed state event, even though that state is crucial for these CSCs. Fixed in
DM-21694by adding the controllerState event. - The deviceError event has a code field that is a string, and as implemented by the vendor, that "code" describes a single error so the event is output once for each error condition that is present. In
DM-21694I improved this by adding an applicationStatus field to the new controllerState event that has the bit mask. In addition I output deviceError once for a given set of error conditions, with code set to a comma-separated list of descriptions of each error so the event only when the error conditions change. I hope we can get rid of the deviceError event as duplicated information, even though a string can be nice.
This task will also clean up the Jira tickets:
Attachments
Issue Links
- mitigates
-
- Done
-
- Won't Fix
- relates to
-
DM-21949 Only output tracking and trackLost events when newly true
- Done
-
- Won't Fix
Activity
That sounds very reasonable to me. I would like to avoid updating the vendor's middleware code (unless we are sure we want a fallback). The only safe way to do that is to be sure that our replacement CSC is acceptable.
I see a merged PR on this ticket. Does that rectify all of the XML work? If so and it includes the settingsApplied work, can the CAP tickets be closed?
Michael Reuter The "merged PR" is for a different ticket: DM-21949. I have no idea why Jira is showing it with this ticket, though they are loosely related. I filed https://jira.lsstcorp.org/browse/IT-1587 since it appears to be a Jira bug.
I had to make changes in ts_hexrotcomm because it uses the Rotator XML. I took the opportunity to make some improvements in hexrotcomm:
- Rename the cmd field of Command to code and rename the cmd argument of BaseCsc.run_command to match.
- Rename the cmd argument of CommandTelemetryServer.put_command to command.
- Add BaseCsc.make_command and BaseCsc.run_multiple_commands. This is useful for safely sending multiple sequential commands to the low-level controller, as the Rotator move command now does.
Description of XML changes:
- https://confluence.lsstcorp.org/display/LTS/Hexapod+XML+Changes
- https://confluence.lsstcorp.org/display/LTS/Rotator+XML+Changes
Pull requests:
The update looks good and there is the consensus with the product owners already. Great job done!
Te-Wei's review for ts_xml:
1. Not sure this will affect the idl or not:
https://github.com/lsst-ts/ts_xml/blob/19707a9151cae96e52f5989db1948bad8ee8dde3/sal_interfaces/Hexapod/Hexapod_Commands.xml#L380
2. I thought offsetLUT will be affected as well:
https://github.com/lsst-ts/ts_xml/blob/19707a9151cae96e52f5989db1948bad8ee8dde3/sal_interfaces/Hexapod/Hexapod_Commands.xml#L441
My answer:
I fixed both issues and improved a documentation string at Tiago's request. (For the record: 1 only affected human readability, but was still worth fixing.)
Te-Wei's review
[base_csc.py]
1. Not sure the meaning of "Must include ``SET_STATE``". This is an Enum (CommandCode), right?
https://github.com/lsst-ts/ts_hexrotcomm/pull/7/files#diff-9884fc357e4ad38105bdf51d866033e9R65
Do you mean the user need to use "CommandCode.SET_STATE"? Sorry, my intuition is to check this file (enums.py) for such kind of enum. Where is it?
I am not sure this is what you impied or not?
When I began to review the PR of ts_rotator, I realized the CommandCode might be in the enum.py of ts_rotator. I thought the ts_hexrotcomm is the upstream of ts_rotator. If this is the case, is it a good idea to let the upstream code to have the knowledge of the code in downstream? Maybe the docstring can point out the CommandCode enum comes from the repo such as the ts_rotator to let other maintainer has the idea if my guess is correct. But maybe their relationship is not what I thought.
2. Do we need to consider the command might fail?
https://github.com/lsst-ts/ts_hexrotcomm/pull/7/files#diff-9884fc357e4ad38105bdf51d866033e9R252-R253
[simple_CSC.py]
1. I do not know the `salobj.Remote` can call this function in server:
https://github.com/lsst-ts/ts_hexrotcomm/pull/7/files#diff-2dcda03af7b6b135f00ed9059cc0bc30R119
How do we do this? `await m.cmd_move_sequentially.set_start()`? Is this correct?
<ts_rotator>
https://github.com/lsst-ts/ts_rotator/pull/8/files
[rotator_csc.py]
1. General question. I saw many `make_command()` and `run_command()` are used. This makes me thought you are using the Command Factory pattern. Is this correct? But I thought people will create a CommandFactory class and each command will be an object inherit from the default command class. Or maybe I am wrong in the guess of your intention?
<ts_hexapod>
https://github.com/lsst-ts/ts_hexapod/pull/9
<test_csc.py >
1. I thought the salobj also provides a timeout error?
https://github.com/lsst-ts/ts_hexapod/pull/9/files#diff-d9365181a6fb138131218b600a0dfccfR136
All changes look good to me! Great job done!
Thank you for the very helpful review. Here are my responses:
[base_csc.py]
1. CommandCode is an enum that specifies the command codes supported by the low-level controller for Rotator or Hexapod (or SimpleCsc). The only command code directly used by BaseCsc is SET_STATE, so the enum must include that item. I have expanded the text of the help string.
Note: Moog wrote just one command code enum. That one enum is used both for Hexapod and Rotator, even though some values are only relevant to one or the other. That is an implementation detail that I prefer not to duplicate. It is more flexible to allow the command codes for Rotator and Hexapod to be different, and it is less confusing to only include codes for commands that are appropriate to the specific device being controlled
2. Excellent question. Unfortunately Moog provided no reliable way to tell if a command has failed. It would be quite difficult to determine that at this level. In the long run I hope we can modify the interface to report failure immediately. Meanwhile I do my best to detect a command that cannot be executed in the CSC itself, and only try to command the low-level controller if I am confident the command can be executed.
[simple_CSC.py]
1. Great question. This method is only intended for unit tests and seemed reasonable to add to SimpleCsc because that is also only intended for unit tests. But I was still uneasy about it. Now that I look at it, I see it only uses public interfaces of the class, and is only called in test_csc.py,, so I will move the method there and simplify it.
[rotator_csc.py]
1. I am using a factory function: BaseCsc.make_command makes a hexrotcomm.Command. I will document the return value. I hope that will make it a bit clearer.
These commands are a struct sent to the low-level controller. Moog only defines one such struct (and it is used for both Rotator and Hexapod) so there is no need to have different types of Command.
Having a factory function is helpful for several reasons:
- I can set the sync_pattern field, which is specific to the device being controlled.
- the class is a ctypes.Structure, which means I cannot define the constructor. The factory function allows me to document the arguments, handle defaults, and check that the command code is a valid value.
<test_csc.py>
1. RemoteCommand.set_start always raises an AckError if it fails, so I provide a subclass of that for the special case that set_start times out before hearing from the CSC. However, for any other timeout I always use the standard asyncio TimeoutError.
Merged ts_xml to develop
Merged ts_hexrotcomm to develop and master and tagged as v0.2.0
Merged ts_hexapod to develop and master and tagged as v0.3.0
Merged ts_rotator to develop and master and tagged as v0.2.0
I propose this task should begin after the integration test in Dec. In that test, we should see the performance and behavior of new rotator CSC with the cooperation of CCW. The hexapod test might be postponed to Jan, 2020 depend on the test status in Dec. And the time is tight now for the test in Dec.
In addition, the update of xml interface should have the consensus with Doug and Bo as the product owners.