Fix #821, add accessor functions for version strings

[jphickey]

Describe the contribution
Adds 3 simple API calls:

const char *OS_GetVersion(void);
const char *OS_GetVersionCodeName(void);
const char *OS_GetVersionDescription(void);

These directly return the values of string macros defined in osapi-version.h.

The accessor function should be the preferred way to get the OSAL version string (vs. using macro directly) as it is evaluated at OSAL library compile time, rather than application compile time, and thus will remain correct in the event that OSAL is relinked without recompiling the application.

Fixes #821

Testing performed
Build and run all unit tests, sanity check CFE.

Expected behavior changes
This adds new APIs to get version strings - does not change anything existing within OSAL.

System(s) tested on
Ubuntu 20.04

Additional context
Looking at the existing macros I determined we really needed 3 functions for version info:

1. There is the "simple" version currently in OS_VERSION macro - this is the semantic version without any extra detail. This is returned by OS_GetVersion().
2. There is the "descriptive" version currently in OS_VERSION_STRING macro - this has extra detail like the most recent official release. This is returned by OS_GetVersionDescription().
3. Finally the release code name, which might be useful to obtain separately/in isolation from the rest of the details.. I separated this from the OS_VERSION_STRING macro and this is returned by OS_GetVersionDescription().

I resisted the temptation to simply toss these into osapi-common.c and instead added a new/separate source file called osapi-version.c to implement these 3 calls. In turn this meant a new coverage test needed to be added too (since the pattern in OSAL is one coverage test per file unit).

Putting this in a separate source file is better as it maintains the pattern/relationship between .c and .h files, and it also allows the version.c file to be auto-generated at some point if we decide to go that route in the future.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Corrected some whitespace style diffs, passed through clang-format-10 again.

[jphickey]

If folks like this general approach (and I do think its much better than using a macro) then two more suggestions/questions:

1. Should we also add an API to get the semantic version as numbers rather than strings (e.g. major/minor/rev)
2. Should we add the same thing to PSP and CFE too (I think we should).

[skliper]

1. Should we also add an API to get the semantic version as numbers rather than strings (e.g. major/minor/rev)

Seems like a better approach than OSAL_API_VERSION (runtime vs compile time)

2. Should we add the same thing to PSP and CFE too (I think we should).

I can't think of any reason why not...

[astrogeco]

I vote for keeping it a string and letting people create their own parsers for SemVer if they care. When I was looking into this a while ago, it seems like the standard practice is to have a plain-text VERSION file with a string and the code then grabs this info from the file when reporting.

Granted, I didn't do an exhaustive search or literature review:

• Rust-lang /src/version
• Julia-lang VERSION
• GitLab VERSION

[skliper]

@CDKnightNASA I don't think we are proposing removing the string version, just adding the API to get numbers.

[astrogeco]

@CDKnightNASA I don't think we are proposing removing the string version, just adding the API to get numbers.

I was referring to Joe's question (1) about getting each element as a number in adition to a string.

What would be the impact of removing the MAJOR, MINOR, etc. macros altogether?

[jphickey]

What would be the impact of removing the MAJOR, MINOR, etc. macros altogether?

Well, it would simplify things, for sure :-)

I guess the advantage of reporting version as a set of 4 integers is that it limits the version to four integers.

If its a free form string, then (by definition) formatting isn't strictly enforced, it is only a matter of time until someone releases an OSAL version tagged as v6.0.1a (IIRC we actually did this with 4.2.1a) and the presence of a letter someone's semantic version parser will break it. Restricting to four integers - which are actually returned as uint8 - prevents this.

But the only actual use of numeric version that I can think of is to determine API compatibility at application compile-time (e.g. to check of compiling with a version of OSAL that has a certain function implemented). There are other ways to address that problem though - Checking a 4-part version number is rather complex to do correctly and not an ideal indicator of API level.

[jphickey]

Checking the code again - we do have a OSAL_API_VERSION macro that is a combination of major, minor, and rev number put into a form for easier preprocessor checking. We should probably keep this around, but it can be simplified (e.g. changed to simply 60000 rather than as an concatenation of OS_MAJOR_VERSION, OS_MINOR_VERSION, and OS_REVISION.

With that - as far as I can tell I don't see any real value for keeping the individual integers - EXCEPT for the comment earlier about the fact that it implicitly enforces that version is always expressible as four simple integers. If we don't need to restrict this, then numbers should probably go away.

[jphickey]

OK - I did think of another possible use case for the numeric version number - it currently refers to the last "official" release, and is actually the only way to get that info.

• The "OS_BUILD_BASELINE" refers to the last tag which is a pre-release, not official release.
• The only other way the official version is reported is that it is buried inside the long description string (i.e. Latest Official Version: osal v5.0.0).

The problem is that the latter is not really parseable - it is a human-friendly string, not a code-friendly string. So if code needs to report the official release, the only way to do so is to print the entire description string, which has a lot of other stuff in it. There is no way to get the official release version in isolation from the rest of the info.

Not meaning to add another option to the mix - but if we DO keep the numeric version and define it as the "official release" - this means that the long description string becomes redundant. All of the real info in that string would become obtainable separately, so CFE could simply pretty-print the info on its own.

So the set API-accessible version items becomes:

1. Baseline (string; version control tag, may be pre-release/unofficial)
2. Code name (string)
3. Last Official release (numeric, set of 4 integers)
4. Build number (numeric)

When CFE pretty-prints in its reporting event, it can just assemble all of these elements together (easy enough). While not a huge simplification, it does mean that CFE-generated events would be consistently formatted. Right now these long strings are somewhat arbitrary and not necessarily identical between components. (note that today some have newlines, some do not, capitalization and punctuation varies)

[jphickey]

Final thought on that - as long as we don't care about restricting to numbers, we could also make (3) above as a string, too.

In that case "baseline" (1) and "last official release" (3) would report identical strings on official releases, but different strings on development versions. This makes it easy to tell the different when reporting.

I'll put this aside and wait for consensus before updating this PR. In the meantime in nasa/cFE#1170 (which was the impetus of all this) I've worked around this in a temporary/interim method which is probably good enough for now - but I would definitely like to get something more coherent for the 6.0 release - right now our version reporting is still a mis-mosh of different ideas and patterns and really hard to understand.

[skliper]

The numbers are used in telemetry though, really cFE should report not only cFE and OSAL as it does now, but PSP also. Maybe these all could be switched to strings? I don't have a problem with that, but a tlm change is more impact. Although with modules the single cFE version also poses a challenge...

[astrogeco]

The numbers are used in telemetry though, really cFE should report not only cFE and OSAL as it does now, but PSP also. Maybe these all could be switched to strings? I don't have a problem with that, but a tlm change is more impact. Although with modules the single cFE version also poses a challenge...

I vote for changing the telemetry to output a string as opposed to individual numbers.

Agree that cFE telemetry should report on PSP as well

[jphickey]

Good point, I forgot about the ES housekeeping telemetry packet - this has version numbers as integers.

I have no complaints if this were changed to strings but ground system people might have an opinion.

[astrogeco]

Random question, do we have other osal or psp info in telemetry?

[jphickey]

The only downside that I can see to reporting as strings is that what is now 4 bytes in telemetry (x2 = 8 bytes total) will need at least 32 bytes as a string, and we are proposing 3 of them to include PSP, so it needs 96 bytes - a pretty big increase in size of the TLM packet.

[ghost]

Regarding the CCB conversation, I am fine with integers in telemetry packets, 99 for a development version, and strings in EVS messages. The development version for RTEMS 4.11 is 4.11.99 for example. (RTEMS 5 is taking a different approach, which can be seen here: https://devel.rtems.org/wiki/Developer/Release under "RTEMS Release 5 Series And Higher Numbering"

[astrogeco]

Hi everyone, I created an issue at the bundle level to keep the discussion going on this without completely derailing the api solution from this PR. See nasa/cFS#200

[astrogeco]

@jphickey what do you think about replacing OS_GetVersionDescription() with a "verbose" option of sorts to OS_GetVersion()?

[jphickey]

I really prefer to just return the "raw material" of the version info without the extra wordiness/punctuation/etc. Then ES (or whatever sends the full/descriptive info) can assemble into a complete, user friendly string. This way it will be more consistent.

[astrogeco]

I really prefer to just return the "raw material" of the version info without the extra wordiness/punctuation/etc. Then ES (or whatever sends the full/descriptive info) can assemble into a complete, user friendly string. This way it will be more consistent.

I was thinking something like a varargs where OS_GetVersion() gives the basic stuff but OS_GetVersion(verbose) returns the extra details. I don't know if that's a valid pattern though

[jphickey]

I was thinking something like a varargs where OS_GetVersion() gives the basic stuff but OS_GetVersion(verbose) returns the extra details. I don't know if that's a valid pattern though

This just seems overly complex, for something that should be really simple...

I concur with the comments in nasa/cFS#200 (comment) that we should first identify specifically what the intent/use-case of this info is, then report the items that satisfy that use-case, and don't make it too complicated.

[jphickey]

Updated per discussion/consensus in nasa/cFS#200. The runtime API now has 4 calls:

const char *OS_GetVersionString(void);
const char *OS_GetVersionCodeName(void);
void OS_GetVersionNumber(uint8[4]);
uint32 OS_GetBuildNumber(void);

Note this change does not affect the existing macros, it just adds the APIs to get the same values. The intent is to not break any code currently depending on the macros, but just to add the API as the preferred way of getting the info at runtime.

[skliper]

This won't match, but in a quick scan I don't see code dependent on 0xFF.