Add function for waiting on multiple wait sets together #330

jacobperron · 2018-11-14T23:02:57Z

Internally, the function collates all wait sets into a single wait set that is waited on.

Resolves #308

Internally, the function collates all wait sets into a single wait set that is waited on.

jacobperron · 2018-11-14T23:21:39Z

Linux
Linux-aarch64
macOS
Windows

hidmic

Looking good! Left a few comments.

hidmic · 2018-11-15T13:17:43Z

rcl/include/rcl/wait.h

@@ -428,6 +427,119 @@ RCL_WARN_UNUSED
 rcl_ret_t
 rcl_wait(rcl_wait_set_t * wait_set, int64_t timeout);

+/// Block until multiple wait sets are ready or until the timeout has been exceeded.
+/**
+ * This function has similar behavior as rcl_wait(), but acts on multiple wait sets.


@jacobperron nit: similar behavior to?

hidmic · 2018-11-15T13:33:21Z

rcl/test/rcl/test_wait.cpp

@@ -435,3 +435,28 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), guard_condition) {
  EXPECT_EQ(RCL_RET_OK, f.get());
  EXPECT_LE(std::abs(diff - trigger_diff.count()), TOLERANCE);
 }
+
+// Test rcl_wait_multiple with a positive finite timeout value (1ms)
+TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), finite_timeout_multiple) {


@jacobperron shouldn't we add a few more tests? Like, for negative timeout, for zero timeout, for invalid arguments, for invalid wait sets, with a dummy guard and a dummy timer in different wait sets to make sure things are being collated and then updated back properly, and so on. Not to repeat rcl_wait() tests, just to verify the new functionality is correct.

Yes, I meant to, but forgot.

hidmic · 2018-11-15T13:36:55Z

rcl/src/rcl/wait.c

+  size_t number_of_timers = 0u;
+  size_t number_of_clients = 0u;
+  size_t number_of_services = 0u;
+  for (i = 0u; i < num_wait_sets; ++i) {


@jacobperron it could be that no wait set was provided, should we have a different rcl error message for that case (right now it'll say that wait sets are empty)?

My intention was to return RCL_RET_WAIT_SET_EMPTY thinking it is intuitive, but I can add a new return code e.g. RCL_RET_WAIT_SET_NONE.

Either that or RCL_RET_INVALID_ARGUMENT plus RCL_SET_ERROR_MSG() with a nice message.

hidmic · 2018-11-15T13:37:12Z

rcl/src/rcl/wait.c

+    number_of_clients == 0u &&
+    number_of_services == 0u)
+  {
+    RCL_SET_ERROR_MSG("wait set is empty");


@jacobperron consider "wait sets are empty" instead.

hidmic · 2018-11-15T13:39:03Z

rcl/src/rcl/wait.c

+{
+  RCL_CHECK_ARGUMENT_FOR_NULL(wait_sets, RCL_RET_INVALID_ARGUMENT);
+  size_t i;
+  size_t j;


@jacobperron nit: I have a slight preference for declaring loop variables within the for statement. Won't block if you prefer to keep things C89-ish.

I guess my head was in C89 mode. Since we're targeting C99, I'm happy to change it.

I see now why I implemented this way. The existing code in this file follows a similar style:

rcl/rcl/src/rcl/wait.c

Lines 479 to 480 in f256da0

{ // scope to prevent i from colliding below

uint64_t i = 0;

rcl/rcl/src/rcl/wait.c

Line 556 in f256da0

size_t i;

Although, as a result there's that awkward scope to prevent variable name collisions... Maybe we should change those instances as well for consistency.

hidmic · 2018-11-15T13:41:45Z

rcl/src/rcl/wait.c

+      rcl_guard_condition_t * guard_condition = rcl_timer_get_guard_condition(wait_set->timers[j]);
+      if (NULL != guard_condition) {
+        // rcl_wait() will take care of moving these backwards and setting guard_condition_count.
+        const size_t index = wait_set->size_of_guard_conditions + (wait_set->impl->timer_index - 1);


@jacobperron not sure I follow how index would take different values across multiple iterations for the same wait_set.

It won't, this is a copy-paste bug.

hidmic · 2018-11-15T13:46:18Z

rcl/src/rcl/wait.c

+  size_t collated_service_index = 0u;
+  for (i = 0u; i < num_wait_sets; ++i) {
+    wait_set = &wait_sets[i];
+    for (j = 0u; j < wait_set->size_of_timers; ++j) {


@jacobperron nit: fine to use for loops, but we could use memcpy too (that depending on the compiler and target architecture may be more performant).

Sure, I can do that. Do you think there are any issues with readability (and correctness)?

memcpy( wait_set->timers, collated_wait_set.timers + collated_timer_index, sizeof(rcl_timer_t *) * wait_set->size_of_timers);

Ugh, that doesn't look nearly as clean as your current code. Nevermind, let's trust loop unrolling will work :).

jacobperron · 2018-11-15T22:42:06Z

@hidmic Thanks for the review! I think I've address all of your comments. Ready for another round of review.

jacobperron · 2018-11-16T01:16:48Z

Linux
Linux-aarch64
macOS
Windows

sloretz · 2018-11-16T01:37:26Z

rcl/src/rcl/wait.c

+  if (0u == num_wait_sets) {
+    RCL_SET_ERROR_MSG("number of wait sets must be greater than zero");
+    return RCL_RET_INVALID_ARGUMENT;
+  }


Could take a short cut and return rcl_wait() if 1u == num_wait_sets

rcl/src/rcl/wait.c

sloretz · 2018-11-16T01:58:24Z

rcl/test/rcl/test_wait.cpp

+    ret = rcl_wait_set_init(&wait_sets[i], 0, 1, 1, 0, 0, rcl_get_default_allocator());
+    ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+
+    // Add a dummy guard condition to avoid an error


What error is the dummy guard condition avoiding?

That is a good question. I copied this over from the existing test involving a timer, without it the test segfaults 😨

I dug into it a bit more, and it seems like the rmw guard condition count isn't being incremented in rcl_wait_set_add_timer(). Adding a dummy guard condition appears to be a hack to get the count to increment...

I think it can be fixed easily by ensuring the rmw guard condition count is incremented when adding timers, but without understanding the reason for the current implementation I am hesitant to change it. @wjwwood, is this a bug or intentional?

I don't know where it was copied from, but here maybe the error could be avoided if you also update the values passed to rcl_wait_set_init() above?

Nevermind, I see that the rmw guard condition count is incremented for timers in rcl_wait as the comment says (my bad):

rcl/rcl/src/rcl/wait.c

Line 502 in f256da0

++(rmw_gcs->guard_condition_count);

Setting the number of guard conditions to zero resolves the issue. I've also updated the tests for rcl_wait() that originally confused me. See e40a26b

jacobperron · 2018-11-16T20:12:37Z

Reviews addressed.

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-11-20T15:23:31Z

@jacobperron are we pushing this one as-is or are you considering a re-implementation in terms of the feature introduced by #335 ?

jacobperron · 2018-11-20T17:58:16Z

are we pushing this one as-is or are you considering a re-implementation in terms of the feature introduced by #335 ?

@hidmic I think this feature is not necessary anymore in favor of #335. If you agree, I will close this PR.

hidmic · 2018-11-20T18:01:03Z

Makes total sense to me, let's close it.

Add function for waiting on multiple wait sets together

1cf5909

Internally, the function collates all wait sets into a single wait set that is waited on.

jacobperron added the in progress Actively being worked on (Kanban column) label Nov 14, 2018

Add unit test and change input type to rcl_wait_set_t array

916694d

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 14, 2018

Add example to docs

ebf69fb

hidmic reviewed Nov 15, 2018

View reviewed changes

hidmic mentioned this pull request Nov 15, 2018

Completes integration tests for action client/server #331

Merged

2 tasks

jacobperron added 3 commits November 15, 2018 13:58

Address review

b87172e

Add more unit tests

bda60cb

Add test for checking input is updated properly

cb30b07

Fix doc example

3f1039d

sloretz requested changes Nov 16, 2018

View reviewed changes

jacobperron added 2 commits November 16, 2018 11:30

Add shortcut and fix bug in rcl_wait_multiple

b58dfa9

Refactored unit tests so there is no need for 'dummy' guard conditions.

e40a26b

jacobperron force-pushed the jacob/wait_multiple branch from 4bde955 to e40a26b Compare November 16, 2018 19:57

jacobperron closed this Nov 20, 2018

jacobperron removed the in review Waiting for review (Kanban column) label Nov 20, 2018

jacobperron deleted the jacob/wait_multiple branch April 4, 2019 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function for waiting on multiple wait sets together #330

Add function for waiting on multiple wait sets together #330

jacobperron commented Nov 14, 2018

jacobperron commented Nov 14, 2018

hidmic left a comment

hidmic Nov 15, 2018

hidmic Nov 15, 2018

jacobperron Nov 15, 2018

hidmic Nov 15, 2018

jacobperron Nov 15, 2018

hidmic Nov 15, 2018

hidmic Nov 15, 2018

hidmic Nov 15, 2018

jacobperron Nov 15, 2018

jacobperron Nov 15, 2018 •

edited

Loading

hidmic Nov 15, 2018

jacobperron Nov 15, 2018

hidmic Nov 15, 2018

jacobperron Nov 15, 2018 •

edited

Loading

hidmic Nov 15, 2018

jacobperron Nov 15, 2018

jacobperron commented Nov 15, 2018

jacobperron commented Nov 16, 2018

sloretz Nov 16, 2018

sloretz Nov 16, 2018

jacobperron Nov 16, 2018

wjwwood Nov 16, 2018

jacobperron Nov 16, 2018 •

edited

Loading

jacobperron commented Nov 16, 2018

hidmic commented Nov 20, 2018

jacobperron commented Nov 20, 2018

hidmic commented Nov 20, 2018

Add function for waiting on multiple wait sets together #330

Add function for waiting on multiple wait sets together #330

Conversation

jacobperron commented Nov 14, 2018

jacobperron commented Nov 14, 2018

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron commented Nov 15, 2018

jacobperron commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

jacobperron commented Nov 16, 2018

hidmic commented Nov 20, 2018

jacobperron commented Nov 20, 2018

hidmic commented Nov 20, 2018

jacobperron Nov 15, 2018 •

edited

Loading

jacobperron Nov 15, 2018 •

edited

Loading

jacobperron Nov 16, 2018 •

edited

Loading