Bond support + fixing various action server issues #1894

SteveMacenski · 2020-07-29T00:14:33Z

Adding heartbeat support for servers so if they crash or fail, the lifecycle manager will kill the system.

#1869 replacement

Tested:

Goes down when a server crashes
Goes down when a server breaks the bond
Up stably >> 1 hour without timeouts on network
Clean exiting
Up - down - up again testing (minus Planner server doesn't receive correct goals after deactivation->activation cycle #1893 which was around before this)
Parameter setting works
heartbeat disable option playing well

Completed:

Test coverage
Documentation updates to navigation website

Relies on ros/bond_core#67 being merged but updated the ros2_depedencies.repos file to use my fork + branch that this PR represents

SteveMacenski · 2020-07-29T00:54:43Z

@naiveHobo please review. ~~The server side disable point I made I'll work on tomorrow.~~ found solution, was easier than I thought.

SteveMacenski · 2020-07-30T00:05:10Z

@naiveHobo review now would be good, added complete coverage of the feature set

nav2_lifecycle_manager/src/lifecycle_manager.cpp

naiveHobo · 2020-07-30T20:08:32Z

The lifecycle_manager tests failed since it's not able to form a bond with the test lifecycle node.

I think it would make sense to make bond optional in lifecycle_manager. That will allow any rclcpp_lifecycle::LifecycleNode to use nav2_lifecycle_manager::LifecycleManager out of the box without worrying about setting up bond.

SteveMacenski · 2020-07-30T20:11:00Z

The lifecycle_manager tests failed since it's not able to form a bond with the test lifecycle node.

Ah, it is optional, I'll update the test to set that up

SteveMacenski · 2020-07-30T20:13:04Z

Oh whoops closed it

SteveMacenski · 2020-07-30T21:14:17Z

Should be all good now. Funny enough, that actually covered one test I didnt cover (the case of no bond connection) so that's convenient :-)

SteveMacenski · 2020-07-30T22:42:09Z

Great, actions are failing tests again.

SteveMacenski · 2020-07-30T22:47:17Z

Actually, I think I uncovered another failure in the action server wrapper - one sec. there was a completely unsafe autostart feature that wasn't totally implemented I removed and I think the tests assumed that it was activated unfairly without telling it to do so (which no other server does other than the tests... not great)

Edit: tested locally, that was it - fixed now

codecov · 2020-07-30T23:18:36Z

Codecov Report

Merging #1894 into master will increase coverage by 0.26%.
The diff coverage is 97.19%.

@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
+ Coverage   71.12%   71.38%   +0.26%     
==========================================
  Files         222      222              
  Lines       10676    10760      +84     
==========================================
+ Hits         7593     7681      +88     
+ Misses       3083     3079       -4

Impacted Files	Coverage Δ
nav2_util/include/nav2_util/lifecycle_node.hpp	`100.00% <ø> (ø)`
nav2_bt_navigator/src/bt_navigator.cpp	`80.37% <71.42%> (+0.25%)`	⬆️
nav2_lifecycle_manager/src/lifecycle_manager.cpp	`96.08% <98.33%> (+3.28%)`	⬆️
nav2_amcl/src/amcl_node.cpp	`83.96% <100.00%> (-0.11%)`	⬇️
nav2_controller/src/nav2_controller.cpp	`79.20% <100.00%> (-0.27%)`	⬇️
nav2_map_server/src/map_server/map_server.cpp	`90.47% <100.00%> (+0.23%)`	⬆️
nav2_planner/src/planner_server.cpp	`72.46% <100.00%> (+0.40%)`	⬆️
...v2_recoveries/include/nav2_recoveries/recovery.hpp	`86.07% <100.00%> (+0.36%)`	⬆️
nav2_recoveries/src/recovery_server.cpp	`92.50% <100.00%> (+0.19%)`	⬆️
...v2_util/include/nav2_util/simple_action_server.hpp	`88.12% <100.00%> (+2.32%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95b905c...13c885f. Read the comment docs.

gramss · 2020-08-01T11:12:26Z

rosdep installing your bond_core branch failed for me:
I'm running 20.04 and freshly installed foxy from debs.

rosdep install -y -r -q --from-paths src --ignore-src --rosdistro foxy
ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
nav2_util: Cannot locate rosdep definition for [bond]
nav2_lifecycle_manager: Cannot locate rosdep definition for [bondcpp]
Continuing to install resolvable dependencies...

(no other mention of bond or your lifecycle branch)

Installing your branch manually works fine, after cleaning the workspace.

SteveMacenski · 2020-08-01T17:34:26Z

See the repos file you should be building with master for dependencies.

gramss · 2020-08-01T18:57:07Z

yes, I cloned the master branch of nav2, which is default.
This is why I commented this. As I think it did not work as expected.
Just the automatism via repos file did not work, but I think CI is setup and working for master. So, it was probably something local on my machine..?
It worked manually, so no big issue.

SteveMacenski · 2020-08-01T19:31:17Z

I don't know what you're asking. See our build instructions for master branch https://navigation.ros.org/build_instructions/index.html#build-ros-2-master it requires you work with the repos file. If you choose not to, then don't run master, run a specific distribution released branch

SteveMacenski · 2020-08-01T19:49:35Z

Master assumes you're building the dependencies we set out in the repos file, because often we need to make updates to fix issues or add features so we need to be able to use non-released versions in master development.

gramss · 2020-08-01T22:29:55Z

aaaah... I was assuming rosdep uses the .repos file. But vcs is responsible to work with that..
I was just too fast skimming the docs. sorry for bothering..
I thought that some clever mechanics (vcs, but I thought that rosdep would do the job) failed to work (due to this PR) on current master and wanted to notify you, so it can be fixed..

But I was just following the wrong docs (and skipping git checkout a dedicated branch for release, as I wanted master)..

* prototype of lifecycle bond system * adding more structure to get around weak ptr issue * working prototype for manager * adding some ns -> s conversions * changing to service node * adding bond connections to all servers * update logs * fixing review comments * fix types * remove extraneous functions * make linters happy * simplifications * adding spinner to get working but now unstable * moving bond connections to activate state * adding defaults * working complete prototype for review * update dependencies * adding connection logging * remove accidental file * fix server side timeout for heartbeats * adding complete unit coverage of bond support * fixing lifecycle test * trying to activate since autostart was removed

daisukes · 2020-08-22T05:59:43Z

@SteveMacenski, I have a question about this.

As commented here, if one of them is down then bring them all down.
https://github.com/ros-planning/navigation2/blob/d191e0a41148ef87597711d5bab18dda98fa7c0c/nav2_lifecycle_manager/src/lifecycle_manager.cpp#L345

How am I supposed to bring them up again?
I'm using auto_start to activate at startup, so I want them to come back automatically.
Watching the lifecycle manager state?

SteveMacenski · 2020-08-24T17:42:58Z

Using the lifecycle manager start functionality like you would if you didn't have auto start set. It just goes back down to the dormant state, you can restart it as normal.

* prototype of lifecycle bond system * adding more structure to get around weak ptr issue * working prototype for manager * adding some ns -> s conversions * changing to service node * adding bond connections to all servers * update logs * fixing review comments * fix types * remove extraneous functions * make linters happy * simplifications * adding spinner to get working but now unstable * moving bond connections to activate state * adding defaults * working complete prototype for review * update dependencies * adding connection logging * remove accidental file * fix server side timeout for heartbeats * adding complete unit coverage of bond support * fixing lifecycle test * trying to activate since autostart was removed

tonynajjar · 2021-11-16T16:12:17Z

Hi!

From Eloquent to Foxy, I deduce this feature should be available in Foxy but I don't see the changes of this PR in the foxy-devel branch (e.g. no bond in bt_navigator). Does that mean the feature is not in Foxy? Am I missing something?

Thanks in advance!

SteveMacenski · 2021-11-16T17:20:02Z

Correct, it is in galactic and newer

adamsj-ros · 2021-12-31T01:48:22Z

nav2_util/src/lifecycle_node.cpp

+    this->get_name(),
+    shared_from_this());
+
+  bond_->setHeartbeatPeriod(0.10);


@SteveMacenski Can I ask where this value for the period came from? The message is small but on our system running many parts of nav2, this equates to almost 100Hz on the bond topic for 5 nodes.

This should be 10hz, not 100hz https://github.com/ros/bond_core/blob/ros2/bondcpp/src/bond.cpp#L291-L299. It was selected to offer a reasonable trade off between responsiveness to a server going down and how long before it goes down would it continue to be unsafe for a robot to operate. Since this needs to match from the lifecycle node -> lifecycle manager and vise versa, this couldn't be easily parameterized so that they would be guaranteed to match, so this is one of the very few "magic numbers" in the stack.

I'd be more than happy to discuss a proposal though to change to another value if you had a suggestion and rationale to share!

The stack up of several bond publishers on the same bond topic is giving us the 100 Hz as measured from ros2 topic hz /bond.

Could this value not be kept as a default in a common header file with an associated parameter name?

There are not currently any common headers between all of these systems, that I am aware of at least.

You should see 10 * N messages on the topic per second, that is correct, where N is the number of lifecycle bond monitored servers. This should be very attainable for DDS given these messages are very small and the default bringup right now includes composition so its all intraprocess.

Is there some precedent for the rate selected from other usage of the bond topic? It seems like there could be a distance metric derived from max speed to come up with a time interval and a timeout value.

That would probably break some encapsulation in the servers, since many don't know or care about the robot's speed. What are you trying to solve? 100hz is nothing, that shouldn't be causing any issues even on small platforms!

mergify · 2021-12-31T01:48:55Z

This pull request is in conflict. Could you fix it @SteveMacenski?

salmagro · 2022-05-16T12:32:00Z

Using the lifecycle manager start functionality like you would if you didn't have auto start set. It just goes back down to the dormant state, you can restart it as normal.

@SteveMacenski I have a question regarding relaunching a custom node when it crashes.

My custom node inherits from the nav2 lifecycle.
I added the createBond() on the on_activate() transition. Like here: https://github.com/ros-planning/navigation2/blob/main/nav2_controller/src/controller_server.cpp#L233-L234
I pass the node name from the lifecycle_nodes list to the nav2_lifecycle_manager.
The node is not being relaunched.

What else do I need to configure? Is there any example or documentation? Thanks.

SteveMacenski · 2022-05-16T17:14:50Z

You have the set respawn as true in launch so that the servers respawn, this is a relatively new feature. I know its in Rolling, but I'm not sure if its been (or will be) backported to Galactic or Foxy. Respawn is a launch feature

SteveMacenski added 16 commits July 13, 2020 19:21

prototype of lifecycle bond system

c483e88

adding more structure to get around weak ptr issue

36ed224

working prototype for manager

9832123

adding some ns -> s conversions

ed9f124

changing to service node

21778b5

adding bond connections to all servers

fcc7cd6

update logs

227c30a

fixing review comments

1f017ee

fix types

ccacefb

remove extraneous functions

c2020d1

make linters happy

78c6f73

simplifications

19e1dcc

adding spinner to get working but now unstable

4640d22

moving bond connections to activate state

c159968

adding defaults

2f95d54

working complete prototype for review

9164130

SteveMacenski requested a review from naiveHobo July 29, 2020 00:15

SteveMacenski added 4 commits July 28, 2020 17:17

update dependencies

4c45eb0

adding connection logging

f853e0e

merge master

854c5e6

remove accidental file

133f09c

fix server side timeout for heartbeats

db42454

SteveMacenski mentioned this pull request Jul 29, 2020

[ROS2] Failing heartbeat timeouts ros/bond_core#65

Closed

adding complete unit coverage of bond support

4f327b9

naiveHobo reviewed Jul 30, 2020

View reviewed changes

nav2_lifecycle_manager/src/lifecycle_manager.cpp Show resolved Hide resolved

SteveMacenski closed this Jul 30, 2020

SteveMacenski reopened this Jul 30, 2020

fixing lifecycle test

0e8a980

naiveHobo approved these changes Jul 30, 2020

View reviewed changes

trying to activate since autostart was removed

13c885f

SteveMacenski merged commit ec26039 into master Jul 30, 2020

SteveMacenski deleted the bond_lifecycle branch July 30, 2020 23:19

adamsj-ros reviewed Dec 31, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bond support + fixing various action server issues #1894

Bond support + fixing various action server issues #1894

SteveMacenski commented Jul 29, 2020 •

edited

Loading

SteveMacenski commented Jul 29, 2020 •

edited

Loading

SteveMacenski commented Jul 30, 2020

naiveHobo commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020 •

edited

Loading

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020 •

edited

Loading

codecov bot commented Jul 30, 2020

gramss commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

gramss commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

gramss commented Aug 1, 2020

daisukes commented Aug 22, 2020

SteveMacenski commented Aug 24, 2020

tonynajjar commented Nov 16, 2021 •

edited

Loading

SteveMacenski commented Nov 16, 2021

adamsj-ros Dec 31, 2021

SteveMacenski Jan 3, 2022

adamsj-ros Jan 3, 2022

SteveMacenski Jan 3, 2022

adamsj-ros Jan 4, 2022

SteveMacenski Jan 4, 2022

mergify bot commented Dec 31, 2021

salmagro commented May 16, 2022

SteveMacenski commented May 16, 2022 •

edited

Loading

Bond support + fixing various action server issues #1894

Bond support + fixing various action server issues #1894

Conversation

SteveMacenski commented Jul 29, 2020 • edited Loading

SteveMacenski commented Jul 29, 2020 • edited Loading

SteveMacenski commented Jul 30, 2020

naiveHobo commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020 • edited Loading

SteveMacenski commented Jul 30, 2020

SteveMacenski commented Jul 30, 2020 • edited Loading

codecov bot commented Jul 30, 2020

Codecov Report

gramss commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

gramss commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

SteveMacenski commented Aug 1, 2020

gramss commented Aug 1, 2020

daisukes commented Aug 22, 2020

SteveMacenski commented Aug 24, 2020

tonynajjar commented Nov 16, 2021 • edited Loading

SteveMacenski commented Nov 16, 2021

adamsj-ros Dec 31, 2021

Choose a reason for hiding this comment

SteveMacenski Jan 3, 2022

Choose a reason for hiding this comment

adamsj-ros Jan 3, 2022

Choose a reason for hiding this comment

SteveMacenski Jan 3, 2022

Choose a reason for hiding this comment

adamsj-ros Jan 4, 2022

Choose a reason for hiding this comment

SteveMacenski Jan 4, 2022

Choose a reason for hiding this comment

mergify bot commented Dec 31, 2021

salmagro commented May 16, 2022

SteveMacenski commented May 16, 2022 • edited Loading

SteveMacenski commented Jul 29, 2020 •

edited

Loading

SteveMacenski commented Jul 29, 2020 •

edited

Loading

SteveMacenski commented Jul 30, 2020 •

edited

Loading

SteveMacenski commented Jul 30, 2020 •

edited

Loading

tonynajjar commented Nov 16, 2021 •

edited

Loading

SteveMacenski commented May 16, 2022 •

edited

Loading