WIP: Make BT nodes have configurable wait times.

[embeddedadam]

The previous solution provided a hardcoded 1s value.
Right now the value can be configured for BT
Action, Cancel, and Service nodes.

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#3920
Primary OS tested on	(Ubuntu)
Robotic platform tested on	(None)

---

Description of contribution in a few bullet points

Added configurable wait times for BT nodes.

Description of documentation updates required from your changes

Added a new parameter which is configurable with the time value.
Needs documentation of its meaning and usage.

Future work that may be required in bullet points

(To consider):
Even the value can be provided from the outside and can be configured,
there should be also a default one being set if no one plans to configure this parameter,
so the old applications maintain the same way of using this software and it makes it more
robust in the case of unintended usage.

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

[SteveMacenski]

That's all right for the nodes themselves, but a question for you to search into: Where are variables set on the blackboard and how can we use that method to also set this timeout value so that users can control it?

I know the answer, but its better for you to find it understand how the system works. If you get stumped let me know and I can point you in the right direction

[SteveMacenski]

Otherwise a small typo, looks good to me! If you fix this + open the docs PR, I can merge the pair!

[SteveMacenski]

You have lots of test failures: https://app.circleci.com/pipelines/github/ros-planning/navigation2/10446/workflows/befffbd1-f698-4d7f-b19e-819cdd7a3490/jobs/32990/tests. Beyond the linting errors, I think you need to make sure you update the tests that rely on these BT nodes to have it set in their blackboards as well!

[SteveMacenski]

Broken tests are the main thing now! After that, this PR is no longer WIP and ready to be removed from draft status!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e066ed1) 90.30% compared to head (e5e08e4) 90.31%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3960      +/-   ##
==========================================
+ Coverage   90.30%   90.31%   +0.01%     
==========================================
  Files         415      415              
  Lines       18462    18471       +9     
==========================================
+ Hits        16673    16683      +10     
+ Misses       1789     1788       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tonynajjar]

Cool PR, I don't know why we didn't think of making it configurable back then and just gave up on it: #3259

[tonynajjar]

Actually I have a nitpick, wait_for_service_timeout seems biased towards services but we actually also use it for action servers. Wouldn't wait_for_server_timeout have been a better name? Even then, having server_timeout and wait_for_server_timeout is confusing as they sound similar. Maybe server_response_timeout and server_bringup_timeout would be better suitable? It would probably be a pain to deprecate server_timeout though

[tonynajjar]

you missed changing this from 1 s to "wait_for_service_timeout_" seconds

[christophfroehlich]

We also had issues with the fixed timeouts, and want to use this PR in our projects. @SteveMacenski would it be possible to backport this to humble+iron? Though, I'm not sure if this could break ABI, but it could be fine because all the nodes are loaded dynamically and not linked against a released binary?

[christophfroehlich]

@SteveMacenski May I ping you again and ask for a backport to humble? Or maybe #4261 from iron.

[SteveMacenski]

wait_for_service_timeout does not appear to be declared to work properly as-is for a migration. It looks like it was added later to resolve that bug 1ef462e

So, I'm a little nervous about back porting this one since there were bugs identified that we missed on review / development and I haven't taken a deep look through the git history since this was merged to see if other bug fixes need to be applied with this to also make it work.

But, same with the manual backports. If I can do another review on this and have a second set of eyes (with that patch applied) then we can try. No promises on this one though.