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

Pass IDLE to on_tick, use that for initialize condition #4744

Merged

Conversation

redvinaa
Copy link
Contributor

@redvinaa redvinaa commented Nov 7, 2024

Basic Info

Info Please fill out this column
Ticket(s) this addresses #3988
Primary OS tested on (Ubuntu, MacOS, Windows)
Robotic platform tested on
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Pass IDLE state to BT action nodes (to the on_tick callback), and use that as initialize condition
  • This way ports can be updated at every restart (on first start and after each halt)
  • See conversation at the bottom of BT nodes not able to accept parameter values #3988

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.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

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.

I see no issues with this, LGTM once CI passes :-)

Thanks for bringing this to our attention!

@SteveMacenski
Copy link
Member

SteveMacenski commented Nov 7, 2024

@redvinaa it looks like 1 test is failing (ignore DWB; that's occasionally flaky). It probably just needs a quick update

@redvinaa
Copy link
Contributor Author

redvinaa commented Nov 8, 2024

I have no idea why that particular test failed. It's possible that a rerun would solve it.

@SteveMacenski
Copy link
Member

I triggered a rebuild, but I don't expect that to change, a unit test like that shouldn't be flaky

@redvinaa
Copy link
Contributor Author

Found it: the subscriber was always recreated in initialize, and as in the test we spin first and then tick, the callback was always called with the old inputs. Changed initialize to only create new subscriber when the topic has changed (or in the beginning when the sub doesn't exist yet).

@SteveMacenski SteveMacenski merged commit f426bda into ros-navigation:main Nov 11, 2024
11 checks passed
Jakubach pushed a commit to Jakubach/navigation2 that referenced this pull request Nov 22, 2024
…on#4744)

* Pass IDLE to on_tick, use that for initialize condition

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

* Fix battery sub creation bug

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

---------

Signed-off-by: redvinaa <[email protected]>
Signed-off-by: Jakubach <[email protected]>
Jakubach pushed a commit to Jakubach/navigation2 that referenced this pull request Nov 22, 2024
…on#4744)

* Pass IDLE to on_tick, use that for initialize condition

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

* Fix battery sub creation bug

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

---------

Signed-off-by: redvinaa <[email protected]>
Signed-off-by: Jakubach <[email protected]>
Jakubach pushed a commit to Jakubach/navigation2 that referenced this pull request Nov 22, 2024
…on#4744)

* Pass IDLE to on_tick, use that for initialize condition

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

* Fix battery sub creation bug

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

---------

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