-
Notifications
You must be signed in to change notification settings - Fork 218
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 #1220 Implement OS_ModuleGetInfo_Impl for RTEMS #1221
Fix #1220 Implement OS_ModuleGetInfo_Impl for RTEMS #1221
Conversation
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.
Recommend testing in open source RTEMS 4.11 and 5 workflows with the latest bundle and confirm the info is valid.
Does this address #1205 ? |
CCB:2022-02-23 Approved with CHANGES
|
Shouldn't need to change anything, just run it and make sure it confirms the code was implemented correctly. Shouldn't report as not implemented anymore. |
Would the test run automatically with the current CI then? I don't quite understand how the not-implemented "flag" gets picked up by the test script. |
It would run in the bundle CI on RTEMS 4.11 and 5 for an emulated x86 target (or whatever we have configured in there), but worth confirming the functional test succeeds on the target mentioned above. The functional test basically just skips the actual functionality test if the unit under test responds with not-implemented (I think it reports as not applicable or similar). The test doesn't "fail" either way in CI, which is why verifying it actually does execute and pass is recommended. |
CCB: 2022-03-02 APPROVED
|
e158392
to
7f049e5
Compare
Looks like we have some build errors. RTEMS 4.11 https://github.com/nasa/cFS/actions/workflows/build-cfs-rtems4.11.yml
|
RTEMS 5 https://github.com/nasa/cFS/actions/runs/1957722983
|
In 4.11 the struct typedef name seems to be If that doesn't work, could just make this implementation only work on 5+ (make 4.11 revert to previous behavior without this feature). Last resort could be of course to drop support for 4.11, but I still like RTEMS 4.11 + pc686 as a litmus test because it defines |
I made a naive "fix" thanks to @acudmore suggestion. See astrogeco@cab8d98 New Error https://github.com/astrogeco/cFS/runs/5482537334?check_suite_focus=true
|
Looks like my second naive fix got us through the build on both 4.11 and 5 I got all greens! |
- Add #define to handle renames from RTEMS 4.11 to RTEMS 5 - Cast to cpuaddress before assignment to `OS_module_address_t` elements Try to fix RTEMS osal type casting for GetModuleInfo
- Use typedef instead of #define
- Add aliased typedef to handle renames from RTEMS 4.11 to RTEMS 5 - Cast to cpuaddress before assignment to `OS_module_address_t` elements
- Add aliased typedef to handle renames from RTEMS 4.11 to RTEMS 5 - Cast to cpuaddress before assignment to `OS_module_address_t` elements
- Add aliased typedef to handle renames from RTEMS 4.11 to RTEMS 5 - Cast to cpuaddress before assignment to `OS_module_address_t` elements
Fix nasa#1221, Use quotes for local includes
Describe the contribution
A clear and concise description of what the contribution is.
Testing performed
The ES "query application" command yields a telemetry packet that contains section information supplied by OS_ModuleGetInfo_Impl. Compared object section sizes (.text, .data, .bss) in the telemetry given by the query app command to section sizes reported by the "size" tool in the platform toolchain. Checked the disassembly of application objects built in FSW to find symbols in each section (.text, .data, .bss) and used the MM application to report the addresses for each. Verified that symbol addresses reported were contained in the interval [addr, addr + size) for each section.
Tested with cFS bundle:
https://github.com/ezpollack/cFS/actions/workflows/build-cfs-rtems4.11.yml
https://github.com/ezpollack/cFS/actions/workflows/build-cfs-rtems5.yml
Expected behavior changes
System(s) tested on
Additional context
This will only work for RTEMS 4.11+. Version 4.11 was the first version to support dynamic loading of objects via the RTEMS Runtime Loader (RTL).
Contributor Info - All information REQUIRED for consideration of pull request
Eric Pollack - NASA/GSFC