rcl_send_response can return a RCL_RET_CLIENT_INVALID?

[firesurfer]

According to the documentation the rcl_send_response function can return a RCL_RET_CLIENT_INVALID.

 * \return RCL_RET_OK if the response was sent successfully, or
 *         RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
 *         RCL_RET_CLIENT_INVALID if the service is invalid, or
 *         RCL_RET_ERROR if an unspecified error occurs.
 */
RCL_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t
rcl_send_response(
  const rcl_service_t * service,
  rmw_request_id_t * response_header,
  void * ros_response);

Shouldn't this be a RCL_RET_SERVICE_INVALID ?

[dirk-thomas]

Yes, it looks like a copy-n-paste error. Can you please create a PR with the proposed change.

But it seems that the current implementations never return this code anyway? @wjwwood

[wjwwood]

Definitely a copy-paste bug. That return code is reserved for checking if the service is valid or not. In the publisher code, an equivalent return code is returned if the argument is null, but perhaps that's not the best idea, and should instead return RCL_RET_INVALID_ARGUMENT as the implementation of this function does. When I think of an "invalid service" I mean:

• rcl_shutdown has been called
• the associated node is invalid

Currently the node can only be invalid if rcl_shutdown has been called, which is a bit redundant. However, I wanted to build in the notion that a node could have a valid/invalid state separate from the init/shutdown state.

A more general issue is that we need to check these states in all functions, and provide rcl_X_is_valid() functions for each type of thing.

[firesurfer]

@dirk-thomas Then I'm going to create a PR that corrects the documentation from RCL_RET_CLIENT_INVALID
to RCL_RET_SERVICE_INVALID ?

@wjwwood
I would expect this error in case the passed service is invalid (is this even possible to determine?). In case the node is invalid or rcl_shutdown has been called I would return the corresponding value. E.g in case the node is invalid I would expect the function to return RCL_RET_NODE_INVALID

By a rcl_X_is_valid() function you mean for example a function that checks if a given node is valid: bool rcl_node_is_valid(rcl_node_t * node) ?

[wjwwood]

I would expect this error in case the passed service is invalid (is this even possible to determine?). In case the node is invalid or rcl_shutdown has been called I would return the corresponding value. E.g in case the node is invalid I would expect the function to return RCL_RET_NODE_INVALID

You're right that the node being invalid might be the first concern, but since this this function doesn't take a node as an argument (it is probably held internally by the service) then I think the RCL_RET_SERVICE_INVALID is the right return code here. Currently, there is no additional state for the service object that needs to be checked for it to be valid, other than the node, but that might change in the future. This return code is a placeholder more than anything. You could imagine in the future a function like rcl_service_shutdown() which "invalidates" a given service without destroying the object like rcl_service_fini() does.

By a rcl_X_is_valid() function you mean for example a function that checks if a given node is valid: bool rcl_node_is_valid(rcl_node_t * node)?

That's correct, and by extension bool rcl_service_is_valid(rcl_service_t * service).

[firesurfer]

I created a PR #80 that simply removes the documentation entry. I didn't add the RCL_RET_SERVICE_INVALID return type because I think there's currently no possible check in the function where this return value might be reasonable.

But I would recommend to keep this issue open because of the things wjwwood said in his last post.

[wjwwood]

@firesurfer actually can you open a new issue for whatever you think needs to be tracked. From my perspective, this can be closed once #80 replaces CLIENT with SERVICE in the documentation, but if you see other issues we can track/discuss those too.