Conversation
WalkthroughAdds a new xbot_rpc package (msgs, srv, headers, provider, build manifests), integrates RPC into xbot_monitoring (deps, provider init, topics/services, request/response/error flow), and removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rovo89
force-pushed
the
feat/rpc
branch
2 times, most recently
from
91cbec9 to
1ccf5cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
src/lib/xbot_rpc/srv/RegisterMethodsSrv.srv (1)
1-3: Consider adding a response field for registration confirmation.The service currently has a void response. While this is valid, adding a boolean success field or a count of registered methods could improve observability and help callers verify successful registration.
Example enhancement:
string node_id # The ID of the node providing the methods. string[] methods # The list of available methods. --- +bool success # Indicates whether registration succeeded. +string message # Optional status or error message.src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
5-8: Useinline constexprfor header-only constants in C++17.While
static constexprworks, usinginline constexpris the modern C++17 best practice for header-only constants. This avoids potential ODR (One Definition Rule) issues and follows the C++ Core Guidelines.Apply this diff:
-static constexpr const char* TOPIC_REQUEST = "/xbot/rpc/request"; -static constexpr const char* TOPIC_RESPONSE = "/xbot/rpc/response"; -static constexpr const char* TOPIC_ERROR = "/xbot/rpc/error"; -static constexpr const char* SERVICE_REGISTER_METHODS = "/xbot/rpc/register"; +inline constexpr const char* TOPIC_REQUEST = "/xbot/rpc/request"; +inline constexpr const char* TOPIC_RESPONSE = "/xbot/rpc/response"; +inline constexpr const char* TOPIC_ERROR = "/xbot/rpc/error"; +inline constexpr const char* SERVICE_REGISTER_METHODS = "/xbot/rpc/register";Note: The static analysis errors shown are false positives from incomplete parsing and can be ignored.
src/lib/xbot_rpc/msg/RpcError.msg (1)
1-13: JSON‑RPC compatibility: id/data types
- id is restricted to string; JSON‑RPC 2.0 allows string, number, or null. Either (a) document the string‑only deviation, or (b) support numbers by carrying id as string over ROS but preserving original type on the MQTT side.
- data is a string here; ensure the MQTT bridge parses it back into JSON (see monitoring fixes below) to avoid double‑encoding.
src/lib/xbot_rpc/src/provider.cpp (2)
25-33: Improve registration robustness and observability.Add waitForExistence and a final error after retries for better startup behavior and diagnostics.
ros::Rate retry_delay(1); for (int i = 0; i < 10; i++) { + if (!registration_client.exists()) { + registration_client.waitForExistence(ros::Duration(1.0)); + } if (registration_client.call(srv)) { ROS_INFO_STREAM("successfully registered methods for " << node_id); break; } ROS_ERROR_STREAM("Error registering methods for " << node_id << ". Retrying."); retry_delay.sleep(); } + if (!registration_client.exists()) { + ROS_ERROR_STREAM("Failed to register methods for " << node_id << " after 10 attempts."); + }
43-52: Unify JSON type usage.You parse into ordered_json but store in basic_json<>. Prefer consistent types to avoid template mismatches.
- nlohmann::basic_json<> params; + nlohmann::ordered_json params; if (!request->params.empty()) { try { - params = nlohmann::ordered_json::parse(request->params); + params = nlohmann::ordered_json::parse(request->params);Also consider updating callback_t in the header to ordered_json for consistency.
src/lib/xbot_rpc/include/xbot_rpc/provider.h (3)
12-14: Macro ergonomics.RPC_METHOD is fine. Consider documenting that the first argument is the method id and the lambda receives (method, params) to avoid confusion.
17-19: Type clarity for JSON.If provider uses ordered_json, align callback_t to ordered_json to avoid conversions.
-typedef std::function<nlohmann::basic_json<>(const std::string& topic, const nlohmann::basic_json<>& params)> callback_t; +typedef std::function<nlohmann::ordered_json(const std::string& method, + const nlohmann::ordered_json& params)> callback_t;
20-29: Provide what() in RpcException.Override what() so std::exception handling yields a meaningful message.
class RpcException : public std::exception { public: const int16_t code; const std::string message; const nlohmann::basic_json<> data; RpcException(int16_t code, const std::string& message, const nlohmann::basic_json<>& data = nullptr) : code(code), message(message), data(data) { } + const char* what() const noexcept override { return message.c_str(); } };src/lib/xbot_monitoring/src/xbot_monitoring.cpp (1)
595-636: Validation tweaks and robust id handling when building ROS message.
- Avoid implicit json->string conversion; use getstd::string().
- Optional: If you intend string‑only ids, keep validation as-is; otherwise support numeric/null and preserve type end‑to‑end (requires widening ROS message schema).
- json id = req.contains("id") ? req["id"] : nullptr; - if (id != nullptr && !id.is_string()) { + json id = req.contains("id") ? req["id"] : nullptr; + if (id != nullptr && !id.is_string()) { return rpc_publish_error(xbot_rpc::RpcError::ERROR_INVALID_REQUEST, "ID is not a string", id); } else if (!req.contains("jsonrpc") || !req["jsonrpc"].is_string() || req["jsonrpc"] != "2.0") { return rpc_publish_error(xbot_rpc::RpcError::ERROR_INVALID_REQUEST, "Invalid JSON-RPC version"); } else if (!req.contains("method") || !req["method"].is_string()) { return rpc_publish_error(xbot_rpc::RpcError::ERROR_INVALID_REQUEST, "Method is not a string", req["id"]); } @@ - msg.id = id != nullptr ? id : ""; + msg.id = (id != nullptr) ? id.get<std::string>() : std::string();Also guard registered_methods with the mutex during lookup:
- bool is_registered = false; - for (const auto& [_, method_ids] : registered_methods) { + bool is_registered = false; + { + std::lock_guard<std::mutex> lk(registered_methods_mutex); + for (const auto& [_, method_ids] : registered_methods) { if (std::find(method_ids.begin(), method_ids.end(), method) != method_ids.end()) { is_registered = true; break; } - } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/lib/xbot_monitoring/CMakeLists.txt(1 hunks)src/lib/xbot_monitoring/package.xml(1 hunks)src/lib/xbot_monitoring/src/xbot_monitoring.cpp(9 hunks)src/lib/xbot_msgs/CMakeLists.txt(0 hunks)src/lib/xbot_msgs/msg/ActionRequest.msg(0 hunks)src/lib/xbot_msgs/msg/ActionResponse.msg(0 hunks)src/lib/xbot_rpc/CMakeLists.txt(1 hunks)src/lib/xbot_rpc/include/xbot_rpc/constants.h(1 hunks)src/lib/xbot_rpc/include/xbot_rpc/provider.h(1 hunks)src/lib/xbot_rpc/msg/RpcError.msg(1 hunks)src/lib/xbot_rpc/msg/RpcRequest.msg(1 hunks)src/lib/xbot_rpc/msg/RpcResponse.msg(1 hunks)src/lib/xbot_rpc/package.xml(1 hunks)src/lib/xbot_rpc/src/provider.cpp(1 hunks)src/lib/xbot_rpc/srv/RegisterMethodsSrv.srv(1 hunks)
💤 Files with no reviewable changes (3)
- src/lib/xbot_msgs/msg/ActionRequest.msg
- src/lib/xbot_msgs/CMakeLists.txt
- src/lib/xbot_msgs/msg/ActionResponse.msg
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
xbot_rpc(15-62)
src/lib/xbot_rpc/src/provider.cpp (1)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (2)
string(74-93)string(74-74)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
xbot_rpc(3-10)src/lib/xbot_rpc/src/provider.cpp (10)
handleRequest(36-63)handleRequest(36-36)publishResponse(65-74)publishResponse(65-66)publishError(76-89)publishError(76-77)init(7-15)init(7-7)publishMethods(17-34)publishMethods(17-17)
🪛 Clang (14.0.6)
src/lib/xbot_rpc/include/xbot_rpc/constants.h
[error] 3-3: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 3-3: expected ';' after top level declarator
(clang-diagnostic-error)
src/lib/xbot_rpc/src/provider.cpp
[error] 1-1: 'xbot_rpc/provider.h' file not found
(clang-diagnostic-error)
src/lib/xbot_rpc/include/xbot_rpc/provider.h
[error] 3-3: 'ros/ros.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-24.04-arm, arm64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04-arm, arm64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04, amd64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, legacy, ./docker/Dockerfile.Legacy)
🔇 Additional comments (8)
src/lib/xbot_rpc/package.xml (1)
1-23: LGTM!The package manifest is well-structured and includes all required dependencies for a ROS package with message generation.
src/lib/xbot_monitoring/package.xml (1)
25-25: LGTM!The xbot_rpc dependency is correctly added using the
<depend>tag, which covers build, export, and runtime requirements.src/lib/xbot_monitoring/CMakeLists.txt (1)
16-16: LGTM!The xbot_rpc component is correctly added to the catkin build dependencies, consistent with the package.xml changes.
src/lib/xbot_rpc/src/provider.cpp (1)
76-89: Guard optional error data correctly.data != nullptr is fine for nlohmann::json. Keep as-is.
Please verify downstream (monitoring) parses err_msg.data back into JSON before publishing to MQTT to avoid double‑encoding (see my comment on rpc_error_callback).
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
41-45: Visibility of publishMethods.publishMethods is already public below; avoid duplicate declarations if any arise during future merges.
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (3)
123-126: OK: Forwarding MQTT RPC requests into local handler.Looks good.
638-648: Result parsing and error propagation: OK.Good fallback to publish an internal error with the raw result on parse failure.
Consider logging the offending provider node/method for easier tracing.
702-706: RPC wiring: LGTM.Good: publishers/subscribers and service advertised before provider.init().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
10-10: Prefer package-qualified include for portability.Using
"constants.h"(local include) may fail when this header is included from other packages. Package-qualified includes are more robust.Apply this diff:
-#include "constants.h" +#include <xbot_rpc/constants.h>
3-11: Add standard library includes for self-containment.The header uses standard types but relies on transitive includes. Adding direct includes prevents breakage if dependencies change their includes.
Apply this diff:
#pragma once +#include <cstdint> +#include <exception> +#include <functional> +#include <map> +#include <string> +#include <utility> + #include <ros/ros.h> #include <xbot_rpc/RpcError.h> #include <xbot_rpc/RpcRequest.h> #include <xbot_rpc/RpcResponse.h> #include <nlohmann/json.hpp> -#include "constants.h" +#include <xbot_rpc/constants.h>src/lib/xbot_monitoring/src/xbot_monitoring.cpp (1)
655-657: Parse serialized JSON before forwarding to MQTT.The
msg->datafield contains a JSON-serialized string (fromprovider.cpp:86). Passing it directly torpc_publish_errorcauses double-encoding, embedding escaped JSON as a string value instead of an object.Apply this diff to parse the data:
void rpc_error_callback(const xbot_rpc::RpcError::ConstPtr &msg) { - rpc_publish_error(msg->code, msg->message, msg->id, msg->data); + json id = msg->id.empty() ? nullptr : json(msg->id); + if (!msg->data.empty()) { + try { + json data = json::parse(msg->data); + rpc_publish_error(msg->code, msg->message, id, data); + } catch (const json::parse_error&) { + // On parse failure, omit data or use string as fallback + rpc_publish_error(msg->code, msg->message, id); + } + } else { + rpc_publish_error(msg->code, msg->message, id); + } }Based on learnings
🧹 Nitpick comments (1)
src/lib/xbot_rpc/src/provider.cpp (1)
60-60: Prefer const reference for read-only exception.Catching by non-const reference when only reading is unconventional.
Apply this diff:
- } catch (std::exception& e) { + } catch (const std::exception& e) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp(9 hunks)src/lib/xbot_rpc/CMakeLists.txt(1 hunks)src/lib/xbot_rpc/include/xbot_rpc/constants.h(1 hunks)src/lib/xbot_rpc/include/xbot_rpc/provider.h(1 hunks)src/lib/xbot_rpc/src/provider.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/xbot_rpc/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
📚 Learning: 2025-10-27T23:20:49.624Z
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
Applied to files:
src/lib/xbot_rpc/include/xbot_rpc/constants.hsrc/lib/xbot_monitoring/src/xbot_monitoring.cppsrc/lib/xbot_rpc/include/xbot_rpc/provider.h
🧬 Code graph analysis (3)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
xbot_rpc(15-33)
src/lib/xbot_rpc/src/provider.cpp (1)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (2)
string(75-94)string(75-75)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
xbot_rpc(3-10)src/lib/xbot_rpc/src/provider.cpp (10)
handleRequest(36-63)handleRequest(36-36)publishResponse(65-74)publishResponse(65-66)publishError(76-89)publishError(76-77)init(7-15)init(7-7)publishMethods(17-34)publishMethods(17-17)
🪛 Clang (14.0.6)
src/lib/xbot_rpc/include/xbot_rpc/constants.h
[error] 3-3: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 3-3: expected ';' after top level declarator
(clang-diagnostic-error)
src/lib/xbot_rpc/src/provider.cpp
[error] 1-1: 'xbot_rpc/provider.h' file not found
(clang-diagnostic-error)
src/lib/xbot_rpc/include/xbot_rpc/provider.h
[error] 3-3: 'ros/ros.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-24.04, amd64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04-arm, arm64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04-arm, arm64, default, ./docker/Dockerfile)
🔇 Additional comments (15)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
1-10: LGTM!The constants header is well-structured. The use of
inline constexpr const char*ensures no ODR violations while providing compile-time constant strings for RPC topic and service names.src/lib/xbot_rpc/src/provider.cpp (4)
7-15: LGTM!The initialization properly sets up the ROS communication infrastructure with appropriate queue sizes. The use of
tcpNoDelay(true)is beneficial for RPC latency.
17-34: LGTM!The retry logic is sensible for service registration during node startup. The structured binding with
_for the unused callback value is idiomatic C++17.
65-74: LGTM!Correctly implements JSON-RPC 2.0 notification semantics by suppressing responses when
idis empty.
76-89: LGTM!The error publishing logic properly handles optional error data and respects JSON-RPC 2.0 notification semantics.
src/lib/xbot_rpc/include/xbot_rpc/provider.h (3)
12-13: LGTM!The macro provides convenient method registration syntax. The lambda captures align with the
callback_tsignature and the brace-enclosed body allows multi-statement implementations.
17-18: LGTM!The callback signature is appropriate for RPC handlers, taking method name and parameters as const references and returning a JSON result.
20-33: LGTM!The exception class follows best practices: const members prevent mutation, proper inheritance from
std::exception, andnoexceptonwhat()satisfies the standard contract.src/lib/xbot_monitoring/src/xbot_monitoring.cpp (7)
30-30: LGTM!Switching to
ordered_jsonensures consistent key ordering in MQTT payloads, which can be beneficial for debugging and testing.
42-44: LGTM!The mutex properly protects the
registered_methodsmap from concurrent access between MQTT callbacks and ROS service handlers, addressing the data race concern.
142-157: LGTM!The RPC provider initialization includes built-in introspection methods. The
rpc.methodshandler properly locks theregistered_methodsmap during iteration and provides sorted output for consistency.
597-641: LGTM!The request handler implements comprehensive JSON-RPC 2.0 validation and properly synchronizes access to
registered_methodsduring method lookup. The forwarding to ROS correctly serializes parameters and handles optional request IDs.
643-653: LGTM!The response handler correctly parses the serialized result JSON before constructing the MQTT response, avoiding double-encoding. The error handling for parse failures is appropriate.
659-664: LGTM!The service handler correctly protects the write to
registered_methodswith the mutex, ensuring thread-safe registration.
708-716: LGTM!The RPC infrastructure is properly wired during initialization. Publishers, subscribers, and service server are created with appropriate queue sizes, and the provider is initialized after all ROS communication channels are established.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
1-10: Header still not self-contained.The header uses
std::string,std::map,std::function, andint16_tbut doesn't explicitly include their headers (<string>,<map>,<functional>,<cstdint>). Whileros/ros.hmay transitively provide these, the header should not rely on transitive includes.Apply this diff to make the header self-contained:
#pragma once +#include <cstdint> +#include <functional> +#include <map> +#include <string> + #include <ros/ros.h> #include <xbot_rpc/RpcError.h> #include <xbot_rpc/RpcRequest.h> #include <xbot_rpc/RpcResponse.h> #include <xbot_rpc/constants.h> #include <nlohmann/json.hpp>src/lib/xbot_monitoring/src/xbot_monitoring.cpp (1)
652-654: Consider future-proofing for the optional data field.The current implementation works correctly because
provider.cppnever sets thedatafield ofRpcError(it remains empty). However, if future code does populate it, the data would need to be parsed from string to JSON to avoid double-encoding in the MQTT response, similar to howmsg->resultis handled inrpc_response_callback.If you want to future-proof this, you could add handling for the data field:
void rpc_error_callback(const xbot_rpc::RpcError::ConstPtr &msg) { - rpc_publish_error(msg->code, msg->message, msg->id); + json id = msg->id.empty() ? nullptr : json(msg->id); + // Currently data is always empty, but parse it if present for future extensibility + json data = nullptr; + if (!msg->data.empty()) { + try { + data = json::parse(msg->data); + } catch (...) { + // If parse fails, include raw string + data = msg->data; + } + } + json err_resp = {{"jsonrpc", "2.0"}, + {"error", {{"code", msg->code}, {"message", msg->message}}}, + {"id", id}}; + if (data != nullptr) { + err_resp["error"]["data"] = data; + } + try_publish("rpc/response", err_resp.dump(2)); }Note: This would also require updating the
rpc_publish_errorsignature to accept optional data, or handling it inline as shown above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp(9 hunks)src/lib/xbot_rpc/include/xbot_rpc/provider.h(1 hunks)src/lib/xbot_rpc/src/provider.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
📚 Learning: 2025-10-27T23:20:49.624Z
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
Applied to files:
src/lib/xbot_monitoring/src/xbot_monitoring.cppsrc/lib/xbot_rpc/include/xbot_rpc/provider.h
🧬 Code graph analysis (2)
src/lib/xbot_rpc/src/provider.cpp (1)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (2)
string(75-94)string(75-75)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
xbot_rpc(3-10)src/lib/xbot_rpc/src/provider.cpp (10)
handleRequest(36-63)handleRequest(36-36)publishResponse(65-74)publishResponse(65-66)publishError(76-85)publishError(76-76)init(7-15)init(7-7)publishMethods(17-34)publishMethods(17-17)
🪛 Clang (14.0.6)
src/lib/xbot_rpc/src/provider.cpp
[error] 1-1: 'xbot_rpc/provider.h' file not found
(clang-diagnostic-error)
src/lib/xbot_rpc/include/xbot_rpc/provider.h
[error] 3-3: 'ros/ros.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-24.04, amd64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04-arm, arm64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04-arm, arm64, legacy, ./docker/Dockerfile.Legacy)
🔇 Additional comments (16)
src/lib/xbot_rpc/src/provider.cpp (4)
7-15: LGTM! Initialization logic is correct.The queue size issue from the previous review has been addressed. The initialization properly sets up all ROS communication channels and triggers method registration.
17-34: LGTM! Retry logic is appropriate.The 10-retry limit with 1-second delays provides a reasonable grace period for the registration service to become available during system startup.
36-63: LGTM! Request handling is robust.The implementation correctly:
- Silently ignores requests for unregistered methods (allowing multiple providers to coexist)
- Validates JSON parameter syntax before execution
- Catches both RpcException and generic exceptions
- Maps exceptions to appropriate JSON-RPC error codes
65-85: LGTM! Response and error handling follows JSON-RPC 2.0 spec.Both methods correctly check for empty request IDs to avoid responding to notifications. The RpcError message's optional
datafield is intentionally left empty in this implementation, which is valid per JSON-RPC 2.0.src/lib/xbot_rpc/include/xbot_rpc/provider.h (3)
11-17: LGTM! Macro and typedef provide clean registration API.The
RPC_METHODmacro elegantly creates inline method registrations, and thecallback_ttypedef clearly documents the callback signature.
19-31: LGTM! Exception class is correctly designed.The RpcException properly inherits from
std::exceptionand provides both structured error information (code, message) and the standardwhat()interface.
33-61: LGTM! Provider class has a clean public API.The class provides:
- Constructor with optional initial methods
- Initialization to set up ROS communication
- Method registration (individual or batch)
- Automatic method publishing to the registry
The private implementation details are appropriately hidden.
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (9)
17-17: LGTM! Includes and type alias are appropriate.The includes bring in the necessary RPC infrastructure, and using
ordered_jsonensures deterministic JSON output which is helpful for testing and debugging.Also applies to: 23-28, 30-30
42-44: LGTM! RPC state management is thread-safe.The
registered_methodsmap is properly protected by a mutex, addressing concurrency concerns from the previous review. The publisher declaration is straightforward.Also applies to: 62-62
93-93: LGTM! MQTT-to-RPC routing is correct.The subscription to the
rpc/requesttopic and the routing logic in the message handler properly integrate RPC into the MQTT callback flow.Also applies to: 124-127
142-157: LGTM! Built-in RPC methods are correctly implemented.The
rpc.pingandrpc.methodsmethods follow JSON-RPC conventions. The mutex is correctly held while iteratingregistered_methodsto prevent concurrent modification.
587-592: LGTM! Error response helper follows JSON-RPC 2.0.The function correctly constructs JSON-RPC 2.0 error responses with the required
jsonrpc,error, andidfields.
594-638: LGTM! Request validation and routing is thorough.The implementation correctly:
- Validates JSON syntax and structure
- Checks JSON-RPC 2.0 version and required fields
- Verifies the requested method is registered (with proper locking)
- Forwards valid requests to providers via ROS
- Returns appropriate error codes for each failure mode
640-650: LGTM! Response handling correctly deserializes the result.The implementation properly parses
msg->result(which arrives as a JSON string from the provider) back into a JSON value before embedding it in the JSON-RPC response. This is the correct approach—theresultfield should be a JSON value, not a double-encoded string.Based on learnings: This follows the established pattern where xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
656-661: LGTM! Method registration is thread-safe.The mutex correctly protects the
registered_methodsupdate, addressing the concurrency concern from the previous review.
705-708: LGTM! RPC integration wiring is complete.The setup properly:
- Creates the ROS publisher for forwarding requests
- Subscribes to response and error topics
- Advertises the registration service
- Initializes the provider to register built-in methods
Also applies to: 713-713
We probably won't need it and it would require even more complexity than we already had (to decode the JSON in xbot_monitoring if necessary).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/xbot_rpc/src/provider.cpp (1)
18-28: Consider retry logic or better error handling for method registration.If the registration service call fails, the provider's methods won't be discoverable by clients, but the provider continues silently. Consider either implementing retry logic with exponential backoff or surfacing the registration failure to the caller.
Example with simple retry:
void RpcProvider::publishMethods() { + const int max_attempts = 3; + for (int attempt = 1; attempt <= max_attempts; ++attempt) { xbot_rpc::RegisterMethodsSrv srv; srv.request.node_id = node_id; srv.request.methods.reserve(methods.size()); for (const auto& [method_id, _] : methods) { srv.request.methods.push_back(method_id); } - if (!registration_client.call(srv)) { - ROS_ERROR_STREAM("Error registering methods for " << node_id); + if (registration_client.call(srv)) { + return; + } + if (attempt < max_attempts) { + ROS_WARN_STREAM("Failed to register methods for " << node_id << ", attempt " << attempt << "/" << max_attempts); + ros::Duration(1.0).sleep(); + } else { + ROS_ERROR_STREAM("Failed to register methods for " << node_id << " after " << max_attempts << " attempts"); } + } }src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
11-12: Rename parameter from "topic" to "method" for clarity.The first parameter of the callback is the method name (as seen in provider.cpp line 50 where it's called with
request->method), not a topic. The parameter name "topic" is misleading.-#define RPC_METHOD(id, body) \ - { id, [](const std::string& topic, const nlohmann::basic_json<>& params) body } +#define RPC_METHOD(id, body) \ + { id, [](const std::string& method, const nlohmann::basic_json<>& params) body }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp(9 hunks)src/lib/xbot_rpc/include/xbot_rpc/provider.h(1 hunks)src/lib/xbot_rpc/msg/RpcError.msg(1 hunks)src/lib/xbot_rpc/src/provider.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
📚 Learning: 2025-10-27T23:20:49.624Z
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
Applied to files:
src/lib/xbot_monitoring/src/xbot_monitoring.cppsrc/lib/xbot_rpc/include/xbot_rpc/provider.h
🧬 Code graph analysis (2)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
xbot_rpc(3-10)src/lib/xbot_rpc/src/provider.cpp (10)
handleRequest(30-57)handleRequest(30-30)publishResponse(59-68)publishResponse(59-60)publishError(70-79)publishError(70-70)init(7-16)init(7-7)publishMethods(18-28)publishMethods(18-18)
src/lib/xbot_rpc/src/provider.cpp (1)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (2)
string(75-94)string(75-75)
🪛 Clang (14.0.6)
src/lib/xbot_rpc/include/xbot_rpc/provider.h
[error] 3-3: 'ros/ros.h' file not found
(clang-diagnostic-error)
src/lib/xbot_rpc/src/provider.cpp
[error] 1-1: 'xbot_rpc/provider.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-24.04-arm, arm64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04-arm, arm64, legacy, ./docker/Dockerfile.Legacy)
🔇 Additional comments (15)
src/lib/xbot_rpc/msg/RpcError.msg (2)
1-3: LGTM!The core error message fields are correctly defined for JSON-RPC 2.0 error responses.
5-12: LGTM!The error code constants correctly match the JSON-RPC 2.0 specification, and the comments provide clear descriptions.
src/lib/xbot_rpc/src/provider.cpp (4)
7-16: LGTM!The initialization properly sets up ROS communication with appropriate queue sizes and the 10-second service wait provides reasonable startup behavior.
30-57: LGTM!The request handling logic is solid: it correctly ignores unknown methods (allowing multiple providers), handles JSON parsing errors, executes callbacks, and catches both custom and standard exceptions with appropriate error responses.
59-68: LGTM!Correctly implements JSON-RPC 2.0 response handling by only publishing responses for requests with an id (notifications don't get responses).
70-79: LGTM!Error publishing correctly follows JSON-RPC 2.0 by only sending errors for requests with an id.
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
19-31: LGTM!The RpcException class provides a clean way to signal RPC errors with both a code and message, properly inheriting from std::exception.
33-61: LGTM!The RpcProvider class is well-structured with appropriate encapsulation. Note that
init()must be called after construction to set up ROS communication.src/lib/xbot_monitoring/src/xbot_monitoring.cpp (7)
23-30: LGTM!The switch to
ordered_jsonensures consistent JSON output ordering, which is helpful for testing and debugging.
42-44: LGTM!The registered_methods map is properly protected with a mutex for thread-safe access between MQTT and ROS callbacks.
Also applies to: 62-62
142-157: LGTM!The built-in RPC methods provide essential functionality:
rpc.pingfor health checks andrpc.methodsfor method discovery. The mutex protection inrpc.methodsis correct.
587-592: LGTM!The function correctly formats JSON-RPC 2.0 error responses for MQTT clients, including the required "jsonrpc" field at the protocol boundary. Based on learnings.
640-650: LGTM!The response callback correctly parses the serialized JSON result from the ROS message and formats it as a proper JSON-RPC 2.0 response for MQTT clients.
652-661: LGTM!Both callbacks correctly handle their respective flows: forwarding errors to MQTT and registering methods with proper mutex protection.
705-708: LGTM!The RPC infrastructure is properly initialized with consistent queue sizes and the provider is initialized after ROS node setup.
Also applies to: 713-713
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
3-9: Standard headers still missing (partial fix from previous review).While line 7 correctly fixed the
constants.hinclude path, the header still lacks explicit includes for standard types it uses directly. Relying on transitive includes fromros/ros.his brittle.Add the following includes after line 6:
#include <xbot_rpc/RpcResponse.h> #include <xbot_rpc/constants.h> +#include <cstdint> +#include <exception> +#include <functional> +#include <map> +#include <string> +#include <utility> #include <nlohmann/json.hpp>
🧹 Nitpick comments (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (1)
47-47: Consider move semantics for the constructor parameter.The constructor copies the
methodsmap. For better performance with large method registries, consider accepting the map by value and moving it.- RpcProvider(const std::string& node_id, const std::map<std::string, callback_t>& methods = {}) : node_id(node_id), methods(methods) {} + RpcProvider(const std::string& node_id, std::map<std::string, callback_t> methods = {}) + : node_id(node_id), methods(std::move(methods)) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
📚 Learning: 2025-10-27T23:20:49.624Z
Learnt from: rovo89
PR: ClemensElflein/open_mower_ros#252
File: src/lib/xbot_rpc/msg/RpcRequest.msg:1-3
Timestamp: 2025-10-27T23:20:49.624Z
Learning: In the open_mower_ros xbot_rpc system, the xbot_monitoring node acts as the JSON-RPC 2.0 gateway between MQTT and internal ROS messaging. The RpcRequest.msg, RpcResponse.msg, and RpcError.msg are internal ROS message formats and do not need to include the "jsonrpc" field because xbot_monitoring handles JSON-RPC 2.0 protocol compliance at the MQTT boundary.
Applied to files:
src/lib/xbot_rpc/include/xbot_rpc/provider.h
🧬 Code graph analysis (1)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (2)
src/lib/xbot_rpc/include/xbot_rpc/constants.h (1)
xbot_rpc(3-10)src/lib/xbot_rpc/src/provider.cpp (10)
handleRequest(30-57)handleRequest(30-30)publishResponse(59-68)publishResponse(59-60)publishError(70-79)publishError(70-70)init(7-16)init(7-7)publishMethods(18-28)publishMethods(18-18)
🪛 Clang (14.0.6)
src/lib/xbot_rpc/include/xbot_rpc/provider.h
[error] 3-3: 'ros/ros.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-24.04, amd64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04, amd64, legacy, ./docker/Dockerfile.Legacy)
- GitHub Check: build (ubuntu-24.04-arm, arm64, default, ./docker/Dockerfile)
- GitHub Check: build (ubuntu-24.04-arm, arm64, legacy, ./docker/Dockerfile.Legacy)
🔇 Additional comments (3)
src/lib/xbot_rpc/include/xbot_rpc/provider.h (3)
11-12: Convenience macro design is sound.The
RPC_METHODmacro provides a clean initializer syntax for registering handlers. The limitation that lambdas cannot capture variables is acceptable—users can manually define lambdas with captures when needed.
18-30: Exception class design is clean and correct.The
RpcExceptiondesign properly inherits fromstd::exception, uses const members for immutability, and correctly implementswhat()withnoexcept. Public access tocodeandmessageis appropriate for a simple exception type.
51-57:addMethodoverloads are well-designed.Both overloads use appropriate insertion methods:
emplacefor constructing in-place from separate arguments, andinsertfor an existing pair. The design provides flexibility for users.
1 participant
For more advanced cases of passing data to ROS (including method calls with parameters), let's use JSON RPC 2.0 over the existing MQTT connection.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Chores