Skip liveliness parts for qos elements with default values #266
Conversation
| } catch (const std::exception & e) { | ||
| RMW_SET_ERROR_MSG_WITH_FORMAT_STRING("Error setting QoS values from strings: %s", e.what()); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| // Helper function to convert string to size_t. | ||
| auto str_to_size_t = | ||
| [](const std::string & str) -> std::optional<size_t> | ||
| [](const std::string & str, const std::size_t default_value) -> std::optional<size_t> |
There was a problem hiding this comment.
I'm concerned about the size_t here. In particular, it is not always the case that sizeof(size_t) == sizeof(uint64_t). Yet the deadline, lifespan, and liveliness changes down below assume that it is the case (for the sec and nsec).
What I'm going to suggest instead is that we make this a "real" function, and templatize default_value. Then all of those will get code generated properly.
Something like:
template<typename T>
static std::optional<T> str_to_size_t(const std::string & str, const T default_value)
{
if (str.empty()) {
return default_value;
}
errno = 0;
char * endptr;
size_t num = strtoul(str.c_str(), &endptr, 10);
if (endptr == str.c_str()) {
// No values were converted, this is an error
RMW_SET_ERROR_MSG("no valid numbers available");
return std::nullopt;
} else if (*endptr != '\0') {
// There was junk after the number
RMW_SET_ERROR_MSG("non-numeric values");
return std::nullopt;
} else if (errno != 0) {
// Some other error occurred, which may include overflow or underflow
RMW_SET_ERROR_MSG(
"an undefined error occurred while getting the number, this may be an overflow");
return std::nullopt;
}
return num;
}
There was a problem hiding this comment.
Great suggestion; I've modified accordingly aa5697d
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Yadunund
force-pushed
the
yadu/minimal-qos-in-liveliness
branch
from
aa5697d to
db537a8
Compare
| template<typename T> | ||
| std::optional<T> str_to_size_t(const std::string & str, const T default_value) |
There was a problem hiding this comment.
This whole function doesn't need to live in the header file; we can just keep it as an anonymous namespace function within liveliness_utils.cpp.
| } | ||
| errno = 0; | ||
| char * endptr; | ||
| size_t num = strtoul(str.c_str(), &endptr, 10); |
There was a problem hiding this comment.
Thinking about it more today, I realized two problems here:
strtoulis documented as returning anunsigned long, not asize_t. This is an existing bug.- If T ended up being a signed integer, we'd likely do the incorrect thing here. We could fix this either by making the template type only support unsigned (any signed integers would fail to compile), or we could do some other template magic to choose
strtoulorstrtolas appropriate.
I would say neither of those are blockers, but if you decide not to fix them I'd appreciate a TODO comment explaining what should be improved here.
There was a problem hiding this comment.
Thanks for the feedback! I've left 1) as a TODO but addressed 2) with an std::is_signed check. See bd3722f
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
| #include "logging_macros.hpp" | ||
|
|
||
| #include "rmw/error_handling.h" |
There was a problem hiding this comment.
Both of these can be moved back into liveliness_utils.cpp.
Co-authored-by: Chris Lalancette <[email protected]> Signed-off-by: Yadu <[email protected]>
Signed-off-by: Yadunund <[email protected]>
This PR is a follow-up to #234.
Instead of convering the qos value to a string and appending to the liveliness token's keyexpression, we only do so when the qos value deviates from the
QoS::default_qos(). This reduces the number of bytes we need to pack into the liveliness tokens.Prior to this PR, the publisher that spins when running
ros2 topic pub /chatter std_msgs/msg/String '{data: hello}' --qos-reliability reliable --qos-durability transient_local --qos-depth 42 --qos-history keep_last, the liveliness token would look likeNow the token looks like
The diff better highlights the differences
I've tested this PR against the
rcltests and no new errors.