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

Revise file name string sizes in AssertFatalAdapter events #2796

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

bocchino
Copy link
Collaborator

@bocchino bocchino commented Jul 9, 2024

Change Description

This PR updates the sizes of the file name strings used in the AssertFatalAdapter events:

  1. Make the event string size configurable, instead of hard-coded to 80.
  2. In the default configuration, set each of the three kinds of truncation that an event string passes through to 200. (Note: 256 is too large for the AMPCS compatibility code which we don't use, but is still in the generated C++.)
  3. Add comments to the config files identifying the three kinds of truncation applied to file names in assertion failure events.
  4. Fix issues in the framework uncovered by these changes.
  5. Incidentally fix an off-by-one error in the CmdSplitter unit tests.

Rationale

On workstation development systems, e.g., Linux and Mac, an 80-character string is not long enough to report the location where an assertion has failed. For example, if an assertion failure occurs in an autocoded C++ file, the file name can have the following form:

/several/file/path/to/fprime/project/build-fprime-automatic-native/F-Prime/Fw/Buffer/BufferGetPortAc.cpp

That's 104 characters; with an 80-character string, the GDS displays the unimportant part (the path prefix that is common to all FSW files) and chops off the part you care about, which is at the end:

% printf /several/file/path/to/fprime/project/build-fprime-automatic-native/F-Prime/Fw/Buffer/BufferGetPortAc.cpp | head -c 80
/several/file/path/to/fprime/project/build-fprime-automatic-native/F-Prime/Fw/Bu

This behavior makes assertion failure event reports essentially unusable on these systems.

The assertion file strings that users get out of the box should satisfy two criteria:

  1. They should be configurable, so that users can use different sizes on different systems.
  2. In the basic configuration, which should run on Linux and Mac, the strings should be long enough to display the information needed to debug the FSW. On smaller systems with less preamble in the file names (or on systems that use numerical hashes instead of file name strings), the configured size can be made smaller.
  3. It should be straightforward to change the configuration to enable longer or shorter assertion failure strings.

The framework unit tests should also compile and run correctly under different configurations. I fixed the issues that I saw when making the configuration changes here. I'm sure there are more issues, which we won't see until we have a general way to test other configurations. This is a CI issue.

Copy link
Contributor

@Joshua-Anderson Joshua-Anderson left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this Rob, this looks great!

I added one comment about the sizing of the combuffer you may want to consider. I suspect we'll also want @LeStarch's signoff on this.

config/FpConfig.h Outdated Show resolved Hide resolved
Also fix bug in CmdSplitterTester
For consitency with AMPCS compatibility code
AMPCS arguments have max size 255
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Looks good, ready to merge?

Svc/CmdSplitter/test/ut/CmdSplitterTester.cpp Show resolved Hide resolved
@bocchino
Copy link
Collaborator Author

Yes, ready!

@LeStarch LeStarch merged commit 129d7f5 into nasa:devel Jul 24, 2024
34 checks passed
@bocchino bocchino deleted the fix/fatal-adapter-file-names branch September 16, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants