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

Selector plugin for selecting the planner and the controller #4091

Merged
merged 16 commits into from
Feb 9, 2024

Conversation

padhupradheep
Copy link
Member


Basic Info

Info Please fill out this column
Ticket(s) this addresses #3269)
Primary OS tested on Ubuntu 22.04
Robotic platform tested on Neobotix MPO-700
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

Now we can select the controller and the planner plugins on the fly

Description of documentation updates required from your changes

Maybe RViz plugins after all needs a documentation somewhere?


Future work that may be required in bullet points

  • Goal checkers and smoothers could be added to the selection list if needed

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@padhupradheep
Copy link
Member Author

padhupradheep commented Feb 1, 2024

Ah right forgot about signing the commits.. It's been bit long since I gave in a PR.

Also just remembered that I also need to add the defaults in all our default BTs. Will update that too..

Copy link
Contributor

mergify bot commented Feb 1, 2024

@padhupradheep, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@padhupradheep padhupradheep marked this pull request as draft February 1, 2024 20:59
@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from 7c2dbce to c31fed7 Compare February 1, 2024 21:03
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Generally looks good to me! What's this tool look like? 😄

nav2_rviz_plugins/src/selector.cpp Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

Also, seems easy enough to add the smoother / goal checker / progress checkers here too! Nice, simple tool, I like it!

@padhupradheep
Copy link
Member Author

Video demo:
Here is a video demo

Also, seems easy enough to add the smoother / goal checker / progress checkers here too! Nice, simple tool, I like it!

Shall I add those too?

@SteveMacenski
Copy link
Member

Shall I add those too?

Please!

@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from 6209c9f to 1e77c77 Compare February 2, 2024 21:01
@SteveMacenski
Copy link
Member

Progress checker too ❤️

I think then its just the BT updates and perhaps this is good for testing / merging? What do you think?

@padhupradheep
Copy link
Member Author

padhupradheep commented Feb 4, 2024

Progress checker too

There is no BT node for that as far as I checked. We can use the dynamic reconf server for it but that would be something unique and really won't fit to the scope of this PR.

I think then its just the BT updates and perhaps this is good for testing / merging? What do you think?

Yes, almost done! I need to update the docs about the updates in the BT and probably you can give it a swing when you get some time and we are all set for the merge once after that.

@SteveMacenski
Copy link
Member

There is no BT node for that as far as I checked.

Oh huh, that must be an oversight. OK.

Yes, please update the docs to match!

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Pending docs!

@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from eeb7dc3 to 1a3fc5c Compare February 6, 2024 17:32
@padhupradheep padhupradheep marked this pull request as ready for review February 6, 2024 17:41
@padhupradheep
Copy link
Member Author

Shall I add this plugin as default in nav2 bringup ?

Since it's more of a development tool, I feel it would be better if the user selects it by hand when required.

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 6, 2024

Yeah that sounds great! Though not in the same spot / scale as your gif. Above or below the Nav2 plugin with similar button spacing / scale as that

@padhupradheep
Copy link
Member Author

padhupradheep commented Feb 6, 2024

image

The older configuration does not seem to fit the complete width of RViz (the panel that has the fps just disappears, because of that), even resizing doesn't help much.

Screenshot from 2024-02-06 19-47-39

That's why thought would do an horizontal arrangement as shown below.

image

bit more compact version..

Would this work?

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 6, 2024

Yeah that new 2x2 version works, though why not have the drop down on the same line as the label instead of a new line? Either way works. I leave it to your discretion / eye

If you're going to make a new gif with the format, try to use not just FollowPath and FollowPath2, but how about FollowPath and AvoidObstacles or something else with different modality implications.

@padhupradheep
Copy link
Member Author

why not have the drop down on the same line as the label instead of a new line

I tried it yesterday, It felt cramped by this way.

image

I'd stick to the other version..

@padhupradheep
Copy link
Member Author

Just leaving a short note on having the selector plugin has default in the rviz bringup. There is some sort of race condition that does not allow the selector plugin to populate atleast one of the component's combo boxes, atleast once in ten instances.

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 7, 2024

There is some sort of race condition that does not allow the selector plugin to populate atleast one of the component's combo boxes, atleast once in ten instances.

Do you see your error log about the server not being up yet? Can you change the auto parameters = parameter_client->get_parameters({plugin_type}); line to use the API which would give more feedback on failure?

It would be good to know where this is happening so we can see what might need to be retried / delayed to get it to work deterministically. 1/10 failures is not something we could merge :( For example, you could have that timer trigger repeatedly at your 200ms (I assume that 200 is ms) until it works properly and log an error each time it fails to get the parameters and waiting for the servers to be ready. Don't cancel until successful. That wouldn't delay rviz bringup either!

A-OK on the layout, it looks good!

@padhupradheep
Copy link
Member Author

Issue is fixed! Thanks for the suggestion! 👍

@SteveMacenski
Copy link
Member

CI had error I assume isn't related to this PR, but retriggering to check.

In the meantime, anything else you want to do before I test and merge?

@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from 6f9a40f to 605e465 Compare February 8, 2024 09:03
@padhupradheep
Copy link
Member Author

In the meantime, anything else you want to do before I test and merge?

Well, I just added the progress_checker too. I just updated from 2x2 to 3x2 configuration, with slight changes to the way combo boxes would be placed.

Now I think, it's ready to be tested.

@SteveMacenski
Copy link
Member

CI had error I assume isn't related to this PR, but retriggering to check.

Huh, your PR is regularly triggering these failures. Can you reproduce locally? Maybe try pulling in main?

@padhupradheep
Copy link
Member Author

I already did a rebase early today.. not sure what's causing the issue. Let me check..

@SteveMacenski
Copy link
Member

Beyond that, I tested it locally and looks good to me

@padhupradheep
Copy link
Member Author

My mistake, somehow messed up the commit while fixing the DCO. I just was following the recommendation it was giving out, but somehow it broke my changes 🤷‍♂️

@padhupradheep
Copy link
Member Author

https://app.circleci.com/pipelines/github/ros-planning/navigation2/10966/workflows/2b3de681-a592-4539-866f-bcfe9e88dedc/jobs/34400/tests

Now it seems to be the test_spin_behavior_fake.missing_result and test_drive_on_heading_recovery.missing_result

@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from 4bcfb51 to f1150e6 Compare February 9, 2024 13:23
@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 9, 2024

I was messing around there recently, but it all passed on my CI jobs and I saw that main was working too after merge. Maybe increment your cache to v20 in the 3x places in the circleci file to break it all? If that still fails, I'll take that as an action item to look into, I won't block you further with it.

@padhupradheep padhupradheep force-pushed the feature/selector-plugin branch from f1150e6 to a21be57 Compare February 9, 2024 17:45
@padhupradheep
Copy link
Member Author

v20 didn't help either..

please let me know, if there is something else that I can try to sort it outt.

@SteveMacenski SteveMacenski merged commit 70dc1f9 into ros-navigation:main Feb 9, 2024
9 of 11 checks passed
@SteveMacenski
Copy link
Member

Not your problem, probably me. If you wanted to take a look at it, the more the merrier 😉

Want to post on discourse with your new feature + gif? I think that's worthy of highlighting!

@padhupradheep
Copy link
Member Author

Not your problem, probably me. If you wanted to take a look at it, the more the merrier 😉

😄

Want to post on discourse with your new feature + gif? I think that's worthy of highlighting!

Oh! Sure can do that.. 👍

@padhupradheep
Copy link
Member Author

padhupradheep commented Feb 9, 2024

It took me one whole year to eventually get on this PR and get it up.. 😵‍💫

I'll clear #3347 too..

If you have anything else, feel free to tag me in and I promise it would be faster this time around.

@SteveMacenski
Copy link
Member

Thanks and will do!

enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
…igation#4091)

* initial commit

Signed-off-by: PRP <[email protected]>

* now dynamic reconfigurability also be achieved

Signed-off-by: PRP <[email protected]>

* updating the changes

Signed-off-by: PRP <[email protected]>

* fixing the mistake

Signed-off-by: PRP <[email protected]>

* adding default values for the selector

Signed-off-by: PRP <[email protected]>

* updating the plugin description

Signed-off-by: PRP <[email protected]>

* setting default in combox and removing after selected

Signed-off-by: PRP <[email protected]>

* minor

Signed-off-by: PRP <[email protected]>

* adding goal checker and smoother selectors to the plugins

Signed-off-by: PRP <[email protected]>

* updating all the relevant BTs

Signed-off-by: PRP <[email protected]>

* updating to 2x2 version

Signed-off-by: PRP <[email protected]>

* updating the bringup too

Signed-off-by: PRP <[email protected]>

* fixing the race condition

Signed-off-by: PRP <[email protected]>

* adding progress checker to the selector plugin

Signed-off-by: PRP <[email protected]>

* fixing CI

Signed-off-by: PRP <[email protected]>

* updating the cache version

Signed-off-by: PRP <[email protected]>

---------

Signed-off-by: PRP <[email protected]>
Signed-off-by: enricosutera <[email protected]>
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
…igation#4091)

* initial commit

Signed-off-by: PRP <[email protected]>

* now dynamic reconfigurability also be achieved

Signed-off-by: PRP <[email protected]>

* updating the changes

Signed-off-by: PRP <[email protected]>

* fixing the mistake

Signed-off-by: PRP <[email protected]>

* adding default values for the selector

Signed-off-by: PRP <[email protected]>

* updating the plugin description

Signed-off-by: PRP <[email protected]>

* setting default in combox and removing after selected

Signed-off-by: PRP <[email protected]>

* minor

Signed-off-by: PRP <[email protected]>

* adding goal checker and smoother selectors to the plugins

Signed-off-by: PRP <[email protected]>

* updating all the relevant BTs

Signed-off-by: PRP <[email protected]>

* updating to 2x2 version

Signed-off-by: PRP <[email protected]>

* updating the bringup too

Signed-off-by: PRP <[email protected]>

* fixing the race condition

Signed-off-by: PRP <[email protected]>

* adding progress checker to the selector plugin

Signed-off-by: PRP <[email protected]>

* fixing CI

Signed-off-by: PRP <[email protected]>

* updating the cache version

Signed-off-by: PRP <[email protected]>

---------

Signed-off-by: PRP <[email protected]>
savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
…igation#4091)

* initial commit

Signed-off-by: PRP <[email protected]>

* now dynamic reconfigurability also be achieved

Signed-off-by: PRP <[email protected]>

* updating the changes

Signed-off-by: PRP <[email protected]>

* fixing the mistake

Signed-off-by: PRP <[email protected]>

* adding default values for the selector

Signed-off-by: PRP <[email protected]>

* updating the plugin description

Signed-off-by: PRP <[email protected]>

* setting default in combox and removing after selected

Signed-off-by: PRP <[email protected]>

* minor

Signed-off-by: PRP <[email protected]>

* adding goal checker and smoother selectors to the plugins

Signed-off-by: PRP <[email protected]>

* updating all the relevant BTs

Signed-off-by: PRP <[email protected]>

* updating to 2x2 version

Signed-off-by: PRP <[email protected]>

* updating the bringup too

Signed-off-by: PRP <[email protected]>

* fixing the race condition

Signed-off-by: PRP <[email protected]>

* adding progress checker to the selector plugin

Signed-off-by: PRP <[email protected]>

* fixing CI

Signed-off-by: PRP <[email protected]>

* updating the cache version

Signed-off-by: PRP <[email protected]>

---------

Signed-off-by: PRP <[email protected]>
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.

2 participants