-
Notifications
You must be signed in to change notification settings - Fork 203
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
Conversation
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. |
I like this! @tngo67 this table has the same "data" as the existing 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. |
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.
A few nitpicks at this point, just things I'd like to see cleaned up prior to final merge. Overall I like the approach.
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. |
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.) |
17de99b
to
e4b5eb9
Compare
I've squashed some addl code fixes in, FYI |
e4b5eb9
to
9912a82
Compare
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. 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? |
Yes, Cmake provides this as |
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. |
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. |
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. |
I like the changes as well. 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? |
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. |
@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. :| |
NP, I've been rebasing like crazy w/ all sorts of fun merge conflicts lately... end of cycle crunch. |
closing, will re-do with unit tests when ready |
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
[email protected]