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

[ros2-master] Fix build for last nav2 main branch #274

Merged

Conversation

doisyg
Copy link

@doisyg doisyg commented Feb 9, 2021

Fix build for last nav2 main branch (rolling ?), basically an update of #233
Mimicking the style of https://github.com/ros-planning/navigation2/tree/main/nav2_dwb_controller/dwb_core.
setSpeedLimit still need to be properly implemented but was added for build to complete.
Do we still need to pass nav2_util::LifecycleNode::SharedPtr node to the initialize(nav2_util::LifecycleNode::SharedPtr node) function since we have rclcpp_lifecycle::LifecycleNode::WeakPtr nh_ as a member variable which is initialized by TebLocalPlannerROS::configure? @SteveMacenski, you added the argument in #233, what do you think ?

More generally, I see the ros2-master branch of this repo as the ros2 dev branch which should try to stay compatible with the nav2 main branch, am I right @amakarow ? Also it seems that this branch is late on many melodic-devel PRs, any plans to port them ?

@SteveMacenski
Copy link
Collaborator

@doisyg this is on my radar right now preping for the galactic release, we should work on getting this merged in / let me take a look now that there's been a few more updates what else may be required

@doisyg
Copy link
Author

doisyg commented Mar 5, 2021

Great! @SteveMacenski are you a maintener of TEB too ?
I believe the ros2-master (and foxy-devel) is behind the ROS1 branches melodic and noetic for several important PRs.
It would be nice to target to include them for the galactic release. I am willing to commit some resources from my team for that.
First step would be to list the difference and then to decide on a methodology

@SteveMacenski
Copy link
Collaborator

SteveMacenski commented Mar 5, 2021

I'm not, this is on my list to circle back to, but getting STVL and NPVL up to date with master is coming first before I will come back to this.

Updates on those important noetic/melodic updates would be good. I'm not sure what the best course of action is there. I'm thinking you should submit those against foxy/ros2 first, then the API updates for ros2-master at the end so that you can cherry pick commits without concern of conflicts between them. I'm not sure "how many" we're talking about here. If its not that much code change, you could cherry pick them into this 1 PR. If its too many, probably another PR would be good to merge in those changes that were already approved into foxy/ros2, then these updates.

@SteveMacenski
Copy link
Collaborator

Hi, I chatted with @croesmann and he’s given me write permissions to support the ROS2 branch! I’ll follow up here next week

@SteveMacenski
Copy link
Collaborator

@doisyg if this builds against Nav2 master, we can merge this

@doisyg
Copy link
Author

doisyg commented Mar 9, 2021

It does!

@SteveMacenski SteveMacenski merged commit 2ecd15e into rst-tu-dortmund:ros2-master Mar 9, 2021
Comment on lines +1018 to +1020
cfg_->robot.base_max_vel_x_backwards = cfg_->robot.base_max_vel_x_backwards;
cfg_->robot.base_max_vel_y = cfg_->robot.base_max_vel_y;
cfg_->robot.base_max_vel_theta = cfg_->robot.base_max_vel_theta;
Copy link

Choose a reason for hiding this comment

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

Isn't this supposed to be:

cfg_->robot.max_vel_x_backwards = cfg_->robot.base_max_vel_x_backwards * speed_limit / 100.0;
cfg_->robot.max_vel_y = cfg_->robot.base_max_vel_y * speed_limit / 100.0;
cfg_->robot.max_vel_theta = cfg_->robot.base_max_vel_theta * speed_limit / 100.0;

Copy link
Author

Choose a reason for hiding this comment

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

These lines are inside :
if (speed_limit == nav2_costmap_2d::NO_SPEED_LIMIT) {
so I believe this is the intended behavior

Copy link

Choose a reason for hiding this comment

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

Sorry I copied the wrong snippet, I meant that we should be setting max_vel_* and not base_max_vel_*

See our attempted fix https://github.com/logivations/teb_local_planner/pull/33/files

Choose a reason for hiding this comment

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

Created PR #372

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch

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.

None yet

3 participants