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

Fix #583, CFE_ES startup table #588

Closed
wants to merge 4 commits into from

Conversation

CDKnightNASA
Copy link
Contributor

@CDKnightNASA CDKnightNASA commented Apr 7, 2020

Describe the contribution
The following replaces the "startup script" code with a table file. Note this does NOT include unit test code changes, so CI will fail. This is for CCB consideration before I spend a bunch of time cleaning up UT.

Fix #583

Testing performed
Builds and runs.

Expected behavior changes
Replaces the startup script file with a start table that ES loads.

System(s) tested on
Debian 9

Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov

@CDKnightNASA CDKnightNASA added enhancement question CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 7, 2020
@CDKnightNASA CDKnightNASA requested review from a user and jphickey April 7, 2020 14:49
@skliper
Copy link
Contributor

skliper commented Apr 7, 2020

Wow, nice work! Looking forward to the CCB discussion...

@tngo67 What do you think from your end?

@tngo67
Copy link

tngo67 commented Apr 8, 2020

Wow, nice work! Looking forward to the CCB discussion...

@tngo67 What do you think from your end?

I see the advantages with this approach, it is cleaner & more in-line with the framework concept. But I also see some drawbacks. From the certification perspective, this will be treated as source code & hence needs to be certified, as oppose to data file, which might not. From dev & testing perspective, I can't add or remove an entry from my run without having to re-compile. From operation perspective, to update this info, I would now have to send command to ES to update its table, whereas, I use to just ftp the new startup script to the target & re-run CFS.

@jphickey
Copy link
Contributor

jphickey commented Apr 8, 2020

I like this!

@tngo67 this table has the same "data" as the existing cfe_es_startup.scr file (same fields, same purpose, etc) -- wouldn't they both need to be certified in the same way, regardless of the format? The only change here is that one is binary and the other is plain text, but the information inside, the purpose and execution is largely the same between them, so I would think whatever certification needs to be done would also be the same.

Also this table is only read and executed at startup, so the process of updating it is the same - send a new file (via FTP or whatever) and then restart CFS. You would not send table commands. The only difference is whether that file you FTP is plain text or binary. There would never be a need to send TBL management commands here.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks at this point, just things I'd like to see cleaned up prior to final merge. Overall I like the approach.

cmake/target/src/target_config.c Outdated Show resolved Hide resolved
cmake/target/src/target_config.c Outdated Show resolved Hide resolved
fsw/cfe-core/src/es/cfe_es_apps.h Outdated Show resolved Hide resolved
fsw/cfe-core/src/es/cfe_es_start.c Outdated Show resolved Hide resolved
fsw/cfe-core/src/inc/cfe_es_table.h Show resolved Hide resolved
@jphickey
Copy link
Contributor

jphickey commented Apr 8, 2020

Also note that I think there is a separate top-level target for tables, so to update the startup script one would just have to re-make this one target (not the whole CFE) and update just the same file. Granted, not quite as easy to edit as a text file, but most "real" (flight-like) targets don't have text editors onboard anyway, so either way to update the file one likely updating it on a PC and FTP'ing it in.

While it is one more step (build tables then ftp) most people probably have a script to do the FTP anyway, so that extra step could be hidden in the script. So at the end of the day the process could be almost the same.

@CDKnightNASA
Copy link
Contributor Author

Wow, nice work! Looking forward to the CCB discussion...
@tngo67 What do you think from your end?

I see the advantages with this approach, it is cleaner & more in-line with the framework concept. But I also see some drawbacks. From the certification perspective, this will be treated as source code & hence needs to be certified, as oppose to data file, which might not. From dev & testing perspective, I can't add or remove an entry from my run without having to re-compile. From operation perspective, to update this info, I would now have to send command to ES to update its table, whereas, I use to just ftp the new startup script to the target & re-run CFS.

WRT source vs data file, of course there are a number of tables used by a number of apps. (SCH, SC, LC are all far more "code-like" with their conditional execution.) So it would be good to resolve that tables are data and not source, or if that's impossible, reconsider the entire table services model. (My original suggestion was actually to enhance table services to allow for binary .tbl but also .json or .yaml or .csv or other structured text inputs as "tables". It would take more resources, of course.)

@CDKnightNASA
Copy link
Contributor Author

I've squashed some addl code fixes in, FYI

@CDKnightNASA CDKnightNASA marked this pull request as draft April 10, 2020 17:18
@CDKnightNASA CDKnightNASA changed the title fix #583 - strawman for CCB consideration, UT not working! fix #583 - CFE_ES startup table Apr 10, 2020
@jphickey
Copy link
Contributor

One issue that is quite an annoyance with the text-based startup file is that it lists the fully-qualified file name, including extension. The issue is that the extension varies from system to system, so when I build with e.g. SIMULATION=native the extensions need to be .so, but when building for RTEMS it needs to be .obj.

Ideally I'd like to see a solution that only lists the basename of the app file in the user-maintained source file, and the extra info, in particular the extension, is attached by the build system based on the file type of loadable modules for the particular platform you are building.

However the first step to any of this would be to get this table-based change in place, then smarter translation on the build side could be a follow-on.

@CDKnightNASA
Copy link
Contributor Author

One issue that is quite an annoyance with the text-based startup file is that it lists the fully-qualified file name, including extension. The issue is that the extension varies from system to system, so when I build with e.g. SIMULATION=native the extensions need to be .so, but when building for RTEMS it needs to be .obj.

Ideally I'd like to see a solution that only lists the basename of the app file in the user-maintained source file, and the extra info, in particular the extension, is attached by the build system based on the file type of loadable modules for the particular platform you are building.

However the first step to any of this would be to get this table-based change in place, then smarter translation on the build side could be a follow-on.

Could either CMake or OSAL provide the shared library extension?

@jphickey
Copy link
Contributor

Could either CMake or OSAL provide the shared library extension?

Yes, Cmake provides this as ${CMAKE_SHARED_LIBRARY_SUFFIX}

@astrogeco astrogeco changed the title fix #583 - CFE_ES startup table Fix #583, CFE_ES startup table Apr 14, 2020
@astrogeco
Copy link
Contributor

astrogeco commented Apr 15, 2020

CCB 20200415 - Gather information from the community on how they feel about non-optional table services. The "savings" from excluding table services are minimal. Revisit in next release.

@CDKnightNASA
Copy link
Contributor Author

Note that alternative implementations can either support the same TBL API's or it would not be difficult to write a shim between ES and TBL.

@CDKnightNASA CDKnightNASA removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 15, 2020
@CDKnightNASA CDKnightNASA modified the milestones: 6.8.0, 7.0.0 Apr 15, 2020
@skliper
Copy link
Contributor

skliper commented Apr 15, 2020

CCB 20200415 - Gather information from the community on how they feel about non-optional table services. The "savings" from excluding table services are minimal. Revisit in next release.

I suggest a survey of current major stakeholders on the toolchain/process change. I'm in favor of it (text parser on-board isn't great), but stakeholders may require an editor/viewer or similar way to minimize process impacts.

@ghost
Copy link

ghost commented Apr 16, 2020

I like the changes as well.
Was the decision made to no longer support the volatile disk startup file/script?
We originally supported this feature to allow a new feature or replacement app to be tested in flight without writing to nonvolatile memory. In the past GSFC missions, updating EEPROM or Flash is a big deal.
A replacement app could be tested by loading it to the cFS RAM disk and using a modified startup script in the cFS RAM disk. This way the app could be tested without committing anything to EEPROM/Flash.

An alternative would be reloading an app from the RAM disk without a reboot, but does that work in all cases?

Are we losing capability by not offering the RAM based startup?

@CDKnightNASA
Copy link
Contributor Author

Are we losing capability by not offering the RAM based startup?

I can consider how we might continue support of RAM-based startup. One option might be to add an ES command that kicks off a table load.

@skliper
Copy link
Contributor

skliper commented Apr 27, 2020

@astrogeco 's call, but might be easier in the end if you rebase your branch on master vs merge master into your branch

@CDKnightNASA
Copy link
Contributor Author

@astrogeco 's call, but might be easier in the end if you rebase your branch on master vs merge master into your branch

Yeah sorry I'm actually trying to rebase and got git into a bind. :|

@skliper
Copy link
Contributor

skliper commented Apr 27, 2020

NP, I've been rebasing like crazy w/ all sorts of fun merge conflicts lately... end of cycle crunch.

@CDKnightNASA
Copy link
Contributor Author

closing, will re-do with unit tests when ready

@CDKnightNASA CDKnightNASA deleted the fix-583-es_tbl branch April 28, 2020 13:03
@skliper skliper removed this from the 7.0.0 milestone Jul 13, 2020
@skliper skliper added invalid and removed question labels Aug 21, 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.

ES should use a table for the "startup script"
5 participants