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

Jump to bottom

Allow unsetting (deleting) parameters #145

Merged
merged 1 commit into from Feb 13, 2023
Merged

Allow unsetting (deleting) parameters #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions foxglove_bridge_base/include/foxglove_bridge/parameter.hpp
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ enum class ParameterType {
class Parameter {
public:
Parameter();
Parameter(const std::string& name);
Parameter(const std::string& name, bool value);
Parameter(const std::string& name, int value);
Parameter(const std::string& name, int64_t value);
Expand Down
9 changes: 8 additions & 1 deletion foxglove_bridge_base/include/foxglove_bridge/websocket_server.hpp
View file
Open in desktop
Expand Up @@ -974,7 +974,14 @@ template <typename ServerConfiguration>
inline void Server<ServerConfiguration>::publishParameterValues(
ConnHandle hdl, const std::vector<Parameter>& parameters,
const std::optional<std::string>& requestId) {
nlohmann::json jsonPayload{{"op", "parameterValues"}, {"parameters", parameters}};
// Filter out parameters which are not set.
Copy link
Collaborator Author

@achim-k achim-k Feb 3, 2023 •
edited

Choose a reason for hiding this comment

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

The spec currently does not declare what parameter message to expect when requesting a non-existing / deleted parameter. Should it be

{
  "op": "parameterValues", 
  "parameters": []
}

or

{
  "op": "parameterValues", 
  "parameters": [{ "name": "/deleted_param" }]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The second example is not valid JSON. There is no undefined literal in https://www.rfc-editor.org/rfc/rfc8259

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adapted my first comment by removing the value field

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first is better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, that's also how it is currently implemented

std::vector<Parameter> nonEmptyParameters;
std::copy_if(parameters.begin(), parameters.end(), std::back_inserter(nonEmptyParameters),
[](const auto& p) {
return p.getType() != ParameterType::PARAMETER_NOT_SET;
});

nlohmann::json jsonPayload{{"op", "parameterValues"}, {"parameters", nonEmptyParameters}};
if (requestId) {
jsonPayload["id"] = requestId.value();
}
Expand Down
5 changes: 5 additions & 0 deletions foxglove_bridge_base/src/parameter.cpp
View file
Open in desktop
Expand Up @@ -7,6 +7,11 @@ Parameter::Parameter()
, _type(ParameterType::PARAMETER_NOT_SET)
, _value() {}

Parameter::Parameter(const std::string& name)
: _name(name)
, _type(ParameterType::PARAMETER_NOT_SET)
, _value() {}

Parameter::Parameter(const std::string& name, bool value)
: _name(name)
, _type(ParameterType::PARAMETER_BOOL)
Expand Down
8 changes: 7 additions & 1 deletion foxglove_bridge_base/src/serialization.cpp
View file
Open in desktop
Expand Up @@ -21,14 +21,20 @@ void to_json(nlohmann::json& j, const Parameter& p) {
} else if (paramType == ParameterType::PARAMETER_STRING_ARRAY) {
j["value"] = p.getValue<std::vector<std::string>>();
} else if (paramType == ParameterType::PARAMETER_NOT_SET) {
throw std::runtime_error("Unintialized parameter");
// empty value.
}

j["name"] = p.getName();
}

void from_json(const nlohmann::json& j, Parameter& p) {
const auto name = j["name"].get<std::string>();

if (j.find("value") == j.end()) {
p = Parameter(name); // Value is not set (undefined).
return;
}
Comment on lines +33 to +36
Copy link
Collaborator Author

@achim-k achim-k Feb 3, 2023 •
edited

Choose a reason for hiding this comment

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

I think it would be more explicit if the value was set to null (nlohmann::detail::value_t::null)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the comment here, null and undefined are not the same thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was referring to foxglove/ws-protocol#354 (comment), where we chose to use undefined (no value provided) instead of null.


const auto value = j["value"];
const auto jsonType = j["value"].type();

Expand Down
2 changes: 1 addition & 1 deletion ros1_foxglove_bridge/src/ros1_foxglove_bridge_nodelet.cpp
View file
Open in desktop
Expand Up @@ -529,7 +529,7 @@ class FoxgloveBridge : public nodelet::Nodelet {
} else if (paramType == ParameterType::PARAMETER_STRING_ARRAY) {
nh.setParam(paramName, param.getValue<std::vector<std::string>>());
} else if (paramType == ParameterType::PARAMETER_NOT_SET) {
ROS_ERROR("Parameter '%s' is not set", paramName.c_str());
nh.deleteParam(paramName);
}
}

Expand Down
14 changes: 14 additions & 0 deletions ros1_foxglove_bridge/tests/smoke_test.cpp
View file
Open in desktop
Expand Up @@ -206,6 +206,20 @@ TEST_F(ParameterTest, testSetParametersWithReqId) {
EXPECT_EQ(1UL, params.size());
}

TEST_F(ParameterTest, testUnsetParameter) {
const std::vector<foxglove::Parameter> parameters = {
foxglove::Parameter(PARAM_1_NAME),
};

const std::string requestId = "req-testUnsetParameter";
auto future = foxglove::waitForParameters(_wsClient, requestId);
_wsClient->setParameters(parameters, requestId);
ASSERT_EQ(std::future_status::ready, future.wait_for(DEFAULT_TIMEOUT));
std::vector<foxglove::Parameter> params = future.get();

EXPECT_EQ(0UL, params.size());
}

TEST_F(ParameterTest, testParameterSubscription) {
auto future = foxglove::waitForParameters(_wsClient);

Expand Down
22 changes: 20 additions & 2 deletions ros2_foxglove_bridge/src/parameter_interface.cpp
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ static rclcpp::Parameter toRosParam(const foxglove::Parameter& p) {
} else if (paramType == ParameterType::PARAMETER_STRING_ARRAY) {
return rclcpp::Parameter(p.getName(), p.getValue<std::vector<std::string>>());
} else if (paramType == ParameterType::PARAMETER_NOT_SET) {
throw std::runtime_error("Unintialized parameter");
return rclcpp::Parameter(p.getName());
}

return rclcpp::Parameter();
Expand All @@ -49,7 +49,7 @@ static foxglove::Parameter fromRosParam(const rclcpp::Parameter& p) {
const auto type = p.get_type();

if (type == rclcpp::ParameterType::PARAMETER_NOT_SET) {
return foxglove::Parameter();
return foxglove::Parameter(p.get_name());
} else if (type == rclcpp::ParameterType::PARAMETER_BOOL) {
return foxglove::Parameter(p.get_name(), p.as_bool());
} else if (type == rclcpp::ParameterType::PARAMETER_INTEGER) {
Expand Down Expand Up @@ -286,6 +286,24 @@ void ParameterInterface::setNodeParameters(rclcpp::AsyncParametersClient::Shared
}

auto future = paramClient->set_parameters(params);

std::vector<std::string> paramsToDelete;
for (const auto& p : params) {
if (p.get_type() == rclcpp::ParameterType::PARAMETER_NOT_SET) {
paramsToDelete.push_back(p.get_name());
}
}

if (!paramsToDelete.empty()) {
auto deleteFuture = paramClient->delete_parameters(paramsToDelete);
if (std::future_status::ready != deleteFuture.wait_for(timeout)) {
RCLCPP_WARN(
_node->get_logger(),
"Param client failed to delete %zu parameter(s) for node '%s' within the given timeout",
paramsToDelete.size(), nodeName.c_str());
}
}

if (std::future_status::ready != future.wait_for(timeout)) {
throw std::runtime_error("Param client failed to set " + std::to_string(params.size()) +
" parameter(s) for node '" + nodeName + "' within the given timeout");
Expand Down
21 changes: 20 additions & 1 deletion ros2_foxglove_bridge/tests/smoke_test.cpp
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ class ParameterTest : public ::testing::Test {
inline static const std::string NODE_1_NAME = "node_1";
inline static const std::string PARAM_1_NAME = "string_param";
inline static const PARAM_1_TYPE PARAM_1_DEFAULT_VALUE = "hello";
inline static const std::string DELETABLE_PARAM_NAME = "deletable_param";

using PARAM_2_TYPE = std::vector<int64_t>;
inline static const std::string NODE_2_NAME = "node_2";
Expand All @@ -35,12 +36,15 @@ class ParameterTest : public ::testing::Test {

protected:
void SetUp() override {
_paramNode1 = rclcpp::Node::make_shared(NODE_1_NAME);
auto nodeOptions = rclcpp::NodeOptions();
nodeOptions.allow_undeclared_parameters(true);
_paramNode1 = rclcpp::Node::make_shared(NODE_1_NAME, nodeOptions);
auto p1Param = rcl_interfaces::msg::ParameterDescriptor{};
p1Param.name = PARAM_1_NAME;
p1Param.type = rcl_interfaces::msg::ParameterType::PARAMETER_STRING;
p1Param.read_only = false;
_paramNode1->declare_parameter(p1Param.name, PARAM_1_DEFAULT_VALUE, p1Param);
_paramNode1->set_parameter(rclcpp::Parameter(DELETABLE_PARAM_NAME, true));

_paramNode2 = rclcpp::Node::make_shared(NODE_2_NAME);
auto p2Param = rcl_interfaces::msg::ParameterDescriptor{};
Expand Down Expand Up @@ -249,6 +253,21 @@ TEST_F(ParameterTest, testSetParametersWithReqId) {
EXPECT_EQ(1UL, params.size());
}

TEST_F(ParameterTest, testUnsetParameter) {
const auto p1 = NODE_1_NAME + "." + DELETABLE_PARAM_NAME;
const std::vector<foxglove::Parameter> parameters = {
foxglove::Parameter(p1),
};

const std::string requestId = "req-testUnsetParameter";
auto future = foxglove::waitForParameters(_wsClient, requestId);
_wsClient->setParameters(parameters, requestId);
ASSERT_EQ(std::future_status::ready, future.wait_for(DEFAULT_TIMEOUT));
std::vector<foxglove::Parameter> params = future.get();

EXPECT_EQ(0UL, params.size());
}

TEST_F(ParameterTest, testParameterSubscription) {
const auto p1 = NODE_1_NAME + "." + PARAM_1_NAME;

Expand Down