Action server implementation #323

jacobperron · 2018-11-08T18:36:39Z

First iteration of implementation.

Connects to #306
Depends on ros2/rcl_interfaces#47
Depends on ros2/rosidl#310

jacobperron · 2018-11-09T02:46:56Z

Intermediate CI to find any issues:

Linux
Linux-aarch64
macOS
Windows

hidmic

Is the calling library responsible for updating the goal handle (i.e. to transition to execute, to succeeded, etc.)?

rcl_action/src/rcl_action/action_server.c

rcl_action/test/rcl_action/test_action_server.cpp

rcl_action/include/rcl_action/types.h

rcl_action/test/rcl_action/test_action_server.cpp

hidmic · 2018-11-12T15:13:17Z

Does this PR depend on #325 too?

jacobperron · 2018-11-12T20:00:33Z

@hidmic

Is the calling library responsible for updating the goal handle (i.e. to transition to execute, to succeeded, etc.)?

In the current implementation, yes. I was hoping to build on top of this (or refactor) after ros2/rosidl#314 in order to encapsulate more logic.

Does this PR depend on #325 too?

Yes, those commits are included in this PR for testing purposes, but I'll rebase after #325 is merged.

jacobperron · 2018-11-13T20:00:47Z

Rebased on master. I think this is ready for review now. The only thing missing is the wait set related functions (which I plan to add in a follow-up PR).

CI:

Linux
Linux-aarch64
macOS
Windows

jacobperron · 2018-11-13T22:28:28Z

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM but for a few small comments. Awesome work @jacobperron !

rcl_action/include/rcl_action/action_server.h

rcl_action/test/rcl_action/test_action_server.cpp

hidmic · 2018-11-14T18:07:43Z

rcl_action/test/rcl_action/test_action_server.cpp

+  // Try to initialize another action server with the same name
+  // FIXME(jacobperron): test failing
+  // rcl_action_server_t same_name_action_server = rcl_action_get_zero_initialized_server();
+  // ret = rcl_action_server_init(&same_name_action_server, &node, ts, action_name, &options);


@jacobperron I presume we're aiming to catch this by means of invalid service name checks, but it doesn't seem like that outcome means the service name is repeated (see here)?

That's right, I realized later that it doesn't care if service names are repeated and meant to remove this commented test. But I can add a test for invalid names.

I also found a bug where empty action names are acceptable because we are appending the additional service/topic strings to it (e.g. "/_action/send_goal").

Oh, that's a bug in rcl_action/names.cc.

hidmic · 2018-11-14T18:08:57Z

rcl_action/src/rcl_action/action_server.c

+  }
+  RCL_CHECK_ARGUMENT_FOR_NULL(goal_handles, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(num_goals, RCL_RET_INVALID_ARGUMENT);
+  *goal_handles = action_server->impl->goal_handles;


@jacobperron should we check for NULL == *goal_handles?

No. We can set *goal_handles to a valid pointer, regardless of it's initial value.

sloretz

Partial review, I'll continue reviewing but wanted to give some feedback first.

sloretz · 2018-11-14T16:45:06Z

rcl_action/package.xml

@@ -18,6 +18,7 @@
  <test_depend>ament_cmake_gtest</test_depend>
  <test_depend>ament_lint_common</test_depend>
  <test_depend>ament_lint_auto</test_depend>
+  <test_depend>test_msgs</test_depend>



Also add <build_depend> + <build_export_depend> for rosidl_generator_c, and <build_depend> for rmw + rcutils.

Addressed in 00ecdf0

sloretz · 2018-11-14T17:18:15Z

rcl_action/src/rcl_action/action_server.c

+  // Initialize clock
+  rcl_ret_t ret = rcl_clock_init(options->clock_type, &action_server->impl->clock, &allocator);
+  if (RCL_RET_OK != ret) {
+    return RCL_RET_ERROR;


Need to deallocate action_server->impl before returning

Addressed in c6b436e

sloretz · 2018-11-14T17:25:01Z

rcl_action/src/rcl_action/action_server.c

+    action_server->impl, "allocating memory failed", return RCL_RET_BAD_ALLOC);
+
+  // Initialize clock
+  rcl_ret_t ret = rcl_clock_init(options->clock_type, &action_server->impl->clock, &allocator);


I recommend allowing clock to be passed in to this function. A ROS clock created here won't receive sim time updates. The time source API's in the client libraries currently need the higher level clock type to do that.

https://github.com/ros2/rclcpp/blob/33a755c535d654c97401eb6d199580368c19eb40/rclcpp/include/rclcpp/time_source.hpp#L63

Okay. Then I suppose it will not be the action servers responsibility to finalize the clock and we hope that it stays valid as long as the action server is valid. Does this sound right?

Does this sound right?

I think so

Addressed in 4eb7fdb

sloretz · 2018-11-14T17:40:17Z

rcl_action/src/rcl_action/action_server.c

+  default_options.status_topic_qos = rcl_action_qos_profile_status_default;
+  default_options.allocator = rcl_get_default_allocator();
+  default_options.clock_type = RCL_ROS_TIME;
+  default_options.result_timeout.nanoseconds = (rcl_duration_value_t)9e11;  // 15 minutes


Recommend RCUTILS_S_TO_NS(15 * 60) for readability and to avoid the cast from a floating point literal.

https://github.com/ros2/rcutils/blob/3c982d048914e1c30002d8ea69d86ac1cc6976fd/include/rcutils/time.h#L30

Addressed in c6b436e

sloretz · 2018-11-14T17:58:22Z

rcl_action/src/rcl_action/action_server.c

+_goal_info_stamp_to_nanosec(const rcl_action_goal_info_t * goal_info)
+{
+  assert(goal_info);
+  return (int64_t)(goal_info->stamp.sec * 1e9) + (int64_t)goal_info->stamp.nanosec;


RCUTILS_S_TO_NS(goal_info->stamp.sec) + ...

Addressed in c6b436e

sloretz · 2018-11-14T17:59:43Z

rcl_action/src/rcl_action/action_server.c

+{
+  assert(nanosec);
+  assert(goal_info);
+  goal_info->stamp.sec = (int32_t) (*nanosec / (int64_t)1e9);


RCUTILS_NS_TO_S()
https://github.com/ros2/rcutils/blob/3c982d048914e1c30002d8ea69d86ac1cc6976fd/include/rcutils/time.h#L37

Addressed in c6b436e

sloretz · 2018-11-14T18:01:06Z

rcl_action/src/rcl_action/action_server.c

+  assert(nanosec);
+  assert(goal_info);
+  goal_info->stamp.sec = (int32_t) (*nanosec / (int64_t)1e9);
+  goal_info->stamp.nanosec = (uint32_t)(*nanosec - (int64_t)(goal_info->stamp.sec * 1e9));


nanosec % RCUTILS_S_TO_NS(1)

Addressed in c6b436e

sloretz · 2018-11-14T18:15:28Z

rcl_action/src/rcl_action/action_server.c

+  goal_handles = (rcl_action_goal_handle_t **)tmp_ptr;
+
+  // Allocate space for a new goal handle
+  tmp_ptr = allocator.allocate(sizeof(rcl_action_goal_handle_t), allocator.state);


I think this allocation could be removed by changing action_server->impl->goal_handles could be changed to an array of goal_handles instead of an array of pointers to goal handles.

True, the main reason I went with an array of pointers was that is made it easier for removing expired goals from the middle of the list:

rcl/rcl_action/src/rcl_action/action_server.c

Lines 533 to 535 in 06df1fd

// Fill in any gaps left in the array with pointers from the end

action_server->impl->goal_handles[i] = action_server->impl->goal_handles[num_goal_handles];

action_server->impl->goal_handles[num_goal_handles--] = NULL;

But I guess it wouldn't be too much overhead to copy the entire goal handle in this case (instead of just the pointer).

sloretz · 2018-11-14T19:47:01Z

rcl_action/src/rcl_action/action_server.c

+    ret = rcl_action_goal_handle_get_info(
+      action_server->impl->goal_handles[i], &status_message->msg.status_list.data[i].goal_info);
+    if (RCL_RET_OK != ret) {
+      return RCL_RET_ERROR;


I think this needs to .._fini() status_message before returning

Addressed in c6b436e

sloretz · 2018-11-14T19:48:48Z

rcl_action/src/rcl_action/action_server.c

+    ret = rcl_action_goal_handle_get_status(
+      action_server->impl->goal_handles[i], &status_message->msg.status_list.data[i].status);
+    if (RCL_RET_OK != ret) {
+      return RCL_RET_ERROR;


Same comment about deallocating status_message

Addressed in c6b436e

sloretz · 2018-11-14T22:31:03Z

rcl_action/src/rcl_action/action_server.c

+rcl_ret_t
+rcl_action_clear_expired_goals(
+  const rcl_action_server_t * action_server,
+  size_t * num_expired)


If there is a C++ wrapper for a goal handle then it might be helpful for the C++ API to separate this API into two: one that says which goals are expired, and another that expires a goal.

This function will fini() goal handles, but a user may have kept a reference to a language specific wrapper rclcpp_action::GoalHandle. It would be nice if the rcl_action goal handle was deallcated when the rclcpp_action::GoalHandle is destructed. Currently this deallocation could happen sooner and calls to the higher level wrapper might use a free'd object.

My original thinking was that since the action server is the entity creating the goal handles, it should be the one responsible for destroying them.

If a rcl goal handle is finalized, then the goal handle becomes invalid (ie. rcl_action_goal_handle_is_valid() returns false) and so, I'm pretty sure, any rcl operations involving the goal handle should fail with an error code. So at least the C++ wrapper of a goal handle wouldn't be using a free'd goal handle without getting an explicit error.

Is it enough for the client libraries to check their goal handles with rcl_action_goal_handle_is_valid() and/or get a fresh list of goal handles with rcl_action_server_get_goal_handles() ?

Is it enough for the client libraries to check their goal handles with rcl_action_goal_handle_is_valid()

I guess the question is what does a C++ goal handle wrapper have a reference to? If it has an rcl_action_goal_handle_t * rcl_handle_; pointing to storage owned by rcl_action_server_impl_t, then rcl_action_goal_handle_is_valid(rcl_handle_) isn't safe because the memory it's pointing to could change any time impl->goal_handles is resized. It seems like it needs to be the other way around where the C++ goal handle wrapper has an rcl_action_goal_handle_t and rcl_action_server_t is the one with a pointer to it.

I see; makes sense now.

Hmm, so just to be clear, rcl* will handle all incoming result requests by first checking for goal handle validity, rejecting the request once the goal handle is no longer valid and only then deallocating it, right? I'd be somewhat more comfortable with just marking the goal handle as expired to keep finalization and deallocation close together, but this will do to. Only downside is that an error in that case isn't really an error, but the normal logic flow, so we have to be careful not to throw/assert/abort.

Hmm, then how does the calling library know when a goal handle has expired?

Hmm, then how does the calling library know when a goal handle has expired?

What about if we return a list of expired goal handles as part of the expire function?

Sounds like an option. Though judging by @sloretz rclcpp API skeleton, I'd imagine that having the client library wrappers checking if the goal handle they own has expired or not may be simpler. But we can somehow reverse the mapping to know which wrappers have to go away when goal handles expire.

The client library can call rcl_action_server_goal_exists() to check if a previously accepted goal has expired.

Removed finalize of expired goal handles in 1140112.
It is expected that the client library will finalize goal handles either after they expire (which can be checked with rcl_action_server_goal_exists()) or after the action server is finalized.

Note, the rcl goal handles are still initialized in rcl_action_accept_new_goal(). I was going to update the API to take an already initialized goal handle as an argument, but then we lose the time stamp created when the goal was accepted:

rcl/rcl_action/src/rcl_action/action_server.c

Lines 362 to 370 in 1140112

// Re-stamp goal info with current time

rcl_action_goal_info_t goal_info_stamp_now = rcl_action_get_zero_initialized_goal_info();

goal_info_stamp_now = *goal_info;

rcl_time_point_value_t now_time_point;

rcl_ret_t ret = rcl_clock_get_now(&action_server->impl->clock, &now_time_point);

if (RCL_RET_OK != ret) {

return NULL; // Error already set

}

_nanosec_to_goal_info_stamp(&now_time_point, &goal_info_stamp_now);

If we want the client library to pass the goal handle to rcl, then we can

Create a setter function for the goal handle's info and override the info in rcl_action_accept_goal_info, or

Leave the responsibility to the client library to use the same clock and stamp the goal handles properly.

sloretz · 2018-11-15T00:01:35Z

rcl_action/src/rcl_action/action_server.c

+  }
+
+  // Shrink goal handle array if some goals expired
+  if (*num_expired > 0u) {


This should deallocate if the new size is zero because allocator.reallocate() is supposed to behave like realloc() which has undefined behavior when the new size is zero.

Addressed in c6b436e

sloretz · 2018-11-15T00:07:50Z

rcl_action/src/rcl_action/action_server.c

+    for (size_t i = 0; i < total_num_goals; ++i) {
+      goal_handle = action_server->impl->goal_handles[i];
+      rcl_ret_t ret = rcl_action_goal_handle_get_info(goal_handle, &goal_info);
+      assert(RCL_RET_OK == ret);


IIUC assert is compiled out in release builds. Is this an assert() because its calling out an assumption that the above call will always succeed? Is it possible the above call could fail in normal use?

I thought it should always succeed, but after thinking about it some more, this can fail if the goal handle is finalized outside of the action server's control.

Addressed in 8f9041e

sloretz · 2018-11-15T00:10:06Z

rcl_action/src/rcl_action/action_server.c

+
+      if (uuidcmp(request_uuid, goal_info.uuid)) {
+        if (rcl_action_goal_handle_is_cancelable(goal_handle)) {
+          goal_handles_to_cancel[num_goals_to_cancel++] = goal_handle;


Intentional postfix increment? I think the style guide says to always use prefix increment.

Yeah, this is intentional. Assigning the goal handle to index num_goals_to_cancel and then increment.

sloretz · 2018-11-15T00:11:21Z

rcl_action/src/rcl_action/action_server.c

+    for (size_t i = 0; i < total_num_goals; ++i) {
+      goal_handle = action_server->impl->goal_handles[i];
+      rcl_ret_t ret = rcl_action_goal_handle_get_info(goal_handle, &goal_info);
+      assert(RCL_RET_OK == ret);


same comment about assert

sloretz · 2018-11-15T00:11:31Z

rcl_action/src/rcl_action/action_server.c

+      if (rcl_action_goal_handle_is_cancelable(goal_handle) &&
+        ((goal_nanosec <= request_nanosec) || uuidcmp(request_uuid, goal_info.uuid)))
+      {
+        goal_handles_to_cancel[num_goals_to_cancel++] = goal_handle;


rcl_action/src/rcl_action/action_server.c

hidmic · 2018-11-15T18:47:58Z

Looks like this PR needs a rebase.

jacobperron · 2018-11-15T23:06:27Z

Rebased.

hidmic

Small comments, awesome work Jacob!

hidmic · 2018-11-16T12:50:16Z

rcl_action/src/rcl_action/action_server.c

+      allocator.deallocate(action_server->impl->goal_handles, allocator.state);
+    } else {
+      void * tmp_ptr = allocator.reallocate(
+        action_server->impl->goal_handles, num_goal_handles, allocator.state);


@jacobperron num_goal_handles --> num_goal_handles * sizeof(rcl_action_goal_handle_t *) ?

Yep, good catch.

Addressed 7aadb55

hidmic · 2018-11-16T13:03:08Z

rcl_action/test/rcl_action/test_action_server.cpp

+  rcl_action_cancel_request_t cancel_request = rcl_action_get_zero_initialized_cancel_request();
+  for (int i = 0; i < UUID_SIZE; ++i) {
+    cancel_request.goal_info.uuid[i] = static_cast<uint8_t>(i + 2);
+  }


@jacobperron just curious, why not reusing one of the init_test_uuid*() functions?

I can reuse init_test_uui0(), but the init_test_uuid1() wouldn't do because the ID doesn't exist in this test case.

Addressed 7aadb55

hidmic · 2018-11-16T13:04:06Z

rcl_action/test/rcl_action/test_action_server.cpp

+  rcl_action_goal_info_t * goal_info = &cancel_response.msg.goals_canceling.data[0];
+  for (int i = 0; i < UUID_SIZE; ++i) {
+    EXPECT_EQ(goal_info->uuid[i], static_cast<uint8_t>(i + 2));
+  }


@jacobperron could be EXPECT_TRUE(uuidcmp(goal_info->uuid, cancel_request.goal_info.uuid)); as well.

Addressed 7aadb55

hidmic · 2018-11-16T13:04:44Z

rcl_action/test/rcl_action/test_action_server.cpp

+    goal_info_out = &cancel_response.msg.goals_canceling.data[i];
+    for (size_t j = 0; j < UUID_SIZE; ++j) {
+      EXPECT_EQ(goal_info_out->uuid[j], static_cast<uint8_t>(i + j));
+    }


@jacobperron same as above but using goals_info_out instead.

In this case, we would need to reconstruct the uuid in a similar loop before comparing anyways.

hidmic · 2018-11-16T13:06:02Z

rcl_action/test/rcl_action/test_action_server.cpp

+  goal_info_out = &cancel_response.msg.goals_canceling.data[num_goals_canceling - 1];
+  for (int i = 0; i < UUID_SIZE; ++i) {
+    EXPECT_EQ(goal_info_out->uuid[i], static_cast<uint8_t>(i + goal_index));
+  }


@jacobperron same as above.

Addressed 7aadb55

* Move communcation tests to separate file * Define UUID_SIZE * Remove const for action server in rcl_action_accept_new_goal() * Make private function static * Add missing override specifiers

Introduce rcl_clock_t to action server implementation.

* Change internal goal handle array to be an array of pointers. * Add check for invalid action names

Rather than initializing internally.

The test is not checking expected behavior

Instead, leave it up to the caller to finalize goal handles. Renamed the function to rcl_action_expire_goals.

Bugfix: Ensure options (containing allocator) are copied before a finalize occurs (e.g. goto fail).

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

@jacobperron One more CI run and I think this one is ready to go.

jacobperron · 2018-11-20T18:07:05Z

Up to rcl_action:

Linux
Linux-aarch64
macOS
Windows

sloretz

Just tiny changes that can be done later.

sloretz · 2018-11-20T19:22:02Z

rcl_action/CMakeLists.txt

@@ -49,8 +53,11 @@ add_library(${PROJECT_NAME}
 )

 ament_target_dependencies(${PROJECT_NAME}
-  "rcl"
+  "rosidl_generator_c"


nit, alphabetical order

sloretz · 2018-11-20T19:22:32Z

rcl_action/CMakeLists.txt

+    target_include_directories(test_action_communication PUBLIC
+      include
+      ${rcl_INCLUDE_DIRS}
+      ${test_msgs_INCLUDE_DIRS}


nit, This is taken care of by ament_target_dependencies() below

sloretz · 2018-11-20T19:23:23Z

rcl_action/CMakeLists.txt

+    target_include_directories(test_action_server PUBLIC
+      include
+      ${rcl_INCLUDE_DIRS}
+      ${test_msgs_INCLUDE_DIRS}


nit, This is taken care of by ament_target_dependencies() below

jacobperron · 2018-11-20T19:59:25Z

@sloretz Thanks, I'll take note and include fixes for the small things in the next PR.

* Implement action server init, fini, and is_valid functions * Add macros for initializing services and publishers * Implement rcl_action_server_get_default_options() * Implement rcl_action_accept_new_goal() * Add function, rcl_action_server_goal_exists(), for checking if goal is already being tracked by an action server * Add unit tests * Implement rcl_action_server_get_goal_handles() * Implement rcl_action_server_get_options() * Implement rcl_action_server_get_action_name() * Implement rcl_action_get_goal_status_array() * Bugfix: reset pointers and size in type finalize functions Also let finalize functions be called on already finalized objects * Implement send/take functions for action server services * Implement action server publishers for feedback and status * Implement rcl_action_process_cancel_request() * Add partial communication tests * Define UUID_SIZE * Use type-erased pointer for rcl_action_publish_status() * Implement rcl_action_clear_expired_goals() Introduce rcl_clock_t to action server implementation. * Change internal goal handle array to be an array of pointers. * Add check for invalid action names * Do heap allocation of temporary array to satisfy MSVC compiler * Bugfix: finalize node in test tear downs and reset expected errors * Update documentation * Update package.xml * Pass in rcl_clock_t to action server Rather than initializing internally. * Do not finalize goal handles in expire function Instead, leave it up to the caller to finalize goal handles. Renamed the function to rcl_action_expire_goals.

This reverts commit cd89209.

dirk-thomas · 2018-11-27T03:49:49Z

rcl_action/CMakeLists.txt

@@ -86,6 +94,34 @@ if(BUILD_TESTING)
    )
    ament_target_dependencies(test_action_client "rcl" "test_msgs")
  endif()
+  ament_add_gtest(test_action_communication
+    test/rcl_action/test_action_communication.cpp
+  )


This test (as well as the one below) is only covering the default rmw impl. It would be good if they test would be invoked for every rmw impl.

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

jacobperron self-assigned this Nov 8, 2018

jacobperron mentioned this pull request Nov 8, 2018

Actions #306

Closed

10 tasks

jacobperron force-pushed the jacob/action_server branch 3 times, most recently from c33d94b to 3406328 Compare November 9, 2018 02:42

jacobperron mentioned this pull request Nov 10, 2018

[rcl_action] Add function to check if goal can be transitioned to CANCELING #325

Merged

jacobperron force-pushed the jacob/action_server branch from 3406328 to 40c2ce3 Compare November 10, 2018 01:34

hidmic reviewed Nov 12, 2018

View reviewed changes

jacobperron force-pushed the jacob/action_server branch from 539bd5e to 5b1a55f Compare November 13, 2018 19:58

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

jacobperron requested a review from sloretz November 13, 2018 20:01

jacobperron mentioned this pull request Nov 13, 2018

Add action integration tests #328

Closed

hidmic reviewed Nov 14, 2018

View reviewed changes

sloretz reviewed Nov 14, 2018

View reviewed changes

hidmic mentioned this pull request Nov 14, 2018

Makes rcl_action_get_*_name() functions check for empty action names. #329

Merged

sloretz 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 force-pushed the jacob/action_server branch from c6b436e to 06df1fd Compare November 15, 2018 23:06

hidmic reviewed Nov 16, 2018

View reviewed changes

jacobperron added 2 commits November 16, 2018 12:21

[rcl_action] Add action server boilerplate

69d88a4

[wip] Implement action server init, fini, and is_valid functions

7a26a61

jacobperron added 14 commits November 16, 2018 12:21

Address review

5bb55ef

* Move communcation tests to separate file * Define UUID_SIZE * Remove const for action server in rcl_action_accept_new_goal() * Make private function static * Add missing override specifiers

Use type-erased pointer for rcl_action_publish_status()

260d93f

Implement rcl_action_clear_expired_goals()

f658280

Introduce rcl_clock_t to action server implementation.

Finish rcl_action_clear_expired_goals() implementation

ef03884

* Change internal goal handle array to be an array of pointers. * Add check for invalid action names

Do heap allocation of temporary array to satisfy MSVC compiler

2c938cb

Bugfix: finalize node in test tear downs and reset expected errors

6bfa497

Update documentation

22d3ace

Update package.xml

5488680

Address review

906c6ce

Pass in rcl_clock_t to action server

df742be

Rather than initializing internally.

Remove commented test

02b9c28

The test is not checking expected behavior

Update default options getter

18fc054

Replace assertions with error return codes

2c734fe

Initialize output variables in tests to known invalid values

7028e3f

jacobperron force-pushed the jacob/action_server branch from bc9a5df to 7028e3f Compare November 16, 2018 20:25

jacobperron added 3 commits November 16, 2018 14:10

Do not finalize goal handles in expire function

1140112

Instead, leave it up to the caller to finalize goal handles. Renamed the function to rcl_action_expire_goals.

Fix incorrect allocation size + minro refactor

7aadb55

Add tests for invalid action names and fix bug

aba8f86

Bugfix: Ensure options (containing allocator) are copied before a finalize occurs (e.g. goto fail).

sloretz approved these changes Nov 20, 2018

View reviewed changes

jacobperron merged commit acc974e into master Nov 20, 2018

jacobperron deleted the jacob/action_server branch November 20, 2018 20:04

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

wjwwood mentioned this pull request Nov 21, 2018

refactor init to not be global #336

Merged

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "Add action server implementation (ros2#323)"

1306214

This reverts commit cd89209.

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "Add action server implementation (ros2#323)"

0725370

This reverts commit cd89209.

dirk-thomas reviewed Nov 27, 2018

View reviewed changes

	// Fill in any gaps left in the array with pointers from the end
	action_server->impl->goal_handles[i] = action_server->impl->goal_handles[num_goal_handles];
	action_server->impl->goal_handles[num_goal_handles--] = NULL;

	// Re-stamp goal info with current time
	rcl_action_goal_info_t goal_info_stamp_now = rcl_action_get_zero_initialized_goal_info();
	goal_info_stamp_now = *goal_info;
	rcl_time_point_value_t now_time_point;
	rcl_ret_t ret = rcl_clock_get_now(&action_server->impl->clock, &now_time_point);
	if (RCL_RET_OK != ret) {
	return NULL; // Error already set
	}
	_nanosec_to_goal_info_stamp(&now_time_point, &goal_info_stamp_now);

Action server implementation #323

Action server implementation #323

Conversation

jacobperron commented Nov 8, 2018 • edited Loading

jacobperron commented Nov 9, 2018

hidmic left a comment

Choose a reason for hiding this comment

hidmic commented Nov 12, 2018

jacobperron commented Nov 12, 2018 • edited Loading

jacobperron commented Nov 13, 2018

jacobperron commented Nov 13, 2018

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron Nov 14, 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

sloretz 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

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

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

hidmic Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

hidmic Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

jacobperron Nov 16, 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

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

hidmic commented Nov 15, 2018

jacobperron commented Nov 15, 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

jacobperron commented Nov 8, 2018 •

edited

Loading

jacobperron commented Nov 12, 2018 •

edited

Loading

jacobperron Nov 14, 2018 •

edited

Loading

jacobperron Nov 15, 2018 •

edited

Loading

jacobperron Nov 15, 2018 •

edited

Loading

hidmic Nov 16, 2018 •

edited

Loading

hidmic Nov 16, 2018 •

edited

Loading

jacobperron Nov 16, 2018 •

edited

Loading

sloretz left a comment •

edited

Loading