(WIP) Trivial header redefine #721
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| /* Mission custom command and telemetry packet headers */ | ||
| #include "ccsds.h" | ||
|
|
||
| typedef struct | ||
| { | ||
| CCSDS_SpacePacket_t SpacePacket; /**< \brief Standard Header on all packets */ | ||
| CCSDS_CmdSecHdr_t Sec; | ||
| uint32 customfield; | ||
| } CCSDS_CommandPacket_t; | ||
|
|
||
| /*----- Generic combined telemetry header. -----*/ | ||
|
|
||
| typedef struct | ||
| { | ||
| CCSDS_SpacePacket_t SpacePacket; /**< \brief Standard Header on all packets */ | ||
| CCSDS_TlmSecHdr_t Sec; | ||
| } CCSDS_TelemetryPacket_t; | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,6 +184,9 @@ typedef struct | |
|
|
||
|
|
||
|
|
||
| #ifdef MESSAGE_FORMAT_IS_CUSTOM | ||
| #include "cfe_mission_custom.h" /* Should resolve/refactor to avoid including mid-file */ | ||
| #else | ||
|
There was a problem hiding this comment. As the "cfe_mission_custom.h" file needs to exist regardless, couldn't it just be generated to always have the "right" content in it (i.e. the user-selected format) and not have to introduce yet another preprocessor switch? There was a problem hiding this comment. No it doesn't? Did you try it? Works exactly like before (remove the file, don't define a format and it's V1, define V2 and it works also... I confirmed it. Only needs to exist if you want to redefine. There was a problem hiding this comment. My point is that you've called So why would we want to generate an empty file and then selectively ignore it? It would be simpler, cleaner to just generate the file with the right content. No There was a problem hiding this comment. Also note that the whole There was a problem hiding this comment. I agree 100% with adopting the source selection and CMake techniques implemented in OSAL. I have suggested that approach multiple times, but it hasn't seemed to have resonated with the group. For the previous comment - because it works with no impacts on previous configurations. There's certainly a trade to be made (it can certainly be implemented either way), I'll leave it up to @jwilmot @acudmore since the long term open source solution would help from architecture guidance. There was a problem hiding this comment. Well since we already have both (CMake config file + C headers) in use my recommendation is to favor the CMake method for new development, because it covers both areas (build + code). That is, we can control the build and also easily generate a synchronized C header from the CMake variables to influence the actual code. Using a strictly C header approach is more limiting, as it is not really possible to go the other way, so the information isn't available to CMake for build/source selection. There was a problem hiding this comment. And I continue to agree 100% on improving this to utilize CMake instead of defines in c headers. It was the solution I was hoping for and was trying to encourage. There was a problem hiding this comment. I do still prefer the core framework implements the basic two formats in the code such that it's covered by cppcheck (I'm not a fan of moving the supported header definitions into sample_defs and generating, but that's a trade we can discuss), but I definitely prefer using CMake to do the configuration work. There was a problem hiding this comment. My point is I don't want users to think they can redefine what version 1 and 2 means (could be implied by moving the definition to sample_defs). I'd rather control those in the core code. |
||
| /*----- Generic combined command header. -----*/ | ||
|
|
||
| typedef struct | ||
|
|
@@ -199,6 +202,7 @@ typedef struct | |
| CCSDS_SpacePacket_t SpacePacket; /**< \brief Standard Header on all packets */ | ||
| CCSDS_TlmSecHdr_t Sec; | ||
| } CCSDS_TelemetryPacket_t; | ||
| #endif | ||
|
|
||
| /* | ||
| * COMPATIBILITY TYPEDEFS: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we need a compile time switch for custom/not custom header format? Every mission needs one (and only one) defined message format. It should just be "the selected message format".
I assume "not custom" means the traditional CCSDS v1 with CFE-defined secondary headers, using the 6-byte TLM header? Does that mean every user who wants v2/extended header needs to enable this "custom" switch?
I'd rather see something where the user just specifies one specific header format, by name. If that happens to be one that CFE provides out of the box, then great. Otherwise the user will have to provide an implementation to go with their selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to break everyone's config if they don't define version 1? We certainly can take that approach if it's preferred. I went with minimizing impacts (it works just like default (v1), define for V2 as before, but adds an additional IS_CUSTOM).
Could also have unique typedefs for each option, and just define as a common name here. Is that what you are asking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't break existing config because it can be done on the CMake side. If CMake needs to do some sort of source selection based on the header format then CMake needs to know what the user wants, so it can select the files appropriately.
So it would work just like anything else - CMake has a default, which is used if the user does nothing, and if the user sets something then it uses that.
Besides that, even if it was only in the C header file, it's just a matter of putting the default value into the sample file. (but again, setting in the value via CMake is totally backward compatible provided we give a default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could certainly be done multiple ways and I was hoping for a CMake related update (per the design suggestions I provided) but I haven't seen progress on that front in line with our allotted schedule. Given that, I made these trivial changes that also work to redefine the header. This is certainly not meant as the perfect solution, it's just a trivial solution that works so I can still make schedule if a better solution is not complete in time.