QoS duration APIs need INFINITE constant and an API contract to enforce proper usage by implementations

[emersonknapp]

Feature request

Feature description

Right now, when requesting endpoint info from rmw implementations, e.g. rmw_get_publishers_info_by_topic, we receive inconsistent results for publishers created with the same QoS settings. This makes it hard to create subscriptions adaptively when using queried QoS profiles. It also means that the reporting is noisy/lossy, we do not know what settings were really given to the endpoints.

• Define a set of constants in the RMW API to define "infinite duration"
• State the contract on the RMW APIs rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic that these infinity values will be reported by RMW implementations
• State the contract on subscription/publisher creation APIs that an input of Zero duration for QoS policies will be interpreted and reported as Infinity.

The DDS Spec's OMG IDL PSM (Platform Specific Model) defines the following (page 139)

struct Duration_t {
long sec;
unsigned long nanosec;
};
const long DURATION_INFINITE_SEC = 0x7fffffff;
const unsigned long DURATION_INFINITE_NSEC = 0x7fffffff;

const long DURATION_ZERO_SEC = 0;
const unsigned long DURATION_ZERO_NSEC = 0;

We should do something similar. However, since we use a uint64_t for both fields of rmw_time_t, I'd suggest the infinity value being "uint64_max" for both seconds and nanoseconds.

Work Log

• Introduce RMW_DURATION_INFINITE constant and API return value promise #301
• Take and return new RMW_DURATION_INFINITE correctly rmw_cyclonedds#288 Change rmw_cyclonedds to conform to API
• Change rmw_fastrtps to conform to API
• Change rmw_connext to conform to API

[mm318]

Given that RMW is supposed to be agnostic to DDS, it is a good idea to have RMW's own consistent definition of infinite duration.

I think an infinite duration should be defined as a rmw_time_t atomically, so if between the seconds and nanoseconds fields only one of them is UINT64_MAX, it still doesn't mean infinity. (Let's do also away with separate definitions of DURATION_INFINITE_SEC and DURATION_INFINITE_NSEC constants).

The RMW implementation layer does seem like the perfect place to convert between the DDS vendor specific values of infinity and the RMW value of infinity. Whether or not the DDS vendors are consistent between each other or with the OMG DDS spec shouldn't be too big of a concern when we have this RMW compatibility layer.

• State the contract on subscription/publisher creation APIs that an input of Zero duration for QoS policies will be interpreted and reported as Infinity.

I am uncomfortable with this contract. I think there are parts of the DDS spec that make the distinction between zero and non-zero duration, in which case zero and infinite duration would mean different things. In the future, non-DDS middleware layers could have specs that make a similar distinction.

[fujitatomoya]

I am uncomfortable with this contract.

so am I. and I believe we should not have redundant definition as infinity. This could be complicated.

[emersonknapp]

Yes, based on these conversations I am thinking to revise the proposal to something like the following:

"""
Define a constant RMW_TIME_INFINITY.

A Publisher offering a {0, 0} rmw_time_t for a Duration-based QoS policy is interpreted as "not specified" and must be reported as RMW_TIME_INFINITY in endpoint queries.

A Subscription requesting a {0, 0} rmw_time_t for a Duration-based QoS policy is interpreted as "not specified" and must reported as RMW_TIME_INFINITY in endpoint queries.
This request is compatible with all offers.
"""

[hidmic]

@emersonknapp are you considering to submit a design document or a draft PR that implements this proposal?

[emersonknapp]

I'd like to submit a design, but I'm focused on getting Foxy out right now - this has to wait for G-turtle anyways, I think.

[Barry-Xu-2018]

@emersonknapp

I'd like to submit a design, but I'm focused on getting Foxy out right now - this has to wait for G-turtle anyways, I think.

Do you finish your design ?

[emersonknapp]

Do you finish your design ?

I haven't started - Foxy isn'r released yet :) Don't wait on me though, if you would like to write the design, by all means go ahead. I can't make any promises for my personal timeline on this.

[Barry-Xu-2018]

@emersonknapp

I haven't started - Foxy isn'r released yet :) Don't wait on me though, if you would like to write the design, by all means go ahead. I can't make any promises for my personal timeline on this.

Got it :)
I only want to know your design.

[hidmic]

@emersonknapp have you had the time to put together a proposal?

[emersonknapp]

Thanks for the check-in/push, really just needed to put something on paper to start this out. Fell off our radar internally but it's still really important, I think. See #255 for an interface/design proposal in code form.

[hidmic]

Thanks for pushing! While we are at it, I think #215 should be taken care of too.

[emersonknapp]

You think I should bake that change into #255 ? I guess that makes sense as a single change "Refining the RMW time API"

[hidmic]

Perhaps. I just think this is a good opportunity to tackle that (apparent) redundancy, nevermind how.

[emersonknapp]

Copying from #255 to keep information central to the feature ticket:

I am actively working on this right now (for any interested parties). I've gathered more information to illustrate how the RMW API is currently incomplete.

The following values are gathered from get_publishers_info_by_topic when a single publisher is publishing with unspecified Deadline duration. It doesn't matter which RMW implementation is being used to publish, the value only depends on the implementation of the subscripton - I have tested it with only Cyclone and FastDDS, with all 4 pub/sub combinations. These same values are reported for Deadline, Lifespan, and Liveliness Lease Duration

Value Type	Fast sec	Fast nsec	Fast total nsec	Cyclone sec	Cyclone nsec	Cyclone total nsec
decimal	2147483647	4294967295	2147483651294967295	9223372036	854775807	9223372036854775807
hex	0x7FFFFFFF	0xFFFFFFFF	0x1DCD6500C46535FF	0x225C17D04	0x32F2D7FF	0x7FFFFFFFFFFFFFFF
DDS IDL PSM Constant	DURATION_INFINITE_SEC	TIME_INVALID_SEC	nothing	nothing	nothing	nothing
int limits	INT32_MAX	UINT32_MAX (or -1)	nothing	nothing	nothing	INT64_MAX

"DDS IDL PSM Constant" above refers to the values laid out in Section 2.3.3 of the DDS 1.4 spec. From my understanding this PSM is meant to provide common interfaces for programming languages. There are separate docs available as well:

• C language (Cyclone) - https://www.omg.org/spec/DDS/20140501/dds_dcps.idl
• C++ language (FastDDS) - https://www.omg.org/spec/DDS-PSM-Cxx/1.0/PDF
  The C++ one doesn't lay out new constants, from what I understand is it is meant to replace the IDL-generated API with more idiomatic language

Based on the above information, the two implementations are providing values that are neither consistent with each other or guidelines from the specification - seems like we just have exposed implementation details.

My next step is an updated API proposal, which will come with coupled prototype PRs for the RMW implementations.

Interesting note: the IDL PSM specifies Durations as:

struct Duration_t {
  long sec;
  unsigned long nanosec;
};

But we use exclusively unsigned values within the RMW API.