Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update service/client construction/destruction API return codes. #247

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Update service/client construction/destruction API return codes. #247

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 54 additions & 13 deletions rmw_cyclonedds_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3757,10 +3757,32 @@ static rmw_ret_t rmw_init_cs(
const char * service_name, const rmw_qos_profile_t * qos_policies,
bool is_service)
{
RET_NULL(node);
RET_WRONG_IMPLID(node);
RET_NULL_OR_EMPTYSTR(service_name);
RET_NULL(qos_policies);
RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
node,
node->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
RMW_CHECK_ARGUMENT_FOR_NULL(type_supports, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_ARGUMENT_FOR_NULL(service_name, RMW_RET_INVALID_ARGUMENT);
if (0 == strlen(service_name)) {
RMW_SET_ERROR_MSG("service_name argument is an empty string");
return RMW_RET_INVALID_ARGUMENT;
}
RMW_CHECK_ARGUMENT_FOR_NULL(qos_policies, RMW_RET_INVALID_ARGUMENT);
if (!qos_policies->avoid_ros_namespace_conventions) {
int validation_result = RMW_TOPIC_VALID;
rmw_ret_t ret = rmw_validate_full_topic_name(service_name, &validation_result, nullptr);
if (RMW_RET_OK != ret) {
return ret;
}
if (RMW_TOPIC_VALID != validation_result) {
const char * reason = rmw_full_topic_name_validation_result_string(validation_result);
RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("service_name argument is invalid: %s", reason);
return RMW_RET_INVALID_ARGUMENT;
}
}

const rosidl_service_type_support_t * type_support = get_service_typesupport(type_supports);
RET_NULL(type_support);

Expand Down Expand Up @@ -3823,10 +3845,13 @@ static rmw_ret_t rmw_init_cs(
RMW_SET_ERROR_MSG("failed to create topic");
goto fail_subtopic;
}
// before proceeding to outright ignore given QoS policies, sanity check them
dds_qos_t * qos;
if ((qos = dds_create_qos()) == nullptr) {
if ((qos = create_readwrite_qos(qos_policies, false)) == nullptr) {
goto fail_qos;
}
dds_reset_qos(qos);
Comment on lines +3848 to +3853
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand this, but then I also don't understand what the QoS policies are doing here in the first place:

  • best-effort, keep-last or a lifespan are fundamentally broken because the request or the response may then be dropped silently;
  • service invocations are (by definition) aperiodic data, so deadline doesn't apply, and, it would seem, neither does liveliness;
  • transient-local would be an interesting choice for dealing with the discovery latencies (and can safely be done with cyclone by leaving the "durability service history" at "keep-last 1", but the discovery latencies have been dealt with in another way already, and so doesn't make sense either.

Allowing avoid_ros_namespace_conventions might be useful, but I think one should really only use that for interfacing with existing non-ROS 2 systems. It seems unlikely that those systems would happen to implement the service invocation protocol.

Arguably that leaves only ignore_local_publications, which could be interpreted as a request to ignore services in the same node, or participant, or, indeed, process. But I'm not sure what the interpretation of it is supposed to be and it doesn't seem to be supported by all RMW implementations anyway. So I doubt it would be wise to interpret it.

And so I'd argue none of the QoS policies are meaningful, and if you want to check the validity, you should check whether they match the above, or are set to "system default" ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand this, but then I also don't understand what the QoS policies are doing here in the first place:

Yes, we should probably have separated service/topic qos profile definitions.
Service QoS are mostly unusable right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I've never seen service QoS policies being used in any meaningful way out there.

However, I won't argue here with the decisions made by each implementation. This change is simply ensuring rmw_cyclonedds_cpp does not accept invalid QoS profiles, as the API mandates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that this create_readwrite_qos/dds_reset_qos combo is the same than calling dds_create_qos.
If that's the case, lgtm.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, it is !


dds_qset_reliability(qos, DDS_RELIABILITY_RELIABLE, DDS_SECS(1));
dds_qset_history(qos, DDS_HISTORY_KEEP_ALL, DDS_LENGTH_UNLIMITED);

Expand Down Expand Up @@ -3891,10 +3916,18 @@ static void rmw_fini_cs(CddsCS * cs)

static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client)
{
RET_NULL(node);
RET_WRONG_IMPLID(node);
RET_NULL(client);
RET_WRONG_IMPLID(client);
RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
node,
node->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
RMW_CHECK_ARGUMENT_FOR_NULL(client, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
client,
client->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
auto info = static_cast<CddsClient *>(client->data);
clean_waitset_caches();

Expand Down Expand Up @@ -3987,10 +4020,18 @@ extern "C" rmw_ret_t rmw_destroy_client(rmw_node_t * node, rmw_client_t * client

static rmw_ret_t destroy_service(const rmw_node_t * node, rmw_service_t * service)
{
RET_NULL(node);
RET_WRONG_IMPLID(node);
RET_NULL(service);
RET_WRONG_IMPLID(service);
RMW_CHECK_ARGUMENT_FOR_NULL(node, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
node,
node->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
RMW_CHECK_ARGUMENT_FOR_NULL(service, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
service,
service->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
auto info = static_cast<CddsService *>(service->data);
clean_waitset_caches();

Expand Down