Skip to content

Commit

Permalink
Improve Error Reporting and Reduce Log Redundancy (#327)
Browse files Browse the repository at this point in the history
### Changelog
None

### Docs
None

### Description

<!-- Describe the problem, what has changed, and motivation behind those
changes. Pretend you are advocating for this change and the reader is
skeptical. -->

The ROS 1 foxglove_bridge rejects parameter names with `.` and spams the
terminal with redundant error messages.

While it is possible for ROS 1 parameters to have a `.` in their name,
they are not "supposed" to (`Character [.] is not valid in Graph
Resource Name`). For this reason, the foxglove_bridge will keep raising
errors for parameter names with `.`

This PR addresses the error handling part of this issue: an unordered
set tracks the encountered invalid parameters to throttle error log
messages.

<!-- In addition to unit tests, describe any manual testing you did to
validate this change. -->

<table><tr><th>Before</th><th>After</th></tr><tr><td>

The bridge std_out gets periodically spammed with the same error
messages until the invalid parameter is removed.
In the app, the only visible error message is too vague.

<img width="824" alt="error_log_before"
src="https://github.com/user-attachments/assets/b7628362-5e36-497e-b3b2-485fc18edf68">

</td><td>

The bridge std_out only prints errors when invalid parameters are first
encountered—one error message per invalid parameter.
In the app, the error message lists the invalid parameters.

<img width="818" alt="error_log_after"
src="https://github.com/user-attachments/assets/1ac6b9d6-3f3f-4b01-b032-d5615438bac5">

</td></tr></table>
  • Loading branch information
rdumas01 authored Nov 26, 2024
1 parent 9ef51e7 commit 7f2968f
Showing 1 changed file with 27 additions and 1 deletion.
28 changes: 27 additions & 1 deletion ros1_foxglove_bridge/src/ros1_foxglove_bridge_nodelet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ constexpr uint32_t SUBSCRIPTION_QUEUE_LENGTH = 10;
constexpr double MIN_UPDATE_PERIOD_MS = 100.0;
constexpr uint32_t PUBLICATION_QUEUE_LENGTH = 10;
constexpr int DEFAULT_SERVICE_TYPE_RETRIEVAL_TIMEOUT_MS = 250;
constexpr int MAX_INVALID_PARAMS_TRACKED = 1000;

using ConnectionHandle = websocketpp::connection_hdl;
using TopicAndDatatype = std::pair<std::string, std::string>;
Expand Down Expand Up @@ -656,6 +657,7 @@ class FoxgloveBridge : public nodelet::Nodelet {

bool success = true;
std::vector<foxglove::Parameter> params;
std::vector<std::string> invalidParams;
for (const auto& paramName : parameterNames) {
if (!isWhitelisted(paramName, _paramWhitelistPatterns)) {
if (allParametersRequested) {
Expand All @@ -665,27 +667,50 @@ class FoxgloveBridge : public nodelet::Nodelet {
success = false;
}
}
if (_invalidParams.find(paramName) != _invalidParams.end()) {
continue;
}

try {
XmlRpc::XmlRpcValue value;
getMTNodeHandle().getParam(paramName, value);
params.push_back(fromRosParam(paramName, value));
} catch (const std::exception& ex) {
ROS_ERROR("Invalid parameter '%s': %s", paramName.c_str(), ex.what());
invalidParams.push_back(paramName);
success = false;
} catch (const XmlRpc::XmlRpcException& ex) {
ROS_ERROR("Invalid parameter '%s': %s", paramName.c_str(), ex.getMessage().c_str());
invalidParams.push_back(paramName);
success = false;
} catch (...) {
ROS_ERROR("Invalid parameter '%s'", paramName.c_str());
invalidParams.push_back(paramName);
success = false;
}
}

_server->publishParameterValues(hdl, params, requestId);

if (!success) {
throw std::runtime_error("Failed to retrieve one or multiple parameters");
for (std::string& param : invalidParams) {
if (_invalidParams.size() < MAX_INVALID_PARAMS_TRACKED) {
_invalidParams.insert(param);
}
}

if (!invalidParams.empty()) {
std::string errorMsg = "Failed to retrieve the following parameters: ";
for (size_t i = 0; i < invalidParams.size(); i++) {
errorMsg += invalidParams[i];
if (i < invalidParams.size() - 1) {
errorMsg += ", ";
}
}
throw std::runtime_error(errorMsg);
} else {
throw std::runtime_error("Failed to retrieve one or multiple parameters");
}
}
}

Expand Down Expand Up @@ -922,6 +947,7 @@ class FoxgloveBridge : public nodelet::Nodelet {
int _serviceRetrievalTimeoutMs = DEFAULT_SERVICE_TYPE_RETRIEVAL_TIMEOUT_MS;
std::atomic<bool> _subscribeGraphUpdates = false;
std::unique_ptr<foxglove::CallbackQueue> _fetchAssetQueue;
std::unordered_set<std::string> _invalidParams;
};

} // namespace foxglove_bridge
Expand Down

0 comments on commit 7f2968f

Please sign in to comment.