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

Arc 78 trajectory follower integration #26

Merged
merged 20 commits into from
Aug 28, 2024

Conversation

saina-ramyar
Copy link
Contributor

PR Details

Description

This PR is part of the integration testing of the Trajectory Follower Wrapper on a physical vehicle.
Related PR:
usdot-fhwa-stol/carma-platform#2419

Related GitHub Issue

Related Jira Key

https://usdot-carma.atlassian.net/browse/ARC-78

Motivation and Context

Integrate the Trajectory Follower controller of Autoware, that has a MPC based lateral controller, to improve vehicle's performance.

How Has This Been Tested?

Integration tested on Blue Lexus

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.

Copy link
Contributor

@MishkaMN MishkaMN left a comment

Choose a reason for hiding this comment

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

Looks like 2 nodes added were from autoware, so I really didn't look at them.
Please let me know if there are specific lines that you added that need review.
It looks like we may be missing last commit because debug prints are still here, so I can look at it after they are removed.

@@ -329,17 +329,21 @@ int64_t calcNearestIndex(
if (traj.points.empty()) {
return -1;
}
const float64_t my_yaw = tf2::getYaw(self_pose.orientation);
const float64_t my_yaw = 0.0; //tf2::getYaw(self_pose.orientation);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? or did we forget to push commit.
If it is actually correct, let's explain why it needs to be this way.

int64_t nearest_idx = -1;
float64_t min_dist_squared = std::numeric_limits<float64_t>::max();
for (size_t i = 0; i < traj.points.size(); ++i) {
const float64_t dx = self_pose.position.x - static_cast<float64_t>(traj.points.at(i).x);
const float64_t dy = self_pose.position.y - static_cast<float64_t>(traj.points.at(i).y);
const float64_t dist_squared = dx * dx + dy * dy;
std::cerr<< "dist_squared: "<<dist_squared<<std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

after the integration is done, it is probably better to remove these cerr prints, because I imagine they will be printing at 30hz which is too much.

@@ -163,6 +163,12 @@ LateralController::LateralController(const rclcpp::NodeOptions & node_options)
// TODO(Frederik.Beaujean) ctor is too long, should factor out parameter declarations
declareMPCparameters();

rcutils_ret_t ret = rcutils_logging_set_logger_level("trajectory_follower", RCUTILS_LOG_SEVERITY_DEBUG);
Copy link
Contributor

Choose a reason for hiding this comment

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

debug prints should be removed

@saina-ramyar
Copy link
Contributor Author

Looks like 2 nodes added were from autoware, so I really didn't look at them. Please let me know if there are specific lines that you added that need review. It looks like we may be missing last commit because debug prints are still here, so I can look at it after they are removed.

I only added one node (longitudinal_controller), there rest already existed. Also haven't made any logic changes, just adding some debug logs which were left and removed now. You can review again.

Comment on lines 166 to 170
rcutils_ret_t ret = rcutils_logging_set_logger_level("trajectory_follower", RCUTILS_LOG_SEVERITY_DEBUG);
if (ret != RCUTILS_RET_OK) {
RCLCPP_ERROR(get_logger(), "Could not set log level for traj follower lib");

}
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove this debugging because it is forcing "trajectory_follower" logger to be DEBUG.

Copy link
Contributor

@MishkaMN MishkaMN left a comment

Choose a reason for hiding this comment

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

Looks good to me. CI is failing due to not enough memory available.
soedinglab/hh-suite#280
We can probably merge this.

@saina-ramyar saina-ramyar merged commit ec706ff into develop Aug 28, 2024
1 of 2 checks passed
@MishkaMN MishkaMN deleted the arc-78-trajectory-follower-integration branch August 29, 2024 01:15
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