action messagess could live in rcl_interfaces since that's where parameter messages live already

RCL_ACTIONS_BUILDING_DLL

already called find_package(rcl REQUIRED) above

I'm a bit unfamiliar with the qos settings, but if I had to guess

For goal submision, getting a result, and cancelling a goal

{
  // Client will probably only have one response from the server to deal with at a time
  RMW_QOS_POLICY_HISTORY_KEEP_LAST,
  1,
  // It's really important client knows whether goal was accepted or rejected
  RMW_QOS_POLICY_RELIABILITY_RELIABLE,
  // TODO what affect does durability have on service calls?
  RMW_QOS_POLICY_DURABILITY_VOLATILE,
  // Use ros namespace conventions for actions by default
  // If a user does `true` here the action client probably shouldn't begin the name with `/_action/` either
  false
};

For status and feedback messages client side

{
  RMW_QOS_POLICY_HISTORY_KEEP_LAST,
  // Store a few samples in case client is processing them slowly
  10,
  // Default QoS seems to be reliaible
  RMW_QOS_POLICY_RELIABILITY_RELIABLE,
  RMW_QOS_POLICY_DURABILITY_VOLATILE,
  false
};

Might add a note that only the UUID needs to be populated, the time accepted won't be known until the service sends a response.

I would expect goal_handle to have a reference to action_server already. That avoids accidentally passing the wrong action server in.

Does rcl_action_update_goal_state get called before or after rcl_action_send_result_response?

That's a good question. I was imagining that the implementation of rcl_action_send_result_response could call this function so the user doesn't have to think about it (ie. before the client receives the message). The same goes for processing cancel request; rcl_action_send_cancel_response or some utility function will handle calling rcl_action_update_goal_state accordingly. I should update the documentation to be explicit about this behavior (and remove some const qualifiers).

I thought it would be a nice way to enforce state changes on service 'events', but perhaps bundling state changes with the service functions adds too much complexity.

Where is the original request message used later?

I thought this might be convenient for a client library. I was mimicking the API from ROS 1: https://github.com/ros/actionlib/blob/d317e63ebabc6c437f8b9f73ece06acf348700ea/include/actionlib/server/server_goal_handle.h#L137

I'm removing this to simplify implementation since this is the only place where typesupport for goal handles comes into play and it does not seem necessary.

I would put invalid at 0, some compilers will zero initialize if building without optimizations, which could help catch bugs during development

@jacobperron rcl_aciton_take_cancel_request() -> rcl_action_take_cancel_request()

@jacobperron so, if I follow the logic correctly, moments after a successful rcl_action_take_goal_request() call, rcl_action caller must populate an rcl_action_goal_info_t instance appropriately to let the server know of the goal. Is that right? If so, it'd be nice to be explicit about it.

Yes, this is correct. I will makes sure this is documented. And add an init/fini for the rcl_action_goal_info_t

@jacobperron same as above, if I'm right about these base action types then I think it'd be nice to have it documented.

@jacobperron would it make sense to expose cancelling operations on a per goal handle basis too and build rcl_action_process_cancel_request() on top of them instead?

The goal handle's state can be updated with rcl_action_update_goal_state(), which I plan to call from this function (i.e. set the goal handles state to CANCELING).

@jacobperron as @sloretz suggested for rcl_action_take_feedback() below, may be useful to expose rmw_request_id_t if multiple goal requests are sent one after the other as in:

Perhaps, I was assuming that the goal ID would be present in the ROS message (ros_goal_request). The client could use the goal ID to distinguish between multiple responses.

@jacobperron same about exposing rmw_request_id_t

Assuming we have a goal ID as part of the ROS request message, I'm not sure that we need to expose a rmw_request_id_t.

nit rcl_action_wait_set_add_action_server since the other API's start with the package name

Every time a client library spin() is called it calls rcl_wait_set_init(ws, num_subs, num_gcs, num_timers, num_clients, num_services) to allocate or reallocate array's in the wait set. Afterwards calls rcl_wait_set_add_*() set values in the arrays. rcl_wait_set_add_action_server() is the second part, but how does the client library know how many services/subscribers/timers/guard conditions etc will be added to the wait set?

Also, how does the client library know which services/subscribers/... should result in which call to the action server API? If a service belonging to an action server is ready how does the client library know to call rcl_action_server_take_cancel_request or rcl_action_server_take_goal_request? It would be convenient if the client library could notify the action server that an entity is ready without knowing what the action server was going to do with it.

A method like ..._action_server_get_num_entities(as, size_t *num_subscribers, size_t *num_timers, ...) which returns how many entities belong to the action server would help the client library count how many entities it needs to resize the wait set only once. Another group of methods like ..._action_server_notify_subscription_ready(as, rcl_subscription_t * sub) to notify the action server when an entity becomes ready would allow the client library to be more ignorant of the server implementation.

// Client library populates these with the free subs, servers, clients, etc
size_t num_subscriptions = 0u;
size_t num_servers = 0u;
size_t num_clients = 0u;
size_t num_timers = 0u;
size_t num_guard_conditions = 0u;

// For each action server..
{
  // rcl_action_server_t as;
  size_t as_num_subscriptions = 0u;
  size_t as_num_servers = 0u;
  size_t as_num_clients = 0u;
  size_t as_num_timers = 0u;
  size_t as_num_guard_conditions = 0u;

  size_t as_start_subscriptions = 0u;
  size_t as_start_servers = 0u;
  size_t as_start_clients = 0u;
  size_t as_start_timers = 0u;
  size_t as_start_guard_conditions = 0u;

  if (RCL_OK == rcl_action_server_get_num_entities(&as, &as_num_subscriptions, &as_num_guard_conditions, &as_num_timers, &as_num_clients, &as_num_servers)
  {
  // Indices in the wait set where this action server starts, client library needs to store this info for each action server
    as_start_subscriptions = num_subscriptions;
    as_start_servers = num_servers;
    as_start_clients = num_clients;
    as_start_timers = num_timers;
    as_start_guard_conditions = num_guard_conditions;

    // Add action server entities to the total
    num_subscriptions += as_num_subscriptions;
    num_servers += as_num_servers;
    num_clients += as_num_clients;
    num_timers += as_num_timers;
    num_guard_conditions += as_num_guard_conditions;
  }
}

// Client library can now resize the wait set
ret = rcl_wait_set_init(&ws, num_subscriptions, num_guard_conditions, num_timers, num_clients, num_services, allocator);
// error handling

ret = rcl_wait_t(ws, timeout);
// error handling

// Client library iterates through the wait set
for (size_t i = 0; i < num_subscriptions; ++i)
{
  // For each action server ...
  {
    if (i >= as_start_subscriptions && i < as_start_subscriptions + as_num_subscriptions; ++i)
    {
      // Action server subscription became ready, tell the server to do something with it
      rcl_action_notify_subscription_ready(&as, ws.subscriptions[i]);
    }
    // ... repeat for remaining entities
  }
}

That's a pretty verbose example, but it gives the client library enough info to do what it needs to. Maybe a struct or two to manage groups of waitable entities that need to be added to a wait set could shorten it?

Idea for waitset groups in #308

Yeah, I can see your point about providing a method for getting the number of services and subscriptions from an action server or action client. It's better than clients assuming the number.

Regarding "which services/subscribers?", I'm not sure how the client libraries will look when it comes to actions and wait sets (right now it's not clear to me without digging further), but I'll add some notify functions as you suggest. It could be the case that we come back and refactor to help with client library implementation.

I would suggest putting the zero initialization functions in the same file as the finalization functions. Something that zero initializes a struct is likely going to need to finalize it later.

@jacobperron Your name / email should go here.