Update service/client construction/destruction API return codes. #247
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI up to
|
| // 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); |
There was a problem hiding this comment.
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" ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
CI up to
|
|
@eboasson @ivanpauno anything else? |
Yeah, it is odd. I don't want to start a discussion here and now, but this is the only Tier 1 implementation that chose to ignore QoS settings when the API suggests (and now explicitly says) otherwise. Perhaps we owe ourselves a discussion about this, but right now we need a (seemingly) consistent API. |
|
Thanks for the review @ivanpauno ! |
delete-merged-branch
bot
deleted the
hidmic/compliant-service-n-client-creation
branch
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#276. Precisely what the title says.