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

Bugfix in integration tests and fixed ROS2 tests #1841

Merged
merged 8 commits into from Jun 13, 2023
Merged

Bugfix in integration tests and fixed ROS2 tests #1841

merged 8 commits into from Jun 13, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Jun 11, 2023

This PR fixes a bug in our integration tests that prevented the C++ ROS2 tests from actually being compiled with the ROS2 flag. Effectively, we did not run any of the ROS2 tests for a while. Since a few small bugs crept into the ROS2 support in the meantime, this PR also fixes the problems in the ROS2 code generation and pulls in lf-lang/reactor-cpp#50.

@cmnrd cmnrd requested a review from lhstrh June 11, 2023 10:43
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 11, 2023

@erlingrj It looks like the fix in the test suite also exposed a bug in the zephyr support (see the test log). Could you take a look at this?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 11, 2023

Ok, I think I found the problem.

@erlingrj
Copy link
Collaborator

Will take a look tomorrow!

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix!

@erlingrj
Copy link
Collaborator

Why should the targetConfig be reloaded after applying the Configurators, which is what we use to change the targetProperty? I believe the Zephyr tests now fail because the Configurator step adds the platform target property, but now this is undone by reloading the targetConfig once more after.

@erlingrj
Copy link
Collaborator

Do we need a new type of Configurator which can modify the target properties and runs last?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 12, 2023

Oh, that is a good point. Thanks! I guess it was not well-defined how the configurator is supposed to be used. The configurator for the ROS2 tests was modifying the resource, so that the target config needs to be reloaded after the transformation. I will change the ROS2 generator to also directly modify the target properties.

@lhstrh
Copy link
Member

lhstrh commented Jun 12, 2023

Oh, that is a good point. Thanks! I guess it was not well-defined how the configurator is supposed to be used. The configurator for the ROS2 tests was modifying the resource, so that the target config needs to be reloaded after the transformation. I will change the ROS2 generator to also directly modify the target properties.

I had assumed that the added reload happened in between runs, but if it happens after the target property changes and before the execution, then that's no bueno.

@cmnrd cmnrd enabled auto-merge June 12, 2023 15:21
@cmnrd cmnrd added this pull request to the merge queue Jun 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 12, 2023
@lhstrh lhstrh enabled auto-merge June 13, 2023 04:56
@lhstrh lhstrh added this pull request to the merge queue Jun 13, 2023
@cmnrd cmnrd removed this pull request from the merge queue due to a manual request Jun 13, 2023
@cmnrd cmnrd enabled auto-merge June 13, 2023 08:45
@cmnrd cmnrd added this pull request to the merge queue Jun 13, 2023
@cmnrd cmnrd removed this pull request from the merge queue due to a manual request Jun 13, 2023
@cmnrd cmnrd added this pull request to the merge queue Jun 13, 2023
@cmnrd cmnrd removed this pull request from the merge queue due to a manual request Jun 13, 2023
@cmnrd cmnrd enabled auto-merge June 13, 2023 15:42
@cmnrd cmnrd added this pull request to the merge queue Jun 13, 2023
Merged via the queue into master with commit 299a3cd Jun 13, 2023
42 checks passed
@cmnrd cmnrd deleted the cpp-ros2-fix branch June 13, 2023 19:23
@lhstrh lhstrh added the bugfix label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants