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

Remove FPP dependencies on native int types #2548

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

bocchino
Copy link
Collaborator

@bocchino bocchino commented Feb 28, 2024

Change Description

This PR makes changes necessary to remove the dependencies of the FPP model and generated C++ on the native int types NATIVE_INT_TYPE and NATIVE_UINT_TYPE.

Incidental changes:

  • I reformatted a few files via clang-format. We want to do this reformatting anyway on a larger scale. Reformatting is not the primary purpose of this PR, but the reformatting helped me understand the code. Since reformatting obscures change/delete info, I reformatted only hpp files, so you can still see the change/delete info in the corresponding cpp files.

  • I added some comments in FpConfig.h explaining what the configurations do.

  • I renamed FwBuffSizeType to FwSizeStoreType as discussed. I added a type alias for FwBuffSizeType for backwards compatibility.

Rationale

This is a first step in removing NATIVE_INT_TYPE and NATIVE_UINT_TYPE from the code base.

Next Steps

  1. Make an alpha release of FPP that eliminates references to NATIVE_INT_TYPE and NATIVE_UINT_TYPE. At this time we can replace NATIVE_INT_TYPE with FwIndexType in the portNum arguments of the port handler implementations.
  2. Scrub NATIVE_INT_TYPE and NATIVE_UINT_TYPE from the code in this repo.
  3. In FpConfig.h, replace configurable types except for PlatformInt/Uint with fixed-width types, to eliminate static analysis warnings.

Related Issues

nasa/fpp#391

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@bocchino
Copy link
Collaborator Author

Note: There are many false-positive warnings due to (1) problems in F Prime that this PR is addressing or (2) problems with the static analysis. Dismissing these warnings one-by-one in the GUI is too painful. I gave up. (You have to click dismiss, wait a few seconds until it resets to the top of the changed file list, and scroll back manually to where you were.)

Os/TaskCommon.cpp Dismissed Show dismissed Hide dismissed
Os/Task.hpp Dismissed Show dismissed Hide dismissed
Svc/ComQueue/ComQueue.cpp Dismissed Show dismissed Hide dismissed
Svc/Health/HealthComponentImpl.cpp Dismissed Show dismissed Hide dismissed
Svc/TlmChan/TlmChan.cpp Dismissed Show dismissed Hide dismissed
Svc/TlmPacketizer/TlmPacketizer.cpp Dismissed Show dismissed Hide dismissed
Svc/SystemResources/SystemResources.cpp Dismissed Show dismissed Hide dismissed
Svc/ComQueue/ComQueue.cpp Dismissed Show dismissed Hide dismissed
Svc/Health/HealthComponentImpl.cpp Dismissed Show dismissed Hide dismissed
Svc/TlmChan/TlmChan.cpp Dismissed Show dismissed Hide dismissed
Svc/TlmPacketizer/TlmPacketizer.cpp Dismissed Show dismissed Hide dismissed
@bocchino
Copy link
Collaborator Author

There are many false-positive warnings due to (1) problems in F Prime that this PR is addressing or (2) problems with the static analysis.

It turns out that the static analysis does not work as expected with regard to fixed-width types. It seems that when you declare a variable

Foo bar;

It wants Foo to be a literal fixed-width numeric type like int32_t or maybe I32. It won't accept a general type alias, even if the aliased type has a fixed width. This seems like a bug in the static analysis.

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.

  1. Review need for pointer cast in model. If needed, switch to PlatformPointerCase
  2. Review type event counter derived types

@LeStarch
Copy link
Collaborator

LeStarch commented Mar 1, 2024

Make them all enum store type, but keep queue size as it is.

@bocchino
Copy link
Collaborator Author

bocchino commented Mar 4, 2024

The event counters are for counting events emitted at runtime, for purposes of throttling. So I think they should be FwIndexType.

Add size type alias to Serializable
Remove type aliases for generated code
@bocchino
Copy link
Collaborator Author

bocchino commented Mar 4, 2024

With the change of FwSizeType to I64, there are a lot of implicit casts from I64 to NATIVE_UINT_TYPE and/or U32. I for this reason, I had to disable -Wconversion in the FPP compilation checks (too many warnings). This issue will be fixed in due course as we replace NATIVE_UINT_TYPE and U32 with FwSizeType, e.g., in the Fw::Buffer interface.

Fw/Types/Serializable.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
@bocchino
Copy link
Collaborator Author

bocchino commented Mar 4, 2024

@LeStarch I made the requested changes. I also had to introduce a type alias into Fw::Serializable, to resolve the discrepancy between the declared types NATIVE_UINT_TYPE and the eventual types FwSizeType. We'll have FPP refer to the alias and change the alias later. That way we decouple the FPP changes from the F Prime changes.

@bocchino bocchino requested a review from LeStarch March 4, 2024 18:37
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.

I see the expected changes fixed!

@LeStarch LeStarch merged commit c02f351 into nasa:devel Mar 5, 2024
34 checks passed
@bocchino bocchino deleted the remove-fpp-deps-on-native-int-types branch March 5, 2024 18:40
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.

2 participants