Fix #945, shell implementation on posix and rtems#946
Merged
astrogeco merged 1 commit intonasa:integration-candidatefrom Apr 21, 2021
Merged
Fix #945, shell implementation on posix and rtems#946astrogeco merged 1 commit intonasa:integration-candidatefrom
astrogeco merged 1 commit intonasa:integration-candidatefrom
Conversation
jphickey
added
the
CCB:Ready
Due to other changes the shell implementation on RTEMS and POSIX failed to build under certain configurations. This fixes and verifies the shell functions as expected.
jphickey
force-pushed
the
fix-945-rtems-shell
branch
from
f9d571a to
7710202
Compare
skliper
added
CCB:2021-04-14
and removed
CCB:Ready
Contributor
|
CCB:2021-04-14 - APPROVED |
astrogeco
added
the
CCB:Approved
astrogeco
added a commit
to nasa/cFS
that referenced
this pull request
Apr 22, 2021
Combines: nasa/cFE#1406 nasa/osal#967 nasa/cFS-GroundSystem#178 Includes: nasa/cFE#1290, Split interface and implementation modules nasa/cFE#1376, add docs to CFE_ES_RegisterCDS() regarding clearing nasa/cFE#1292, Remove testrunner and convert testcase to app cfe-IC:2021-04-20, HOTFIX: Always build cfe_assert. nasa/osal#950, Eliminate time and access name collisions with VxWorks nasa/osal#946, Fix Shell implementation on posix and rtems nasa/cFS-GroundSystem#174, update executable name and version in setup.py nasa/cFS-GroundSystem#175, Add executable install guide
astrogeco
added a commit
to nasa/cFS
that referenced
this pull request
Apr 22, 2021
Combines: nasa/cFE#1406 nasa/osal#967 nasa/cFS-GroundSystem#178 Includes: nasa/cFE#1290, Split interface and implementation modules nasa/cFE#1376, add docs to CFE_ES_RegisterCDS() regarding clearing nasa/cFE#1292, Remove testrunner and convert testcase to app cfe-IC:2021-04-20, HOTFIX: Always build cfe_assert. nasa/osal#950, Eliminate time and access name collisions with VxWorks nasa/osal#946, Fix Shell implementation on posix and rtems nasa/cFS-GroundSystem#174, update executable name and version in setup.py nasa/cFS-GroundSystem#175, Add executable install guide
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the contribution
Fix #945
Due to other changes the shell implementation on RTEMS and POSIX failed to build under certain configurations.
active_idfield was not valid. Using the existingOS_CloseAllFiles()function does what is needed here and is much simpler.For now, the broken impl file is simply deleted, and RTEMS will always use
os-impl-no-shell.c. In a future version, the implementation can be fixed properly.Testing performed
Build POSIX and RTEMS with shell enabled (OSAL_CONFIG_INCLUDE_SHELL=true)
Run "shell-test" and confirm behavior on POSIX (passes) and RTEMS (skips)
(Note that CFE does not directly use the OSAL shell function, but apps might; it is optional)
Expected behavior changes
Broken code is removed, so OSAL builds with
OSAL_CONFIG_INCLUDE_SHELL=trueon all platforms, but RTEMS will still result in OS_ERR_NOT_IMPLEMENTED always.System(s) tested on
Ubuntu 20.04 (native)
RTEMS 4.11.3
Additional context
I briefly investigated what it would take to fix this on RTEMS properly. The issue is that the
rtems_shell_init()function attaches to a "device name" (typically /dev/console for interactive use), not individual input/output file descriptors. So it appears to be a matter of creating a pseudoterminal-type device that "reads" from the script and writes to the file. But it also appeared this was an area that changed a fair bit in RTEMS between 4.11 and 5.x release, so any solution would be unlikely to work on both, and this is why I decided its probably not worth pursuing for now. (a stakeholder who wants the shell to work on RTEMS can always re-implement).But since the existing code in
os-impl-shell.cwas very broken, it is just removed (not even useful as a basis for future; better to use posix or vxworks as basis instead).Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.