Cleanup of https://github.com/ros2/rclcpp/pull/2683

[jmachowinski]

We did a quick fix of #2664 in order to get it merged fast in jazzy.

This is the proper fix for the issue reported in #2664

[jmachowinski]

@mjcarroll @wjwwood @fujitatomoya @alsora
While doing the cleanup I found some other issues in the code. We should definitely backport the double adding of the notifier the jazzy....

[jmachowinski]

There is still one issue going on, were I don't know if its a bug in the rmw or not.
The reason why I don't know if it is a bug or not, is that it is not clearly specified what
the behavior is if I add an entity twice to a waitset.

If you add a guard condition twice to a rcl_waitsets, the behavior is odd.
If the guard condition is triggered, only the first entry in the rcl_waitset is set, the second is nullptr.

The problem here, is that this breaks code like this :

  rcl_guard_condition_t * cond = &guard_condition_->get_rcl_guard_condition();

  size_t idx1;
  rcl_wait_set_add_guard_condition(&wait_set, cond, &idx1);

  size_t idx2;
  rcl_wait_set_add_guard_condition(&wait_set, cond, &idx2);

  guard_condition_->trigger();

  rcl_wait(&wait_set, 1);
  
  if(wait_set.guard_conditions[idx1] != nullptr)
  {
    // do something
  }

 // this entry will be a nullptr, even though it should not.
  if(wait_set.guard_conditions[idx2] != nullptr)
  {
    // do something
  }

[clalancette]

If you add a guard condition twice to a rcl_waitsets, the behavior is odd.

Which RMW are you testing with? While the behavior should in theory be consistent across them, they all implement it fairly differently.

[jmachowinski]

@clalancette I used the default, so this should have been fastDDS.
The big question here is, what is the wanted behavior ?
If we all agree, that the second guard condition, should not be null as well, I can come up with a test and patch....

[wjwwood]

The big question here is, what is the wanted behavior ?
If we all agree, that the second guard condition, should not be null as well, I can come up with a test and patch....

We discussed this in the ROS 2 PMC meeting as well as in the latest client library working group meeting, but for everyone else's benefit the conclusion was that adding the same entity (guard condition, timer, sub, etc...) to a wait set should be an error and we shouldn't require the rmw layer to allow it and leave both non-null. I believe the agreement was to make the check in the functions that add the entities to the wait set, but we may also try to avoid this in the calling code in rclcpp.

[jmachowinski]

Pulls: #2714
Gist: https://gist.githubusercontent.com/jmachowinski/08c4a37a9860fd54b4b0476dde773ae1/raw/93b766847151d1c56a80851ffa36e5b830619230/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15075

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[jmachowinski]

@mjcarroll can you have a look at 5eaf3dc ?
This changes the test, in a way, that it does not add the same waitable twice to the waitset, but I am not sure what the test was supposed to test in the first place, and if the test now still tests it...

[mjcarroll]

I had to rebuild a little bit of context here around this test. I think that the original intention was to see that the guard_condition associated with the node's callback group is only fired in the case that the contents of the callback group changed. This is why we spin, see that the execute happens, and then spin again to verify that it doesn't happen.

I think that there could be other ways to test that this happens without relying on adding the guard condition to the waitset twice.

[mjcarroll]

Aside from that, this looks good. If we need to come up with another way to test, then that's fine.