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

Skip liveliness parts for qos elements with default values #266

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Skip liveliness parts for qos elements with default values #266

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89403d0
Skip liveliness parts for qos elements with default values
Yadunund Aug 22, 2024
db537a8
Make str_to_size_t a template function
Yadunund Aug 26, 2024
bd3722f
Move str_to_size_t back to impl and address feedback on strtoul
Yadunund Aug 27, 2024
8159b0b
Alphabetize include
Yadunund Aug 27, 2024
f2ee15e
Update rmw_zenoh_cpp/src/detail/liveliness_utils.cpp
Yadunund Aug 27, 2024
dd44983
Move headers back
Yadunund Aug 27, 2024
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
5 changes: 3 additions & 2 deletions rmw_zenoh_cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,11 @@ install(
)

add_executable(rmw_zenohd
src/zenohd/main.cpp
src/detail/zenoh_config.cpp
src/detail/liveliness_utils.cpp
src/detail/logging.cpp
src/detail/qos.cpp
src/detail/zenoh_config.cpp
src/zenohd/main.cpp
)

target_link_libraries(rmw_zenohd
Expand Down
114 changes: 63 additions & 51 deletions rmw_zenoh_cpp/src/detail/liveliness_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@
#include <utility>
#include <vector>

#include "logging_macros.hpp"
#include "qos.hpp"

#include "rcpputils/scope_exit.hpp"

#include "rmw/error_handling.h"

namespace rmw_zenoh_cpp
{
namespace liveliness
Expand Down Expand Up @@ -171,44 +169,75 @@ std::vector<std::string> split_keyexpr(

///=============================================================================
// TODO(Yadunund): Rely on maps to retrieve strings.
std::string qos_to_keyexpr(rmw_qos_profile_t qos)
std::string qos_to_keyexpr(const rmw_qos_profile_t & qos)
{
std::string keyexpr = "";
const rmw_qos_profile_t & default_qos = QoS::get().default_qos();

// Reliability.
keyexpr += std::to_string(qos.reliability);
if (qos.reliability != default_qos.reliability) {
keyexpr += std::to_string(qos.reliability);
}
keyexpr += QOS_DELIMITER;

// Durability.
keyexpr += std::to_string(qos.durability);
if (qos.durability != default_qos.durability) {
keyexpr += std::to_string(qos.durability);
}
keyexpr += QOS_DELIMITER;

// History.
keyexpr += std::to_string(qos.history);
if (qos.history != default_qos.history) {
keyexpr += std::to_string(qos.history);
}
keyexpr += QOS_COMPONENT_DELIMITER;
keyexpr += std::to_string(qos.depth);
if (qos.depth != default_qos.depth) {
keyexpr += std::to_string(qos.depth);
}
keyexpr += QOS_DELIMITER;

// Deadline.
keyexpr += std::to_string(qos.deadline.sec);
if (qos.deadline.sec != default_qos.deadline.sec) {
keyexpr += std::to_string(qos.deadline.sec);
}
keyexpr += QOS_COMPONENT_DELIMITER;
keyexpr += std::to_string(qos.deadline.nsec);
if (qos.deadline.nsec != default_qos.deadline.nsec) {
keyexpr += std::to_string(qos.deadline.nsec);
}
keyexpr += QOS_DELIMITER;

// Lifespan.
keyexpr += std::to_string(qos.lifespan.sec);
if (qos.lifespan.sec != default_qos.lifespan.sec) {
keyexpr += std::to_string(qos.lifespan.sec);
}
keyexpr += QOS_COMPONENT_DELIMITER;
keyexpr += std::to_string(qos.lifespan.nsec);
if (qos.lifespan.nsec != default_qos.lifespan.nsec) {
keyexpr += std::to_string(qos.lifespan.nsec);
}
keyexpr += QOS_DELIMITER;

// Liveliness.
keyexpr += std::to_string(qos.liveliness);
if (qos.liveliness != default_qos.liveliness) {
keyexpr += std::to_string(qos.liveliness);
}
keyexpr += QOS_COMPONENT_DELIMITER;
keyexpr += std::to_string(qos.liveliness_lease_duration.sec);
if (qos.liveliness_lease_duration.sec != default_qos.liveliness_lease_duration.sec) {
keyexpr += std::to_string(qos.liveliness_lease_duration.sec);
}
keyexpr += QOS_COMPONENT_DELIMITER;
keyexpr += std::to_string(qos.liveliness_lease_duration.nsec);
if (qos.liveliness_lease_duration.nsec != default_qos.liveliness_lease_duration.nsec) {
keyexpr += std::to_string(qos.liveliness_lease_duration.nsec);
}

return keyexpr;
}

///=============================================================================
std::optional<rmw_qos_profile_t> keyexpr_to_qos(const std::string & keyexpr)
{
const rmw_qos_profile_t & default_qos = QoS::get().default_qos();
rmw_qos_profile_t qos;

const std::vector<std::string> parts = split_keyexpr(keyexpr, QOS_DELIMITER);
if (parts.size() < 6) {
return std::nullopt;
Expand All @@ -232,46 +261,29 @@ std::optional<rmw_qos_profile_t> keyexpr_to_qos(const std::string & keyexpr)
}

try {
qos.history = str_to_qos_history.at(history_parts[0]);
qos.reliability = str_to_qos_reliability.at(parts[0]);
qos.durability = str_to_qos_durability.at(parts[1]);
qos.liveliness = str_to_qos_liveliness.at(liveliness_parts[0]);
qos.history = history_parts[0].empty() ? default_qos.history : str_to_qos_history.at(
history_parts[0]);
qos.reliability = parts[0].empty() ? default_qos.reliability : str_to_qos_reliability.at(
parts[0]);
qos.durability = parts[1].empty() ? default_qos.durability : str_to_qos_durability.at(parts[1]);
qos.liveliness =
liveliness_parts[0].empty() ? default_qos.liveliness : str_to_qos_liveliness.at(
liveliness_parts[0]);
} 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>
{
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;
};

const auto maybe_depth = str_to_size_t(history_parts[1]);
const auto maybe_deadline_s = str_to_size_t(deadline_parts[0]);
const auto maybe_deadline_ns = str_to_size_t(deadline_parts[1]);
const auto maybe_lifespan_s = str_to_size_t(lifespan_parts[0]);
const auto maybe_lifespan_ns = str_to_size_t(lifespan_parts[1]);
const auto maybe_liveliness_s = str_to_size_t(liveliness_parts[1]);
const auto maybe_liveliness_ns = str_to_size_t(liveliness_parts[2]);
const auto maybe_depth = str_to_size_t(history_parts[1], default_qos.depth);
const auto maybe_deadline_s = str_to_size_t(deadline_parts[0], default_qos.deadline.sec);
const auto maybe_deadline_ns = str_to_size_t(deadline_parts[1], default_qos.deadline.nsec);
const auto maybe_lifespan_s = str_to_size_t(lifespan_parts[0], default_qos.lifespan.sec);
const auto maybe_lifespan_ns = str_to_size_t(lifespan_parts[1], default_qos.lifespan.nsec);
const auto maybe_liveliness_s = str_to_size_t(
liveliness_parts[1],
default_qos.liveliness_lease_duration.sec);
const auto maybe_liveliness_ns = str_to_size_t(
liveliness_parts[2],
default_qos.liveliness_lease_duration.nsec);
if (maybe_depth == std::nullopt ||
maybe_deadline_s == std::nullopt ||
maybe_deadline_ns == std::nullopt ||
Expand Down
34 changes: 33 additions & 1 deletion rmw_zenoh_cpp/src/detail/liveliness_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
#include <string>
#include <vector>

#include "logging_macros.hpp"

#include "rmw/error_handling.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both of these can be moved back into liveliness_utils.cpp.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops my bad dd44983

#include "rmw/types.h"

namespace rmw_zenoh_cpp
Expand Down Expand Up @@ -218,7 +221,7 @@ std::string demangle_name(const std::string & input);
*
* See rmw/types.h for the values of each policy enum.
*/
std::string qos_to_keyexpr(rmw_qos_profile_t qos);
std::string qos_to_keyexpr(const rmw_qos_profile_t & qos);

///=============================================================================
/// Convert a rmw_qos_profile_t from a keyexpr. Return std::nullopt if invalid.
Expand All @@ -227,6 +230,35 @@ std::optional<rmw_qos_profile_t> keyexpr_to_qos(const std::string & keyexpr);
///=============================================================================
/// Convert a Zenoh id to a string.
std::string zid_to_str(const z_id_t & id);

///=============================================================================
// Helper function to convert string to size_t.
// The function is templated to enable conversion to size_t or std::size_t.
template<typename T>
std::optional<T> str_to_size_t(const std::string & str, const T default_value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

{
if (str.empty()) {
return default_value;
}
errno = 0;
char * endptr;
size_t num = strtoul(str.c_str(), &endptr, 10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it more today, I realized two problems here:

  1. strtoul is documented as returning an unsigned long, not a size_t. This is an existing bug.
  2. 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 strtoul or strtol as 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I've left 1) as a TODO but addressed 2) with an std::is_signed check. See bd3722f

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;
}
} // namespace liveliness
} // namespace rmw_zenoh_cpp

Expand Down