Skip to content

Use different param name for slam_toolbox param_file#2243

Closed
v-lopez wants to merge 2 commits intoros-navigation:foxy-develfrom
pal-robotics-forks:foxy-devel
Closed

Use different param name for slam_toolbox param_file#2243
v-lopez wants to merge 2 commits intoros-navigation:foxy-develfrom
pal-robotics-forks:foxy-devel

Conversation

@v-lopez
Copy link
Copy Markdown
Contributor

@v-lopez v-lopez commented Mar 13, 2021

Basic Info

Info Please fill out this column
Ticket(s) this addresses #2240
Primary OS tested on Ubuntu 20.04
Robotic platform tested on simulated turtlebot

Description of contribution in a few bullet points

  • Use renamed slam_toolbox param-file

v-lopez and others added 2 commits March 12, 2021 22:54
@v-lopez
If provided param file has no slam_toolbox params, don't forward it (r…
a66ab43 
…os-navigation#2214)

* If provided param file has no slam_toolbox params, don't forward it

* Update copyright

* Linter fixes

* Reorder slam_launch to hide uglyness at the end
Use different param name for slam_toolbox param_file
8be0637
@v-lopez v-lopez changed the title Foxy devel Use different param name for slam_toolbox param_file Mar 13, 2021
@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 13, 2021

I'm not sure if we should have merged in the foxy one on slam toolbox, that changes the API for params file for anyone with scripts. What do you think? ros2/main is fine, but I'm concerned about foxy actually that I should revert that merge.

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 13, 2021

Well before there was no param, and now there is one that should not conflict with any.

I believe the only people that might be affected are those that had slam_toolbox parameters in their nav2 bringup parameter file. Which did not make sense since they were not being used.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 13, 2021 •
edited
Loading

https://github.com/SteveMacenski/slam_toolbox/pull/364/files what do you mean? there was a parameter that was renamed. I have to assume most people did not use the slam:=True setting in the tb3 bringup demo and instead use their own param file and launched separately

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 13, 2021

Yes sorry, the param that we added last week was just renamed, but I think it was after the last sync to foxy.

So the rename would only affect people who use slam_toolbox from source and started using the parameter in the last 8 days.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 15, 2021

OK, reverting that foxy-devel PR SteveMacenski/slam_toolbox#365 but we're leaving / merging the ros2 ones that will roll into Galactic and anything newer. We don't want to break behavior in the middle of a distribution

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 15, 2021

I think that means we can close this PR too since this is foxy specific and the slam toolbox params won't support changing that name in the middle of a released distro? Or is there a different way around this for just foxy to enable this behavior?

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 15, 2021

I don't think I follow.

We added a param 10 days ago and we have to remove it or rename it to fix the issue you reported.

Removing it breaks API as much as renaming it. But renaming with the changes I made will allow us to keep the functionality.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 15, 2021 •
edited
Loading

SteveMacenski/slam_toolbox#365 I did remove it, it breaks user behavior in an already released distribution, its as simple as that. It can't go into Foxy. To enable this behavior in foxy, it must not break already released behavior. Between distributions and in ros2 or main this is OK.

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 15, 2021 •
edited
Loading

Yeah, but reverting that will cause #2240 to appear on foxy.

To fix #2240, either the original PR https://github.com/SteveMacenski/slam_toolbox/pull/347/files must be reverted and the param removed altogether, or the param must be renamed as I did with SteveMacenski/slam_toolbox#364.

Keeping the name params_file is what causes #2240, which is more damaging than renaming a parameter that was added just 10 days ago.

I apologise for the confusion, and if you choose to remove it altogether I will look for an alternative way of getting on our repositories to make it work, but the current state of foxy as of now will not work due to #2240.

My point being that 10 days ago this behavior did not exist, it was added but has not yet been released in foxy, so techically it's not breaking any released behavior.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 15, 2021 •
edited
Loading

#2240 was never merged into foxy, only main. None of those push/pop things were ever put into foxy to break anything.

I don't understand your point. We added something to main that broke default behavior for Slam toolbox. You fixed it and it was merged. But you also fixed something that wasn't broken in foxy because there was never a foxy Nav2 patch for the issue. But we merged it into slam toolbox for some reason so I reverted it.

So foxy, if we close this PR, will be as if nothing happened at all. Main in Nav2/ROS2 in SLAM Toolbox will contain the behavior you were looking to add with setting parameters via the main nav2 file. Foxy will not.

Ergo, I'm asking if its ok to close this PR.

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 15, 2021

You are right, I was mixing the push/pop issue that affects the nav2 nodes, with the issue that using params_file as a LaunchConfiguration in both nav2_bringup and slam_toolbox causes.

The whole purpose of the Push/Pop was to prevent the nav2 params_file from being automatically passed to slam_toolbox. In turn, the push/pop caused #2240.

But in foxy the params_file provided to nav2_bringup is being forwarded automatically to slam_toolbox, preventing the loading of the default parameter files.

That's why I was saying that we should rename params_file in slam_toolbox via SteveMacenski/slam_toolbox#364 or remove the param altogether, but I was wrongly referencing #2240.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 15, 2021 •
edited
Loading

So can we close this since this is for foxy-devel? I'm not disagreeing with the issue, but I'm saying that we cannot rename a param like that mid-release. We would need to find another way around it for Foxy. Hence, closing this PR is what I'm asking if you're OK with.

@SteveMacenski SteveMacenski reopened this Mar 15, 2021
@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 16, 2021

So can we close this since this is for foxy-devel? I'm not disagreeing with the issue, but I'm saying that we cannot rename a param like that mid-release. We would need to find another way around it for Foxy. Hence, closing this PR is what I'm asking if you're OK with.

Ok, let's close this then.

For fixing foxy slam_toolbox bug we could search if the provided param file has slam_toolboxparameters, and if it doesn't, ignore the parameter and load the default one. It's hacky but it'd be backwards compatible.

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 16, 2021

I don't think there's a bug in Slam toolbox, there's a bug in launch that impacts Nav2's usage of slam toolbox

@v-lopez
Copy link
Copy Markdown
Contributor Author

v-lopez commented Mar 16, 2021

Agreed, would you be ok with the workaround I proposed on my previous comment?

@SteveMacenski
Copy link
Copy Markdown
Member

SteveMacenski commented Mar 16, 2021

Sure, we could do that.

v-lopez pushed a commit to pal-robotics-forks/slam_toolbox that referenced this pull request Mar 17, 2021
Check params_file arg, and use default if it has no slam_toolbox params
f4f29fb 
This is a workaround to
ros-navigation/navigation2#2243 to keep foxy API
stable by not changing the params_file name.
v-lopez pushed a commit to pal-robotics-forks/slam_toolbox that referenced this pull request Mar 17, 2021
Check params_file arg, and use default if it has no slam_toolbox params
0df5e79 
This is a workaround to
ros-navigation/navigation2#2243 to keep foxy API
stable by not changing the params_file name.
@v-lopez v-lopez mentioned this pull request Mar 17, 2021
Merged
SteveMacenski pushed a commit to SteveMacenski/slam_toolbox that referenced this pull request Mar 18, 2021
@v-lopez
Check params_file arg, and use default if it has no slam_toolbox para…
99e12e5 
…ms (#368)

* Check params_file arg, and use default if it has no slam_toolbox params

This is a workaround to
ros-navigation/navigation2#2243 to keep foxy API
stable by not changing the params_file name.

* Use has_node_params in nav2_common
maxlein pushed a commit to kin-dergarten/slam_toolbox that referenced this pull request Aug 5, 2021
@v-lopez @maxlein
Check params_file arg, and use default if it has no slam_toolbox para…
54cf431 
…ms (SteveMacenski#368)

* Check params_file arg, and use default if it has no slam_toolbox params

This is a workaround to
ros-navigation/navigation2#2243 to keep foxy API
stable by not changing the params_file name.

* Use has_node_params in nav2_common
@hcdiekmann hcdiekmann mentioned this pull request Mar 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@v-lopez @SteveMacenski