Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates the RPC provider package from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
src/lib/xbot_monitoring/src/xbot_monitoring.cpp (2)
151-153: Minor data race oncurrent_high_level_state.
current_high_level_stateis written inhigh_level_status_callback(line 488) and read inpose_callback(lines 522-523) potentially from different threads via theAsyncSpinner. While the impact is minimal (a missed idle check could add one extra point), consider usingstd::atomic<uint8_t>for correctness.🔧 Proposed fix
EventHistory event_history; PositionHistory position_history; -uint8_t current_high_level_state = mower_msgs::HighLevelStatus::HIGH_LEVEL_STATE_NULL; +std::atomic<uint8_t> current_high_level_state{mower_msgs::HighLevelStatus::HIGH_LEVEL_STATE_NULL};Add include at the top:
`#include` <atomic>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/xbot_monitoring/src/xbot_monitoring.cpp` around lines 151 - 153, current_high_level_state is subject to a data race because it's written in high_level_status_callback and read in pose_callback from different AsyncSpinner threads; change its type from uint8_t to std::atomic<uint8_t> and include <atomic> at the top of the file, then update any direct reads/writes (references to current_high_level_state) to use the atomic operations (load/store) as needed to ensure thread-safe access.
743-747: Parameter reuse may cause confusion.
event_history_max_sizeis used to initialize bothevent_historyandposition_history, but the parameter name only references events. Consider either adding a separateposition_history_max_sizeparameter or documenting this dual-use in a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/xbot_monitoring/src/xbot_monitoring.cpp` around lines 743 - 747, The code currently reads a single parameter named event_history_max_size via paramNh.param and uses it to initialize both event_history.init(...) and position_history.init(...); introduce clarity by either adding a new parameter (e.g., position_history_max_size) and use paramNh.param("position_history_max_size", <fallback>) to initialize position_history.init(static_cast<size_t>(position_history_max_size)), or if shared sizing is intentional add a brief comment next to the paramNh.param call explaining that event_history_max_size intentionally controls both event_history and position_history; update the initialization lines to use the appropriate variable names (event_history_max_size vs position_history_max_size) and ensure defaults remain sensible.src/lib/xbot_mqtt/include/xbot_mqtt/publish.h (1)
46-59: Details can overwrite injected fields.If
detailscontains keys"id","t", or"type", thepayload.update(details)call will overwrite the auto-generated values. If this is intentional (allowing caller overrides), consider documenting it. Otherwise, consider merging in the opposite order or usingmerge_patch.🔧 If overwriting should be prevented
inline void publishEvent(ros::Publisher& pub, const std::string& type, nlohmann::json details = nullptr, bool retain = false) { - nlohmann::json payload = { - {"id", generateNanoId()}, - {"t", ros::Time::now().toSec()}, - {"type", type}, - }; - if (details.is_object()) { - payload.update(details); - } + nlohmann::json payload = details.is_object() ? details : nlohmann::json::object(); + payload["id"] = generateNanoId(); + payload["t"] = ros::Time::now().toSec(); + payload["type"] = type; publish(pub, "events/json", payload, retain); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/xbot_mqtt/include/xbot_mqtt/publish.h` around lines 46 - 59, publishEvent currently builds payload with generated "id","t","type" then calls payload.update(details), which allows details to overwrite those injected fields; to prevent this, change the merge order or guard keys so injected values from generateNanoId(), ros::Time::now().toSec(), and the type parameter cannot be replaced by details: in publishEvent (referencing generateNanoId(), publishEvent, payload.update, and publish) merge details into payload in a way that preserves existing keys (e.g., only copy keys from details that are not "id","t","type" or perform payload = details; then set payload["id"]=generateNanoId(); payload["t"]=...; payload["type"]=type), and then call publish(pub, "events/json", payload, retain).src/lib/xbot_monitoring/src/PositionHistory.h (2)
22-28: Hardcoded relative file path may cause issues.
file_path_is set to a relative path"positions.jsonl". If the working directory changes between runs or varies across deployment environments, data could be written to unexpected locations or fail to load.Consider deriving an absolute path from a ROS parameter or a well-known directory (e.g.,
~/.ros/or a package-specific data directory).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/xbot_monitoring/src/PositionHistory.h` around lines 22 - 28, The init method currently assigns a hardcoded relative path to file_path_ ("positions.jsonl"); change init (or overload it) to derive an absolute path from a ROS parameter (e.g., fetch a param like "position_history_path") with a sensible fallback such as the user's ROS home directory or a package-specific data directory, expanding '~' and converting to an absolute path (use std::filesystem::absolute or equivalent) before assigning file_path_; ensure loadFromDisk() and startNewSegment() continue to use file_path_ unchanged and that init documents/validates the resolved path.
124-126: Consider logging malformed lines for debugging.The catch-all silently discards any errors during line parsing. While this is appropriate for resilience against corrupted files, emitting a
ROS_WARNwith the line number or content excerpt would aid troubleshooting without breaking the recovery flow.🔧 Proposed improvement
} catch (...) { - // skip malformed lines + ROS_WARN_STREAM("PositionHistory: skipping malformed line in " << file_path_); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/xbot_monitoring/src/PositionHistory.h` around lines 124 - 126, The catch-all in PositionHistory.h currently swallows parsing errors; instead, log a non-fatal warning using ROS_WARN (or ROS_WARN_STREAM) that includes the offending line number and/or a short excerpt of the line content before continuing to skip it. Locate the catch (...) in the function that reads/parses lines (the parsing loop in PositionHistory.h), access the loop's line index and the buffer/string being parsed (e.g., the variable named line or similar), and emit a warning like "Malformed line N: '...'" (truncate long lines), then continue to preserve existing recovery behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/xbot_monitoring/src/EventHistory.h`:
- Around line 24-31: The persistent events file (events.jsonl) is unbounded
because appendToDisk(json_payload) always appends even when add() evicts old
entries from buffer_ (size bounded by max_size_), and the startup rescan (the
loader around lines 48-64) reads the entire file; change appendToDisk and the
eviction path so the on-disk history mirrors the in-memory bounded deque: when
buffer_.pop_front() removes an entry, remove or rotate that record on disk
(e.g., rewrite events.jsonl from current buffer_ or implement log
rotation/truncation) and ensure the startup loader reads only the bounded file;
update add(), appendToDisk(), and the startup load routine accordingly so disk
size stays capped with max_size_.
- Around line 16-19: The code crashes when event_history_max_size == 0 because
both loadFromDisk() and add() run eviction loops that call pop_front() on an
empty deque; fix by treating 0 as "disabled" or clamping before any eviction: in
init(size_t max_size) check if max_size == 0 and either set a disabled_ flag or
set max_size_ = 0 and skip loadFromDisk(); update loadFromDisk(), add(), and any
eviction logic (where you compare deque size to max_size_) to early-return when
max_size_ == 0 (or only perform while (max_size_ > 0 && deque.size() >
max_size_) pop_front()), so no pop_front() is called when history is disabled.
In `@src/lib/xbot_monitoring/src/PositionHistory.h`:
- Around line 97-104: persistSegment currently always appends to file_path_
(positions.jsonl) causing unbounded growth while closed_segments_ is capped by
max_segments_; change persistence to periodically rewrite the file to reflect
the in-memory ring buffer (or rotate logs): when closed_segments_.size() reaches
max_segments_ (or on a periodic threshold) acquire file_mutex_, write all
entries from closed_segments_ (using segmentToJson(seg).dump()) into a temporary
file, flush/close it, then atomically replace file_path_ (rename temp ->
file_path_); otherwise continue appending as before—ensure you use the same
file_mutex_ to serialize this truncate/replace and append logic so on-disk
contents match the bounded closed_segments_ buffer.
In `@src/mower_logic/src/mower_logic/mower_logic.cpp`:
- Line 247: The publishMowerEvent call is being invoked from command/request
paths (e.g., where `enabled` is a requested flag and inside `checkSafety()`),
causing repeated/incorrect MQTT lifecycle publishes; instead detect transitions
on the observed/actual state and emit events there. Move the
`publishMowerEvent("MOWING_STARTED"/"MOWING_STOPPED")` calls out of
command/safety paths and add edge-detection in the observed-state update logic:
store the previous observed mower mode/flags (e.g., observed enabled/mowing,
emergency, docking states), compare to the new observed state when
sensor/feedback or status messages arrive, and only call `publishMowerEvent`
when a true edge (false->true or true->false) occurs; apply the same change for
EMERGENCY_* and DOCKING publishes referenced in this file (also at the other
noted locations).
- Around line 350-357: The code reads area index, id, and name separately from
MowingBehavior::INSTANCE (via get_current_area, get_current_area_id,
get_current_area_name) causing data races and inconsistent combinations; change
MowingBehavior to provide an atomic snapshot accessor (e.g., a thread-safe
method that returns a small struct {int index; int id; std::string name;}
protected by the existing mutex) and update this call site to acquire that
snapshot once, compare snapshot.index to high_level_status.current_area, and
call publishMowerEvent("AREA_CHANGED", ...) using snapshot.id and snapshot.name;
ensure the snapshot method is used everywhere instead of the three separate
getters so string fields are accessed under the same lock.
- Around line 679-681: shutdownHandler currently calls publishMowerEvent and
ros::shutdown which are not async-signal-safe; change the handler to only set a
process-wide std::atomic<bool> (e.g. shutdown_requested) and return immediately,
then in the main thread or ROS spin loop detect that shutdown_requested was set
(use shutdown_requested.exchange(false, std::memory_order_relaxed) or similar to
ensure one-shot handling) and from the main thread call
publishMowerEvent("SHUTDOWN") and ros::shutdown(); update references in the code
to remove non-safe calls from shutdownHandler and use the atomic flag logic to
perform the actual shutdown sequence in normal thread context.
In `@src/mower_map/srv/GetMowingAreaSrv.srv`:
- Line 3: You added a new field `string area_id` to the service contract
`GetMowingAreaSrv`, which will change the generated service MD5 and break ABI
for prebuilt clients; instead, create a new service (e.g., `GetMowingAreaSrvV2`)
that contains the new `string area_id` field and keep the original
`GetMowingAreaSrv` unchanged so existing consumers remain compatible; update any
server and client code to use `GetMowingAreaSrvV2` where the new field is
required and ensure build/system manifests reference the new service name.
---
Nitpick comments:
In `@src/lib/xbot_monitoring/src/PositionHistory.h`:
- Around line 22-28: The init method currently assigns a hardcoded relative path
to file_path_ ("positions.jsonl"); change init (or overload it) to derive an
absolute path from a ROS parameter (e.g., fetch a param like
"position_history_path") with a sensible fallback such as the user's ROS home
directory or a package-specific data directory, expanding '~' and converting to
an absolute path (use std::filesystem::absolute or equivalent) before assigning
file_path_; ensure loadFromDisk() and startNewSegment() continue to use
file_path_ unchanged and that init documents/validates the resolved path.
- Around line 124-126: The catch-all in PositionHistory.h currently swallows
parsing errors; instead, log a non-fatal warning using ROS_WARN (or
ROS_WARN_STREAM) that includes the offending line number and/or a short excerpt
of the line content before continuing to skip it. Locate the catch (...) in the
function that reads/parses lines (the parsing loop in PositionHistory.h), access
the loop's line index and the buffer/string being parsed (e.g., the variable
named line or similar), and emit a warning like "Malformed line N: '...'"
(truncate long lines), then continue to preserve existing recovery behavior.
In `@src/lib/xbot_monitoring/src/xbot_monitoring.cpp`:
- Around line 151-153: current_high_level_state is subject to a data race
because it's written in high_level_status_callback and read in pose_callback
from different AsyncSpinner threads; change its type from uint8_t to
std::atomic<uint8_t> and include <atomic> at the top of the file, then update
any direct reads/writes (references to current_high_level_state) to use the
atomic operations (load/store) as needed to ensure thread-safe access.
- Around line 743-747: The code currently reads a single parameter named
event_history_max_size via paramNh.param and uses it to initialize both
event_history.init(...) and position_history.init(...); introduce clarity by
either adding a new parameter (e.g., position_history_max_size) and use
paramNh.param("position_history_max_size", <fallback>) to initialize
position_history.init(static_cast<size_t>(position_history_max_size)), or if
shared sizing is intentional add a brief comment next to the paramNh.param call
explaining that event_history_max_size intentionally controls both event_history
and position_history; update the initialization lines to use the appropriate
variable names (event_history_max_size vs position_history_max_size) and ensure
defaults remain sensible.
In `@src/lib/xbot_mqtt/include/xbot_mqtt/publish.h`:
- Around line 46-59: publishEvent currently builds payload with generated
"id","t","type" then calls payload.update(details), which allows details to
overwrite those injected fields; to prevent this, change the merge order or
guard keys so injected values from generateNanoId(), ros::Time::now().toSec(),
and the type parameter cannot be replaced by details: in publishEvent
(referencing generateNanoId(), publishEvent, payload.update, and publish) merge
details into payload in a way that preserves existing keys (e.g., only copy keys
from details that are not "id","t","type" or perform payload = details; then set
payload["id"]=generateNanoId(); payload["t"]=...; payload["type"]=type), and
then call publish(pub, "events/json", payload, retain).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8a42da9-2278-448a-a162-dafb87a4b27c
📒 Files selected for processing (26)
src/lib/xbot_monitoring/CMakeLists.txtsrc/lib/xbot_monitoring/package.xmlsrc/lib/xbot_monitoring/src/EventHistory.hsrc/lib/xbot_monitoring/src/PositionHistory.hsrc/lib/xbot_monitoring/src/capabilities.hsrc/lib/xbot_monitoring/src/xbot_monitoring.cppsrc/lib/xbot_mqtt/CMakeLists.txtsrc/lib/xbot_mqtt/include/xbot_mqtt/constants.hsrc/lib/xbot_mqtt/include/xbot_mqtt/provider.hsrc/lib/xbot_mqtt/include/xbot_mqtt/publish.hsrc/lib/xbot_mqtt/msg/MqttPublish.msgsrc/lib/xbot_mqtt/msg/RpcError.msgsrc/lib/xbot_mqtt/msg/RpcRequest.msgsrc/lib/xbot_mqtt/msg/RpcResponse.msgsrc/lib/xbot_mqtt/package.xmlsrc/lib/xbot_mqtt/src/provider.cppsrc/lib/xbot_mqtt/srv/RegisterMethodsSrv.srvsrc/mower_logic/CMakeLists.txtsrc/mower_logic/package.xmlsrc/mower_logic/src/mower_logic/behaviors/MowingBehavior.cppsrc/mower_logic/src/mower_logic/behaviors/MowingBehavior.hsrc/mower_logic/src/mower_logic/mower_logic.cppsrc/mower_map/CMakeLists.txtsrc/mower_map/package.xmlsrc/mower_map/src/mower_map_service.cppsrc/mower_map/srv/GetMowingAreaSrv.srv
| // status change ? | ||
| const auto last_status = status_state_subscriber.getMessage(); | ||
| if (last_status.mow_enabled != enabled) { | ||
| publishMowerEvent(enabled ? "MOWING_STARTED" : "MOWING_STOPPED"); |
Contributor
There was a problem hiding this comment.
Emit MQTT lifecycle events from confirmed state edges.
These publishes are currently driven by requested state / level-triggered safety conditions. checkSafety() can re-enter them every 0.5s while services retry, feedback lags, or an abort is still in flight, so history can record duplicates or transitions that never actually happened. Please emit MOWING_*, EMERGENCY_*, and DOCKING from observed state edges instead of these command paths.
Also applies to: 324-324, 593-594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mower_logic/src/mower_logic/mower_logic.cpp` at line 247, The
publishMowerEvent call is being invoked from command/request paths (e.g., where
`enabled` is a requested flag and inside `checkSafety()`), causing
repeated/incorrect MQTT lifecycle publishes; instead detect transitions on the
observed/actual state and emit events there. Move the
`publishMowerEvent("MOWING_STARTED"/"MOWING_STOPPED")` calls out of
command/safety paths and add edge-detection in the observed-state update logic:
store the previous observed mower mode/flags (e.g., observed enabled/mowing,
emergency, docking states), compare to the new observed state when
sensor/feedback or status messages arrive, and only call `publishMowerEvent`
when a true edge (false->true or true->false) occurs; apply the same change for
EMERGENCY_* and DOCKING publishes referenced in this file (also at the other
noted locations).
rovo89
force-pushed
the
feat/events-and-position
branch
2 times, most recently
from
898c3ad to
5b153cc
Compare
Comment on lines
+526
to
+528
| position_history.addPoint(msg->pose.pose.position.x, | ||
| msg->pose.pose.position.y, | ||
| msg->header.stamp); |
Owner
There was a problem hiding this comment.
Since you have the state and the AbsolutePose anyways we could also store both.
This would give us:
- Indication what the mower was doing at the moment (travel lines vs mow lines)
- Indication in which areas the position was good / bad so that we can optimize issues.
Probably make this generic, so that we could color the mower's historic positions for generic things (e.g. wifi signal strength etc) and have a dropdown in the GUI for that.
Collaborator
Author
There was a problem hiding this comment.
Yes, that would also be an option. Just thought I'd keep the message size minimal since those message will come pretty frequently, and the state that xbot_monitoring has might be slightly delayed, being sent only at 1 Hz. Might need another round of thinking to get this right. Not sure if you've seen it, for the history I have the state per "segment", starting a new one periodically or whenever the state changes.
rovo89
force-pushed
the
feat/events-and-position
branch
from
8af8ddd to
10fdb7a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores