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

Reimplement impl with fpp-to-cpp #171

Merged
merged 17 commits into from
Oct 26, 2023
Merged

Reimplement impl with fpp-to-cpp #171

merged 17 commits into from
Oct 26, 2023

Conversation

thomas-bc
Copy link
Contributor

@thomas-bc thomas-bc commented Oct 11, 2023

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Implements nasa/fprime#2204

fprime-util impl [--ut] now uses fpp-to-cpp instead of the Python autocoders (through cmake)
Also uses the same interface to run the implementation after fprime-util new --component

Also some small changes to help text here and there.

src/fprime/fpp/impl.py Fixed Show fixed Hide fixed
src/fprime/fpp/impl.py Fixed Show fixed Hide fixed
Copy link
Contributor Author

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Review comments

src/fprime/fpp/common.py Outdated Show resolved Hide resolved
src/fprime/fpp/impl.py Outdated Show resolved Hide resolved
src/fprime/util/cookiecutter_wrapper.py Outdated Show resolved Hide resolved
@LeStarch
Copy link
Collaborator

There are two issues with this PR:

  1. Running fprime-util impl --ut does not produce .template files, and thus will clobber the existing UT files
  2. The imports aren't working for the SignalGen UT
  3. The import for Ref/SignalGen/SignalGen.hpp comes out as SignalGen/SignalGen.hpp
  4. The import in SignalGenTestMain.cpp isn't using a relative import, and thus cannot find SignalGenTester.hpp

These both might be issues in FPP (@bocchino, thoughts?)

@thomas-bc
Copy link
Contributor Author

(1) fprime-util impl --ut does not mess with filenames. Currently it seems FPP generates <filename>.template for fpp-to-cpp, but regular <filename> for fpp-to-cpp --templates. I can implement the file renaming, but we may want to implement that in FPP instead?
(2) and (3) are an oversight on my end - I forgot to test with the old project structure (e.g. Ref/)
(4) Not too sure there, may be related to (2) and (3), I'll investigate

@LeStarch
Copy link
Collaborator

@thomas-bc:
(1) We should implement it in fpp
(4) This is likely an issue in FPP as I fixed a similar issue in fpp's Tester.cpp

@bocchino
Copy link
Collaborator

bocchino commented Oct 19, 2023

Running fprime-util impl --ut does not produce .template files

We can change this, but the way it works in FPP now is consistent with the way it works in XML and has always worked.

and thus will clobber the existing UT files

I don't think so, because the existing UT files are typically in test/ut and the generated files are in the component directory. This way the user can just copy the files over, without renaming files. It is inconsistent with the .template naming for the flight implementation, but it seems to work well based on experience so far.

@bocchino
Copy link
Collaborator

This is likely an issue in FPP as I fixed a similar issue in fpp's Tester.cpp

I agree, I'll open an issue to fix it in FPP.

@bocchino
Copy link
Collaborator

Is now a good time to make a v2.0.1 release of FPP to get these changes into F Prime mainline?

@LeStarch LeStarch self-requested a review October 25, 2023 16:41
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.

Just two comments!

src/fprime/fpp/common.py Outdated Show resolved Hide resolved
src/fprime/fpp/impl.py Outdated Show resolved Hide resolved
build.build_dir / "F-Prime",
build.build_dir,
]
if build.is_submodule_build_structure():
Copy link
Collaborator

@LeStarch LeStarch Oct 25, 2023

Choose a reason for hiding this comment

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

Why? Also, this should not use "cmake root", but build.get_settings("project_path") should it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes absolutely, my mistake. By using project_root, we don't need to check whether the build structure is old or new, as project_root and framework_path will simply be the same. Note, both of these values are guaranteed to exist as per the following:
https://github.com/fprime-community/fprime-tools/blob/4fc88dc0a20fcadd5b09bb5a188bc41373237d5d/src/fprime/fbuild/settings.py#L52-L53

since find_fprime will error out if it can't find framework_location

src/fprime/fpp/common.py Outdated Show resolved Hide resolved
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.

Left two more comments.

@LeStarch LeStarch merged commit b68d6c3 into nasa:devel Oct 26, 2023
28 checks passed
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.

3 participants