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

Deprecate rmw_time_t #298

Closed
wants to merge 2 commits into from
Closed

Deprecate rmw_time_t #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdb45ea
Deprecate rmw_time_t
Feb 10, 2021
4a1ed5b
Address review comments
Feb 17, 2021
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
4 changes: 2 additions & 2 deletions rmw/include/rmw/rmw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2414,7 +2414,7 @@ rmw_destroy_wait_set(rmw_wait_set_t * wait_set);
* \param[inout] events Array of events to wait on.
* Can be `NULL` if there are no events to wait on.
* \param[in] wait_set Wait set to use for waiting.
* \param[in] wait_timeout If `NULL`, block indefinitely until an entity becomes ready.
* \param[in] wait_timeout If negative, block indefinitely until an entity becomes ready.
* If zero, do not block -- check only for immediately available entities.
* Else, this represents the maximum amount of time to wait for an entity to become ready.
* \return `RMW_RET_OK` if successful, or
Expand All @@ -2435,7 +2435,7 @@ rmw_wait(
rmw_clients_t * clients,
rmw_events_t * events,
rmw_wait_set_t * wait_set,
const rmw_time_t * wait_timeout);
rmw_duration_t wait_timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

The thing I'm concerned about here is that this is an API change at the RMW layer. While we can fix all of the in-tree rmws, this still is a hard break for all of the out-of-tree ones. I'd be much happier with this change if we deprecated the old signature, added a new signature, and had a shim to convert between them. Then we can remove the old signature for H-Turtle. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what you're suggesting?

Galactic

rmw_wait(const rmw_time_t * wait_timeout) DEPRECATED { 
    rmw_wait2(time_to_duration(wait_timeout));
}

rmw_wait2(rmw_duration_t duration);

H-Turtle

rmw_wait(rmw_duration_t duration);

rmw_wait2(rmw_duration_t duration) DEPRECATED {
    rmw_wait(duration);
}

I-Turtle

rmw_wait(rmw_duration_t duration);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or more along the lines of just introducing a new name entirely (can't think what it should be called... rmw_wait_on_set?), then just deprecate rmw_wait forever in I-turtle.

Or, do you think this change is not warranted and that we should maintain the rmw_time_t type?

Copy link
Contributor Author

@emersonknapp emersonknapp Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh - another note: changing rmw_qos_profile_t to use rmw_duration_t instead of rmw_time_t is also is a breaking change for anything interacting with them directly - e.g. in https://github.com/ros2/rmw_cyclonedds/pull/283/files the .sec and .nsec usages have to be fixed. How would we handle this?

Copy link
Contributor

@clalancette clalancette Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Or, do you think this change is not warranted and that we should maintain the rmw_time_t type?

That's hard for me to say. If I take a look at how the const rmw_time_t * wait_timeout parameter to rmw_wait is used by the various RMWs, it looks like this:

  • Fast-DDS - maps it to a Duration_t, which uses an int32_t/uint32_t to represent the same data. There's clearly an impedance mis-match there (and the source of the clamp_rmw_time_to_dds_time method in rmw_dds_common).
  • CycloneDDS - maps it to an internal dds_time_t. There is some possibility of impedance mismatch there (since in theory you could have very large numbers in both sec and nsec, and hence overflow the int64_t that dds_time_t is), but in real life that would be a pathological case. (Incidentally, we should probably check for that pathological case before doing the conversion at https://github.com/ros2/rmw_cyclonedds/blob/d024823043504ea40af24bf22365a21cd203df55/rmw_cyclonedds_cpp/src/rmw_node.cpp#L3384 , but that is orthogonal to this) (Also, it is kind of weird that rmw_cyclonedds maps this to a dds_time_t and not a dds_duration_t. Practically it doesn't matter since they are both int64_t, but that seems more natural).
  • Connext - maps it to an internal DDS::Duration_t. Internally this seems to be an int32/uint32 sec/nsec in Connext: https://community.rti.com/rti-doc/500/ndds.5.0.0/doc/html/api_cpp/structDDS__Duration__t.html
  • (extra credit) eCal maps it to std::duration here . Implicitly, the way that they are using it as waiting on a condition variable seems to be using it as an int64.
  • (extra credit) iceoryx maps it to a struct timespec here.
  • (extra credit) dps maps it to a std::duration here
  • (extra credit) gurumdds maps it to an internal dds_Duration_t. That uses an int32/uint32 to represent sec/nsec, much like Fast-DDS.

So, in short, all of the RMWs do it differently. No matter what we do here, we're going to have some impedance mismatch with one of them. From that perspective, rmw_duration_t doesn't seem substantially better than rmw_time_t, just different.

However, it may make sense to look at it from the point of view of the users of the RMW. Do you think it makes more sense for a caller of rmw_wait to use an rmw_duration_t instead of an rmw_time_t? If so, why?

Or more along the lines of just introducing a new name entirely (can't think what it should be called... rmw_wait_on_set?), then just deprecate rmw_wait forever in I-turtle.

Even simpler:

For Galactic:

  • add in rmw_wait_on_set(rmw_duration_t duration);
  • mark rmw_wait(rmw_time_t *) deprecated
  • Update all internal users to use rmw_wait_on_set

For H-Turtle:

  • Remove rmw_wait(rmw_time_t *)

Oh - another note: changing rmw_qos_profile_t to use rmw_duration_t instead of rmw_time_t is also is a breaking change for anything interacting with them directly - e.g. in https://github.com/ros2/rmw_cyclonedds/pull/283/files the .sec and .nsec usages have to be fixed. How would we handle this?

Ug. In an ideal world, we would add another structure and do the deprecation dance there as well, but that is starting to get a bit ridiculous. I'm curious to hear your thoughts above before we discuss the finer point here.

Copy link
Contributor Author

@emersonknapp emersonknapp Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, at the end of the day, my real goal is to solve #210, which is an actual behavioral problem that is not handled in RMW. Based on this conversation, I'm inclined to lean back on an implementation that doesn't require changing rmw_time_t to achieve the goal.

Edit: my inclination especially is based on the fact that there's no across-the-board benefit for RMW implementations.
The original reason to change this was to make time representation more uniform across the ROS 2 core, since rcl, rclcpp, rclpy all represent times/durations as a single int64 nanoseconds. Perhaps we can gain the necessary benefit with just a few convenience functions/macros for converting between

Copy link
Contributor

Choose a reason for hiding this comment

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

Edit: my inclination especially is based on the fact that there's no across-the-board benefit for RMW implementations.
The original reason to change this was to make time representation more uniform across the ROS 2 core, since rcl, rclcpp, rclpy all represent times/durations as a single int64 nanoseconds. Perhaps we can gain the necessary benefit with just a few convenience functions/macros for converting between

I see. So that would argue for making the switch to int64. Making that more consistent does seem worthwhile, but I also think that it shouldn't block your other work on #210. So maybe the thing to do here is to put all of this on hold, concentrate on #210, and return to this after Galactic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#301


/// Return the name and namespace of all nodes in the ROS graph.
/**
Expand Down
30 changes: 18 additions & 12 deletions rmw/include/rmw/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ extern "C"
// implementation. It may need to be increased in the future.
#define RMW_GID_STORAGE_SIZE 24u

#ifndef _WIN32
# define RMW_DECLARE_DEPRECATED(name, msg) name __attribute__((deprecated(msg)))
#else
# define RMW_DECLARE_DEPRECATED(name, msg) name __pragma(deprecated(name))
#endif

/// Structure which encapsulates an rmw node
typedef struct RMW_PUBLIC_TYPE rmw_node_t
{
Expand Down Expand Up @@ -326,7 +332,12 @@ typedef struct RMW_PUBLIC_TYPE rmw_time_t

/// Nanoseconds component of this time point
uint64_t nsec;
} rmw_time_t;
} RMW_DECLARE_DEPRECATED (rmw_time_t,
"rmw_time_t has been deprecated in Galactic in favor of rmw_duration_t. "
"It will be removed in H-Turtle");

/// A duration of time, measured in nanoseconds.
typedef rcutils_duration_value_t rmw_duration_t;

typedef rcutils_time_point_value_t rmw_time_point_value_t;

Expand Down Expand Up @@ -389,11 +400,6 @@ enum RMW_PUBLIC_TYPE rmw_qos_durability_policy_t
"RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_NODE is deprecated. " \
"Use RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC if manually asserted liveliness is needed."

#ifndef _WIN32
# define RMW_DECLARE_DEPRECATED(name, msg) name __attribute__((deprecated(msg)))
#else
# define RMW_DECLARE_DEPRECATED(name, msg) name __pragma(deprecated(name))
#endif

/// QoS liveliness enumerations that describe a publisher's reporting policy for its alive status.
/// For a subscriber, these are its requirements for its topic's publishers.
Expand Down Expand Up @@ -425,13 +431,13 @@ enum RMW_PUBLIC_TYPE rmw_qos_liveliness_policy_t
};

/// QoS Deadline default, 0s indicates deadline policies are not tracked or enforced
#define RMW_QOS_DEADLINE_DEFAULT {0, 0}
#define RMW_QOS_DEADLINE_DEFAULT 0

/// QoS Lifespan default, 0s indicate lifespan policies are not tracked or enforced
#define RMW_QOS_LIFESPAN_DEFAULT {0, 0}
#define RMW_QOS_LIFESPAN_DEFAULT 0

/// QoS Liveliness lease duration default, 0s indicate lease durations are not tracked or enforced
#define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT {0, 0}
#define RMW_QOS_LIVELINESS_LEASE_DURATION_DEFAULT 0

/// ROS MiddleWare quality of service profile.
typedef struct RMW_PUBLIC_TYPE rmw_qos_profile_t
Expand All @@ -444,13 +450,13 @@ typedef struct RMW_PUBLIC_TYPE rmw_qos_profile_t
/// Durability QoS policy setting
enum rmw_qos_durability_policy_t durability;
/// The period at which messages are expected to be sent/received
struct rmw_time_t deadline;
rmw_duration_t deadline;
/// The age at which messages are considered expired and no longer valid
struct rmw_time_t lifespan;
rmw_duration_t lifespan;
/// Liveliness QoS policy setting
enum rmw_qos_liveliness_policy_t liveliness;
/// The time within which the RMW node or publisher must show that it is alive
struct rmw_time_t liveliness_lease_duration;
rmw_duration_t liveliness_lease_duration;

/// If true, any ROS specific namespacing conventions will be circumvented.
/**
Expand Down
51 changes: 19 additions & 32 deletions rmw/test/test_topic_endpoint_info.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "gmock/gmock.h"
#include "osrf_testing_tools_cpp/scope_exit.hpp"
#include "rcutils/allocator.h"
#include "rcutils/time.h"

#include "rmw/error_handling.h"
#include "rmw/topic_endpoint_info.h"
Expand Down Expand Up @@ -146,10 +147,10 @@ TEST(test_topic_endpoint_info, set_qos_profile) {
qos_profile.depth = 0;
qos_profile.reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE;
qos_profile.durability = RMW_QOS_POLICY_DURABILITY_VOLATILE;
qos_profile.deadline = {1, 0};
qos_profile.lifespan = {2, 0};
qos_profile.deadline = RCUTILS_S_TO_NS(1);
emersonknapp marked this conversation as resolved.
Show resolved Hide resolved
qos_profile.lifespan = RCUTILS_S_TO_NS(2);
qos_profile.liveliness = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC;
qos_profile.liveliness_lease_duration = {3, 0};
qos_profile.liveliness_lease_duration = RCUTILS_S_TO_NS(3);
qos_profile.avoid_ros_namespace_conventions = false;

rmw_ret_t ret = rmw_topic_endpoint_info_set_qos_profile(nullptr, &qos_profile);
Expand All @@ -173,19 +174,14 @@ TEST(test_topic_endpoint_info, set_qos_profile) {
EXPECT_EQ(
topic_endpoint_info.qos_profile.durability,
RMW_QOS_POLICY_DURABILITY_VOLATILE) << "Unequal durability";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.sec, 1u) << "Unequal deadline sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.nsec, 0u) << "Unequal deadline nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.sec, 2u) << "Unequal lifespan sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.nsec, 0u) << "Unequal lifespan nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline, RCUTILS_S_TO_NS(1LL)) << "Unequal deadline";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan, RCUTILS_S_TO_NS(2LL)) << "Unequal lifespan";
EXPECT_EQ(
topic_endpoint_info.qos_profile.liveliness,
RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC) << "Unequal liveliness";
EXPECT_EQ(
topic_endpoint_info.qos_profile.liveliness_lease_duration.sec,
3u) << "Unequal liveliness lease duration sec";
EXPECT_EQ(
topic_endpoint_info.qos_profile.liveliness_lease_duration.nsec,
0u) << "Unequal liveliness lease duration nsec";
topic_endpoint_info.qos_profile.liveliness_lease_duration, RCUTILS_S_TO_NS(3LL)
) << "Unequal liveliness lease duration";
EXPECT_EQ(
topic_endpoint_info.qos_profile.avoid_ros_namespace_conventions,
false) << "Unequal avoid namespace conventions";
Expand All @@ -205,17 +201,12 @@ TEST(test_topic_endpoint_info, zero_init) {
EXPECT_EQ(topic_endpoint_info.qos_profile.depth, 0u) << "Non-zero depth";
EXPECT_EQ(topic_endpoint_info.qos_profile.reliability, 0) << "Non-zero reliability";
EXPECT_EQ(topic_endpoint_info.qos_profile.durability, 0) << "Non-zero durability";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.sec, 0u) << "Non-zero deadline sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.nsec, 0u) << "Non-zero deadline nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.sec, 0u) << "Non-zero lifespan sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.nsec, 0u) << "Non-zero lifespan nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline, 0) << "Non-zero deadline";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan, 0) << "Non-zero lifespan";
EXPECT_EQ(topic_endpoint_info.qos_profile.liveliness, 0) << "Non-zero liveliness";
EXPECT_EQ(
topic_endpoint_info.qos_profile.liveliness_lease_duration.sec,
0u) << "Non-zero liveliness lease duration sec";
EXPECT_EQ(
topic_endpoint_info.qos_profile.liveliness_lease_duration.nsec,
0u) << "Non-zero liveliness lease duration nsec";
topic_endpoint_info.qos_profile.liveliness_lease_duration,
0) << "Non-zero liveliness lease duration";
EXPECT_EQ(
topic_endpoint_info.qos_profile.avoid_ros_namespace_conventions,
false) << "Non-zero avoid namespace conventions";
Expand All @@ -229,10 +220,10 @@ TEST(test_topic_endpoint_info, fini) {
qos_profile.depth = 0;
qos_profile.reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE;
qos_profile.durability = RMW_QOS_POLICY_DURABILITY_VOLATILE;
qos_profile.deadline = {1, 0};
qos_profile.lifespan = {2, 0};
qos_profile.deadline = RCUTILS_S_TO_NS(1);
qos_profile.lifespan = RCUTILS_S_TO_NS(2);
qos_profile.liveliness = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC;
qos_profile.liveliness_lease_duration = {3, 0};
qos_profile.liveliness_lease_duration = RCUTILS_S_TO_NS(3);
qos_profile.avoid_ros_namespace_conventions = false;
rmw_ret_t ret = rmw_topic_endpoint_info_set_qos_profile(&topic_endpoint_info, &qos_profile);
EXPECT_EQ(ret, RMW_RET_OK) << "Expected OK for valid arguments";
Expand Down Expand Up @@ -277,15 +268,11 @@ TEST(test_topic_endpoint_info, fini) {
EXPECT_EQ(topic_endpoint_info.qos_profile.depth, 0u) << "Non-zero depth";
EXPECT_EQ(topic_endpoint_info.qos_profile.reliability, 0) << "Non-zero reliability";
EXPECT_EQ(topic_endpoint_info.qos_profile.durability, 0) << "Non-zero durability";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.sec, 0u) << "Non-zero deadline sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline.nsec, 0u) << "Non-zero deadline nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.sec, 0u) << "Non-zero lifespan sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan.nsec, 0u) << "Non-zero lifespan nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.deadline, 0u) << "Non-zero deadline";
EXPECT_EQ(topic_endpoint_info.qos_profile.lifespan, 0u) << "Non-zero lifespan";
EXPECT_EQ(topic_endpoint_info.qos_profile.liveliness, 0) << "Non-zero liveliness";
EXPECT_EQ(topic_endpoint_info.qos_profile.liveliness_lease_duration.sec, 0u) <<
"Non-zero liveliness lease duration sec";
EXPECT_EQ(topic_endpoint_info.qos_profile.liveliness_lease_duration.nsec, 0u) <<
"Non-zero liveliness lease duration nsec";
EXPECT_EQ(topic_endpoint_info.qos_profile.liveliness_lease_duration, 0u) <<
"Non-zero liveliness lease duration";
EXPECT_EQ(topic_endpoint_info.qos_profile.avoid_ros_namespace_conventions, false) <<
"Non-zero avoid namespace conventions";
}