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

Add initial ROS2 humble packages #2474

Merged
merged 45 commits into from
Dec 2, 2024
Merged

Conversation

MishkaMN
Copy link
Contributor

@MishkaMN MishkaMN commented Nov 25, 2024

PR Details

Description

Add initial ROS2 humble packages

NOTE: CI is modified for now to read from a list and build. In the future, only packages exist in this repository will be built.
These are packages already building fine in this PR: #2472

NOTE: There seem a lot of changes, but they are all basically following changes:

  • needed to include tf2_buffer in gnss_to_map_converter instead of using #include <tf2/LinearMath/Vector3.h>
  • declare_parameter needed default values
  • changing from header .h to .hpp
  • using rclcpp::Duration::from_nanoseconds(<int_nanoseconds>) function as the rclcpp::Duration(<int_nanoseconds>) constructor no longer works
  • tf2_listener and tf2_buffer needed to be made into shared_ptr

Related GitHub Issue

NA

Related Jira Key

ARC-233

Motivation and Context

ROS2 Humble Upgrade

How Has This Been Tested?

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@MishkaMN MishkaMN self-assigned this Nov 25, 2024
@MishkaMN MishkaMN added the enhancement New feature or request label Nov 25, 2024
@MishkaMN
Copy link
Contributor Author

Currently CI is failing only because of lint checks in the unit tests.

Copy link
Contributor

@john-chrosniak john-chrosniak left a comment

Choose a reason for hiding this comment

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

Confirmed everything builds in ROS2 Humble

@@ -63,64 +63,36 @@ jobs:
source "$INIT_ENV"
./src/${{ github.event.repository.name }}/docker/checkout.bash -r /opt/carma/ -b ${{ steps.determine-base-branch.outputs.git_branch }}

- name: Install external dependencies
# Install the multiple object tracking deps
run: sudo bash /opt/carma/src/multiple_object_tracking/scripts/install_dependencies.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we no longer need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should keep this, I just had it turned off, but let me turn them on.

@@ -46,7 +46,6 @@ sonar.modules= bsm_generator, \
carma_wm, \
carma_wm_ctrl, \
roadway_objects, \
platooning_strategic_IHP, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be replaced with platooning_strategic_ihp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

goto catch, added

package='platoon_strategic_ihp',
plugin='platoon_strategic_ihp::Node',
name='platoon_strategic_ihp_node',
package='platooning_strategic_ihp',
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes should also be made in the package.xml and CMakeLists.txt of platooning_strategic_ihp

Copy link
Contributor Author

@MishkaMN MishkaMN Nov 27, 2024

Choose a reason for hiding this comment

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

Anish is handling this in his PR, so we can probably leave this for now. I'll just return these to original then

# ROS2 installation
###
# Source the ROS2 autoware installation
source /home/carma/catkin/setup.bash
if [[ ! -z "$ROS1_PACKAGES$ROS2_PACKAGES" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably remove the ROS1_PACKAGES variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -51,6 +51,6 @@
- /guidance/plugins/stop_and_wait_plugin
- /guidance/plugins/sci_strategic_plugin
- /guidance/plugins/stop_controlled_intersection_tactical_plugin
- /guidance/plugins/platoon_strategic_ihp
- /guidance/plugins/platooning_strategic_ihp
Copy link
Contributor

@john-chrosniak john-chrosniak Nov 27, 2024

Choose a reason for hiding this comment

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

Comment not specific for here but there are still references to platoon_strategic_ihp elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually just reverted the chagnes for this plugin and let Anish handle it fully in his PR

@MishkaMN
Copy link
Contributor Author

MishkaMN commented Dec 1, 2024

Ignoring SonarScan failure about code coverage because there is a separate stories as it is not part of the scope to add more unit tests for Humble upgrade.

ASSERT_FALSE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::UNAVAILABLE), rclcpp::Time(1e9 * 1)));
ASSERT_FALSE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::DARK), rclcpp::Time(1e9 * 1)));
ASSERT_FALSE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::STOP_THEN_PROCEED), rclcpp::Time(1e9 * 1)));
ASSERT_FALSE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::PRE_MOVEMENT), rclcpp::Time(1e9 * 1)));
ASSERT_FALSE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::PERMISSIVE_MOVEMENT_ALLOWED), rclcpp::Time(1e9 * 1)));
ASSERT_TRUE(lcip->validLightState(std::pair<boost::posix_time::ptime, lanelet::CarmaTrafficSignalState>(dummy_time, lanelet::CarmaTrafficSignalState::PERMISSIVE_MOVEMENT_ALLOWED), rclcpp::Time(1e9 * 1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this changed from FALSE to TRUE

Copy link
Contributor Author

@MishkaMN MishkaMN Dec 2, 2024

Choose a reason for hiding this comment

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

yeah because we recently changed the logic where we allowed PERMISSIVE_MOVEMENTS in addition to PROTECTED.
This is the PR #2373. PR itself was opened long time ago during distributed testing in summer, but was never merged, so we caught up to it only now. But it looked like the unit test was not updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation!

std::vector<std::string> lightbar_priorities = {}; // Tsesters are for unit testing. Keep it there.
std::vector<std::string> lightbar_cda_table = {}; // Keys for lightbar_cda_to_ind_table, 1-to-1 with lightbar_ind_table
std::vector<std::string> lightbar_ind_table = {}; // Values for lightbar_cda_to_ind_table, 1-to-1 with lightbar_cda_table
std::vector<std::string> lightbar_priorities = {"lightbar_manager"}; // Tsesters are for unit testing. Keep it there.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could we correct the typo in the comment "Tsesters"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the comment anyways because it was wrong.

Copy link

sonarcloud bot commented Dec 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
68.63% Line Coverage on New Code (required ≥ 80%)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@MishkaMN MishkaMN merged commit a7111c1 into develop-humble Dec 2, 2024
3 of 5 checks passed
@MishkaMN MishkaMN deleted the arc-233-humble-packages branch December 2, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants