Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close #80, Add build number and baseline #86

Conversation

astrogeco
Copy link
Contributor

@astrogeco astrogeco commented Aug 5, 2020

Describe the contribution
Close #80

Testing performed
Built bundle and confirmed SAMPLE_APP reports development version.

Expected behavior changes
Version report now uses the version string. See excerpt from cfs log:

EVS Port1 42/1/SAMPLE_APP 1: SAMPLE App Initialized Sample App Development Build v1.1.0+dev64 Last Official Release: v1.1.0

System(s) tested on
Ubuntu Docker on Mac OS X

Additional context
Add any other context about the contribution here.

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Gerardo E. Cruz-Ortiz, NASA-GSFC

@astrogeco astrogeco added this to the 1.2.0 milestone Aug 5, 2020
@astrogeco astrogeco requested a review from skliper August 5, 2020 02:22
@astrogeco astrogeco changed the base branch from main to integration-candidate August 5, 2020 02:22
@skliper
Copy link
Contributor

skliper commented Aug 5, 2020

Are multi-line events allowed?

@astrogeco
Copy link
Contributor Author

astrogeco commented Aug 5, 2020

Are multi-line events allowed?

Who can we ask? Or where can we check? They print out fine on the console and output file.

@skliper
Copy link
Contributor

skliper commented Aug 5, 2020

I'd be more comfortable with following the standard one line format unless there is a critical reason to change. I'm not aware of any other event anywhere that has new lines. Could impact ground stations, event processing/handling tools, etc. I wouldn't think the effort to verify multi-line doesn't break anything across all the ground test and ops tools/teams is worth it in this case.

@astrogeco
Copy link
Contributor Author

I'd be more comfortable with following the standard one line format unless there is a critical reason to change. I'm not aware of any other event anywhere that has new lines. Could impact ground stations, event processing/handling tools, etc. I wouldn't think the effort to verify multi-line doesn't break anything across all the ground test and ops tools/teams is worth it in this case.

Fair enough. I did it because it is more readable. Maybe bring it up at the CCB and see what others think?

@jphickey
Copy link
Contributor

jphickey commented Aug 5, 2020

I would concur with sticking to a single line. The FSW doesn't care - EVS passes it through no problem - but ground system might not display multiple lines correctly.

@astrogeco
Copy link
Contributor Author

I think it's worthwhile to check if the different ground systems can handle it. I can keep it as one line for now though.

@astrogeco astrogeco force-pushed the 80-add-build-number-and-baseline branch from 9a45d32 to cb48edc Compare August 5, 2020 13:43
@ghost
Copy link

ghost commented Aug 5, 2020

I agree with keeping it on one line. I'm not sure I like it on the console, since I am used to looking at messages from an app in a single line. Also don't know if it will trip up ground system parsing.

@astrogeco astrogeco force-pushed the 80-add-build-number-and-baseline branch 2 times, most recently from 5723664 to dc57b7a Compare August 5, 2020 13:45
@astrogeco
Copy link
Contributor Author

If the ground station trips up then that's definitely a problem.

\begin-rant

I know we're used to seeing things a certain way. I just want to point out that we want to make sure that messages stand out and are obvious. This is especially important as we aim to grow the cFS user base. The one-line event now gets buried. And thus the message is lost to the user.

1980-012-14:03:20.26549 ES Startup: Loading file: /cf/sample_app.so, APP: SAMPLE_APP
1980-012-14:03:20.26835 ES Startup: SAMPLE_APP loaded and created
1980-012-14:03:20.27038 ES Startup: Loading file: /cf/ci_lab.so, APP: CI_LAB_APP
EVS Port1 42/1/SAMPLE_APP 1: SAMPLE App Initialized Sample App Development Build v1.1.0+dev64 Last Offical Release: v1.1.0
1980-012-14:03:20.27351 ES Startup: CI_LAB_APP loaded and created
1980-012-14:03:20.27413 ES Startup: Loading file: /cf/to_lab.so, APP: TO_LAB_APP
1980-012-14:03:20.27428 CI_LAB listening on UDP port: 1234

Text should be formatted for humans. Spaces and line breaks help us process information more effectively.

\end-rant

Add build number and baseline macros
Add version string macros
Use new version string in noop and version report
@astrogeco astrogeco force-pushed the 80-add-build-number-and-baseline branch from dc57b7a to 03f4efa Compare August 5, 2020 14:00
@jphickey
Copy link
Contributor

jphickey commented Aug 5, 2020

FWIW, the version info should really be a separate event from the startup info.

At startup, it can send both, but the NOOP command only sends the version info event. That would put them on separate lines (startup + version info) and also keep each event to one line.

@astrogeco
Copy link
Contributor Author

FWIW, the version info should really be a separate event from the startup info.

At startup, it can send both, but the NOOP command only sends the version info event. That would put them on separate lines (startup + version info) and also keep each event to one line.

I just followed the existing pattern but it sounds like it's worth checking that all the sample and lab apps are using the noop to communicate the version.

@astrogeco
Copy link
Contributor Author

FWIW, the version info should really be a separate event from the startup info.
At startup, it can send both, but the NOOP command only sends the version info event. That would put them on separate lines (startup + version info) and also keep each event to one line.

I just followed the existing pattern but it sounds like it's worth checking that all the sample and lab apps are using the noop to communicate the version.

Also noticed that some apps are using OS_printf versus CFE_EVS_SendEvent

@jphickey
Copy link
Contributor

jphickey commented Aug 5, 2020

CFE ES does it (more) properly - it defines a separate CFE_ES_VERSION_INF_EID (91) and sends the version info on this EID, as a separate event from the startup event. Notably, this puts it on the separate line on the console (more human friendly) - and gives a specific event id that the GSW can look for to obtain version info (more software friendly).

I do concur that the version info being "buried" inside other event strings is not ideal - but I would prefer to see separate event IDs, rather than having one event span multiple lines.

@astrogeco astrogeco merged commit 716fe43 into nasa:integration-candidate Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Build name and Build number to version.h
3 participants