Update Ref and RPI for compatibility with FPP #349 (fully qualified component instance names) #2823
Conversation
This file contains 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
|
Will probably need to wait for both the FPP and GDS releases for that one |
thomas-bc
added
the
Update Instructions Needed
Sync up with `devel`
Sync `fpp/349` with `devel`
thomas-bc
reviewed
Sep 3, 2024
| namespace fileUplink { enum { WARN = 3, FATAL = 5 }; } | ||
| namespace RPI_rateGroup10HzComp { enum { WARN = 3, FATAL = 5 }; } | ||
| namespace RPI_rateGroup1HzComp { enum { WARN = 3, FATAL = 5 }; } | ||
| namespace RPI_cmdDisp { enum { WARN = 3, FATAL = 5 }; } |
There was a problem hiding this comment.
I know this was likely discussed already but I want to capture in writing and refresh my mind:
Why are we namespacing with _ here? Could FPP not handle :: ?
And do we need to namespace at all since they are used-defined?
These patterns should probably be documented somewhere if they are required
There was a problem hiding this comment.
We need one C++ name for each qualified FPP name. The easiest way seems to be to use the underscores as shown. We could use :: but this seems awkward. You would have to write each of the names inside a namespace.
There was a problem hiding this comment.
This should be documented in the FPP User's Guide.
There was a problem hiding this comment.
I can add that to the docs!
There was a problem hiding this comment.
Sounds good! I opened an issue on it. See nasa/fpp#510.
There was a problem hiding this comment.
It is imo a little confusing from the user perspective that some stuff (e.g. components) would be namespaced RPI::rateGroup10HzComp but its configuration has to be RPI_rateGroup10HzComp
From a user perspective it is not clear what makes the distinction, and a little confusing. But maybe with the User Guide it will make more sense.
There was a problem hiding this comment.
It is imo a little confusing from the user perspective that some stuff (e.g. components) would be namespaced RPI::rateGroup10HzComp but its configuration has to be RPI_rateGroup10HzComp
The rationale is that RPI::rateGroup10HzComp is a fully qualified name in C++, declared at the top level, whereas RPI_rateGroup10HzComp is a name declared inside the topology namespace. Note the following:
- I agree the use of underscores is not ideal, but I don't know how to do it better. I don't think we should nest the instance namespaces inside the topology namespaces.
- We could be consistent and use underscores everywhere, but we decided to move the instance names out of the topology namespace and use namespaces for them.
We will explain this in the FPP User's Guide.
LeStarch
approved these changes
Sep 4, 2024
LeStarch
pushed a commit
to fprime-community/fprime-workshop-led-blinker
that referenced
this pull request
Sep 4, 2024
kevin-f-ortega
pushed a commit
to fprime-community/fprime-workshop-led-blinker
that referenced
this pull request
Oct 10, 2024
LeStarch
added a commit
to fprime-community/fprime-workshop-led-blinker
that referenced
this pull request
Oct 16, 2024
* Update fpp model (#56) * Fixing changes to OSAL Tasks (#65) Co-authored-by: Michael D Starch <Michael.D.Starch@jpl.nasa.gov> * Update hpp copy paste code to include the override keyword to match the generated code (#68) * Update instructions to reference install guide (#57) * Update pre-requisites (#59) * Update project name (#60) * Add override to parameterUpdated (#63) * Fix typos in requirements.md (#64) * Update fprime git submodule to use https (#66) * update hpp copy paste code to include the override keyword to match the generated code --------- Co-authored-by: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com> Co-authored-by: Nate Gay <email@n8.gay> Co-authored-by: Roberto Valenzuela <valenzuelarober@gmail.com> Co-authored-by: Joshua Anderson <joshua.l.anderson@jpl.nasa.gov> Co-authored-by: crsmith <celeste.r.smith@jpl.nasa.gov> * Fixes for nasa/fprime#2831 (#71) * Fixes for nasa/fprime#2831 * Missing logger import * Fixes for nasa/fprime#2823 (#73) * update for compatibility with fpp #349 * update integration tests --------- Co-authored-by: jawest <justine.a.west@jpl.nasa.gov> * Update LedBlinkerPackets.xml (#74) Remove systemResources TLM channels from deployment so pr-2866 can run successfully Co-authored-by: Shivaly-Reddy <164100839+Shivaly-Reddy@users.noreply.github.com> * Phase 1 State Machine changes (nasa/fprime#2829) (#75) * Reworking tutorial to match improved workflow reordering the sections so they better match best software engineering practices updated parameterUpdated to use a switch statement formatting enum argument checking happens under the hood; so, removing the check from command handler. Also removing the EVR from fpp marking which portion of the documentation needs to be updated to remove non-existing evr using async instead of sync. No real good reason to use sync here, and async makes the code implementation issuer (avoid using mutex) removing mutex and its use since we switch run handler to async port updating documentation to remove mutex/lock using member variable convention updated member variables to use correct name. also fixed formatting updated documentation to use new member variables updated documentation to use new member variables updated documentation to use new member variables Adding formatting file from fprime added missing step Added clarifying note updated day of component implementation typo using async port. improving parameterUpdated function Added sequence diagram explaining blink command removed unneeded tlm xml section. cleaned up code to use switch statement adding missing dodispatch now that the run port in an async port Having this parameter is critical for the unit test portion. Add it here and not leave it up to the students to add Updating uts so they don't depend on 'try it yourself' activity updated UT doc fixed typos fixed markdown formatting making comments and signatures consistent with latest fprime-util formatting adding default value to parameter adding default value to parameter typo Readding param get try-it-yourself task. It's a good exercise for the students to have Readding since it's better for the reader to exercise these aspects simplified run handler Updated documentation so that all design for day 1 is together and all initial implementation for day 2 is together. This better follows the design and implementation workflow that we use when developing software minor format updates Updating words per review Updated documentation so that all design for day 2 is together and all continued implementation for day 2 is together. This better follows the design and implementation workflow that we use when developing software Using event instead of EVR since the word event is more consitent throughout the tutorial Formatting Formatting Fixing title giving member variable a better name and updating code in tutorial to match latest code Updated documents per review Adding spoiler tags around answers fixing camelcase and comments fixed code formatting to match fprime's formatting fixing formatting Updating per review. fixing. want to use camel case fixed formatting of UTs * fixing formatting of code * updated fprime pointer to point to latest in devel * updated to work with packet tlm xml commented out. Also loading parameters as we should be * Updated pointer to use latest fprime tag 3.5.0 * Escaping key word * typo * fixing gpio configuration to match with fprime 3.5.0 * fixed code for config of gpio * updated led blinker for fprime 3.5.0 * Adding answer so tut flows better --------- Co-authored-by: Rob Bocchino <bocchino@icloud.com> Co-authored-by: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com> Co-authored-by: Michael D Starch <Michael.D.Starch@jpl.nasa.gov> Co-authored-by: csmith608 <63731123+csmith608@users.noreply.github.com> Co-authored-by: Nate Gay <email@n8.gay> Co-authored-by: Roberto Valenzuela <valenzuelarober@gmail.com> Co-authored-by: Joshua Anderson <joshua.l.anderson@jpl.nasa.gov> Co-authored-by: crsmith <celeste.r.smith@jpl.nasa.gov> Co-authored-by: M Starch <LeStarch@googlemail.com> Co-authored-by: Justine West <35715959+jwest115@users.noreply.github.com> Co-authored-by: jawest <justine.a.west@jpl.nasa.gov> Co-authored-by: Shivaly-Reddy <164100839+Shivaly-Reddy@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change Description
As part of nasa/fpp#349, component instance names are now fully qualified in topology c++ auto generated files. Updated Ref and RPI deployment fpp files and integration tests to match this change.
Rationale
Needed for compatibility with nasa/fpp#349
Testing/Review Recommendations
Built both deployments successfully. Ran integration tests for Ref deployment successfully.