Add EDL commands to XTCE generator #37
Conversation
- uslp map id is 4 not 6 bits - fix encode/decode of pascal byte strings to prefix size of bits - version is set to oresat-configs version - add missing type annotations - skip edl commands for oresat0 xtce
- replace the "|" for long string with ">-" in yamls, to not include new lines - the time sync command does not return anything in oresat-c3, tho it documented as returning a bool. Since this represent the code as is, I remove the response, we can re-add it later, if wanted.
| - string: `"str"` (NOTE: `fix_size` or `max_size` must be set.) | ||
| - binary: `"bytes"` (NOTE: `fix_size` or `size_prefix` must be set.) |
There was a problem hiding this comment.
It took a bit for me to grok this, I think it might make more sense to have different unique types for the fixed and dynamic length versions of str and bytes. I usually see the dynamic str here referred to as c-style strings so maybe cstring or cstr and the dynamic length bytes as length-value or LV so maybe lvbytes. If you squint you could probably also get away with bytearray as the python type vaguely has the same connotation.
This way all str must and only must have fixed_size set and cstr must only have max_size set. Similarly with bytes and fixed_size, and lvbytes and size_prefix. Each type now has a unique behavior instead of conditional overlapping behaviors.
| def decode_request(self, raw: bytes) -> tuple[Any]: | ||
| """Decode a EDL request payload.""" | ||
| return self._decode(raw, self.request) | ||
|
|
||
| def encode_request(self, values: tuple[Any]) -> bytes: | ||
| """Encode a EDL request payload.""" | ||
| return self._encode(values, self.request) | ||
|
|
||
| def decode_response(self, raw: bytes) -> tuple[Any]: | ||
| """Decode a EDL response payload.""" | ||
| return self._decode(raw, self.response) | ||
|
|
||
| def encode_response(self, values: tuple[Any]) -> bytes: | ||
| """Encode a EDL reponse payload.""" | ||
| return self._encode(values, self.response) |
There was a problem hiding this comment.
In non-test code there's no single user that will use both encode_request() and decode_request() at the same time (similarly with the response()s). This presents an API that can be misused, I'd like to see it presented in a way that would be harder to "you're holding it wrong" with.
I'd do it with a separate class for the sender and the receiver, or client/server, source/dest, etc. Something like:
class EdlCommandSource:
def __init__(self, request, response):
self.decode_fmt = response
self.encode_fmt = request
def decode(self, raw: bytes) -> tuple[Any]:
...
def encode(self, values: tuple[Any]) -> bytes:
...
class EdlCommandDest(EdlCommandSource):
def __init__(self, request, response):
super().__init__(response, request)With this the ground scripts only use EdlCommandSource and EdlService only uses EdlCommandDest and correct use is ensured via the type system.
As an added bonus most of the struct fmt strings can be generated once in __init__ instead of each time .encode()/decode() is called.
There was a problem hiding this comment.
I don't see the need for this.
| @@ -65,7 +65,9 @@ linters = "pycodestyle,pyflakes,pylint,mccabe,mypy,radon" | |||
| # E0401: Cannot find implementation or library stub for module named | |||
| # R0902: Too many instance attributes | |||
| # W0511: TODOs or FIXMEs | |||
| ignore = "E402,C901,C0103,E203,R0912,R0915,R901,R901,R0914,C0413,C0206,R1716,W1514,R1702,E0401,R0902,W0511" | |||
| # W0102: Dangerous default value for function argument | |||
There was a problem hiding this comment.
W0102 should really be left enabled, especially if we expect people newer to python to contribute. The dangerous default values, typically an empty collection in function arguments, are one of the classic python warts that will catch the unaware.
Your specific uses in this PR don't run afoul of the danger since the uses are read only, but in general the accepted way of doing what you want is to have the default value be None and then check for none in the body and assign the empty collection to it there. For example
def _add_parameter_type(..., value_descriptions: dict[str, int] = {}, ...):
...becomes
def _add_parameter_type(..., value_descriptions: Optional[dict[str, int]] = None, ...):
if value_descriptions is None:
value_descriptions = {}
...| attrib={ | ||
| "name": "unix_time", | ||
| "shortDescription": "Unix coarse timestamp", | ||
| "name": "edl_responses", |
There was a problem hiding this comment.
I've been trying to come up to speed on XTCE too and I agree it's pretty confusing. I've found the project xtcetools and also yamcs quickstart have example XTCEs that include some CCSDS related setup. Also these slide decks have some instruction of moderate help.
They all seem to do inheritance differently than what you've got here. Where you're driving it via the single top level container edl_responses and selecting with IncludeCondition they go the other way, making heavy use of RestrictionCriteria on BaseContainer. I think it should be something like:
flowchart TD
USLP-- "RestrictionCriteria\nuslp_virtual_channel_id == 0" -->edl(EDL Response)
edl--"RestrictionCriteria\nedl_command_code == 0"-->tx_control_response
edl--"RestrictionCriteria\nedl_command_code == 4"-->co_node_enable_response
edl--"RestrictionCriteria\nedl_command_code == 5"-->co_node_status_response
edl-->...
There was a problem hiding this comment.
I agree with the way you describe as it was the way I originally tried to implement this as it makes more sense, the issue I ran into I could only use IncludeConditionRestrictopCriteria once in a sequence definition according to the xsd spec.
There was a problem hiding this comment.
The key thing is I wanted on root contianer for Yamcs to point to to decode edl responses.
| ET.SubElement( | ||
| cmd_entry_list, | ||
| "FixedValueEntry", | ||
| attrib={"name": "hmac", "binaryValue": "0" * 64, "sizeInBits": str(32 * 8)}, | ||
| ) | ||
| ET.SubElement( | ||
| cmd_entry_list, | ||
| "FixedValueEntry", | ||
| attrib={"name": "uslp_fecf", "binaryValue": "0000", "sizeInBits": "16"}, | ||
| ) |
There was a problem hiding this comment.
xtcetools' BogusSAT-1 has an example of how to do a packet footer with BaseContainer. Apparently you need to set LocationInContainerInBits referenceLocation="containerEnd" on the ParameterRefEntrys that you want at the end of a container, and then it'll arrange for those fields to come after all the fields in anything that uses the container as a BaseContainer. I'm unclear if there's other tags that will take referenceLocation or if you only have bits to specify.
But so yeah, hmac and uslp_fecf can be moved into uslp_header (possibly renamed to just uslp).
There was a problem hiding this comment.
Interesting, I did not see that. I'll try that out.
There was a problem hiding this comment.
This only worked for parameters, not arguments
|
I've been running this in yamcs and I'm now more confused than I was. Yamcs has support for USLP packet decoding but I can't as of yet find the encoding parts. Are we defining USLP here in XTCE to replace that? If USLP encoding really is missing, why wouldn't we follow their lead and implement it as a |
To answer one of my own questions they've explicitly documented that only support for TC and CLTU are implemented for telecommands. |
| from dacite import from_dict | ||
| from yaml import CLoader, load | ||
|
|
||
| _COMMAND_DATA_FMT = { |
There was a problem hiding this comment.
Similar to "magic numbers", are we using magic letters here? Or is this tied to a spec?
There was a problem hiding this comment.
These are struct format characters. You can see them being used down below in struct.pack() and struct.unpack() for format strings.
|
|
||
| def decode_request(self, raw: bytes) -> tuple[Any]: | ||
| """Decode a EDL request payload.""" | ||
| return self._decode(raw, self.request) |
There was a problem hiding this comment.
I'm having a hard time grokking why self.request is embedded into the object but values are not.
Is this object intended to keep-state on anything?
There was a problem hiding this comment.
Nope, it is just the definition
| description: OPD node status | ||
|
|
||
| - uid: 0xE | ||
| name: rtc_set_time |
There was a problem hiding this comment.
I know this is a bit bigger than this PR, but I would advocate for all commands to have one of either
- Always having an ack
- Always having some kind of response
In this case, maybe have the vehicle respond with the new rtc time it has?
There was a problem hiding this comment.
Make this an issue in oresat-c3-software
There was a problem hiding this comment.
All commands are the current one in c3-software as is, we can changes this after this PR and https://github.com/oresat/oresat-c3-software/pull/37PR are both merged.
Added a new
edl_cmd_defs.yamlconfig to define EDL commands and added all existing EDL commands to it.Updated the XTCE generator to also include the EDL commands for non-OreSat0 missions (OreSat0 existed before oresat-configs).
Mostly had to read thru https://www.omg.org/spec/XTCE/20180204/SpaceSystem.xsd to figure out the XTCE format for generator.
Yamcs can be pointed to
beacontelemetry container,edl_responsestelemetry container, and the telecommand container from the generated XTCE.Also added encoder / decoder for the EDL payloads that can be used by the C3 software rather than the current hard-coded code.
I have not tested the
edl_responsetelemetry container with Yamcs.