Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a HAL runtime (RobotHAL + RobotInstance) with a driver state machine (DriverBase), nests PD controller configs into controller-specific Config types, renames/refactors the MuJoCo driver and example, adds magic_enum and rules_cc module/build integrations, and introduces HAL unit tests and CI dependency steps. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant RI as RobotInstance
participant HAL as RobotHAL
participant DB as DriverBase
participant RD as RobotDescription
App->>RI: RobotInstance(model_path, drivers, controllers)
RI->>HAL: construct(model_path, drivers)
HAL->>RD: load model_path
HAL->>DB: validateDriverCoverage()
HAL-->>RI: construction complete
App->>RI: start()
RI->>HAL: startDrivers()
HAL->>DB: (each) start() -> transitionTo(CONFIGURED/READY)
DB->>DB: isLegalTransition()
loop control loop
App->>RI: tick/update
RI->>HAL: update(action, robot_state)
HAL->>DB: setJointFeedbackAction(action)
HAL->>DB: updateRobotStateImpl(robot_state)
DB-->>HAL: updated state
end
App->>RI: stop()
RI->>HAL: stopDrivers()
HAL->>DB: (each) stop() -> transitionTo(STOPPING)
HAL-->>RI: stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
motorium/motorium_control/include/motorium_control/JointPDController.h (1)
38-65:⚠️ Potential issue | 🟠 MajorRemove or wire the duplicate
Configtype.There are now two public config structs, but
JointPDControlleraccepts only the nested one and the namespace-scopeConfig::validate()has no visible implementation. This can create confusing compile/link failures for users constructingmotorium::control::Config.♻️ Proposed cleanup if the nested config is intended
-struct Config { - std::vector<std::string> joint_names; - vector_t kp; - vector_t kd; - - void validate() const; -}; - /* `@brief` Implicit PD Controller does not compute the feedback terms itself but forwards gains and setpoints to be evaluated at the driver. This way🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_control/include/motorium_control/JointPDController.h` around lines 38 - 65, There are two conflicting Config types (the namespace-scope Config with validate() and the nested JointPDController::Config with constructor and validate()), which causes ambiguity and dead declarations; remove the duplicate top-level struct Config (or if the top-level was intended, move the nested Config implementation there) and ensure only JointPDController::Config remains public and fully defined: delete the namespace-scope struct and any forward-declared validate() for it, keep the nested Config declaration (std::vector<std::string> joint_names; vector_t kp; vector_t kd; Config(); Config(std::vector<std::string>, std::vector<double>, std::vector<double>); void validate() const;) and implement its validate() in the corresponding .cpp so users construct motorium::control::JointPDController<...>::Config unambiguously.motorium/motorium_control/src/JointPDController.cpp (1)
43-49:⚠️ Potential issue | 🟠 MajorValidate gain values, not just gain sizes.
kp/kdcan currently contain negative, NaN, or infinite values and still reach Lines 81-82 and 100, which can invert or poison feedback commands.🛡️ Proposed validation tightening
void JointPDController<IsImplicit>::Config::validate() const { MT_CHECK(!joint_names.empty()) << "[JointPDController] Configuration error: Joint names list is empty."; MT_CHECK(static_cast<long>(joint_names.size()) == kp.size()) << "[JointPDController] Configuration error: Size mismatch between joint_names and kp."; MT_CHECK(static_cast<long>(joint_names.size()) == kd.size()) << "[JointPDController] Configuration error: Size mismatch between joint_names and kd."; + for (Eigen::Index i = 0; i < kp.size(); ++i) { + MT_CHECK(std::isfinite(kp(i)) && kp(i) >= 0.0) + << "[JointPDController] Configuration error: kp must be finite and non-negative."; + MT_CHECK(std::isfinite(kd(i)) && kd(i) >= 0.0) + << "[JointPDController] Configuration error: kd must be finite and non-negative."; + } }Also add
#include <cmath>if it is not already available transitively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_control/src/JointPDController.cpp` around lines 43 - 49, The validate() in JointPDController<IsImplicit>::Config currently only checks sizes; update it to also iterate over kp and kd and MT_CHECK each element is finite (use std::isfinite) and non-negative (>= 0) to reject negative, NaN, or infinite gains, emitting an informative message that includes the offending index and value and references kp/kd/joint_names; also add `#include` <cmath> if not already present so std::isfinite is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dependencies.txt`:
- Line 14: Remove the invalid system package entry "ros-jazzy-magic-enum" from
dependencies.txt (delete the line containing that token) because it breaks
apt-get; if the Colcon/ROS path actually needs a system-provided magic_enum, add
a build dependency in the ROS package manifests instead by adding a
<build_depend>magic_enum</build_depend> to motorium_hal/package.xml and
motorium_mujoco/package.xml (do not reintroduce the apt package into
dependencies.txt).
In `@motorium/examples/arx/arm_mujoco_sim.cpp`:
- Around line 75-95: desired_state is default-constructed and never updated
before being passed to controller.computeJointControlAction, so the example
holds the arm at the zero pose indefinitely; either populate desired_state
(e.g., call setJointState or equivalent on model::RobotState to set q/v/τ_ff)
before the loop to command a non-zero reference or add a one-line comment
clarifying the zero-reference is intentional. Also replace the infinite
while(true) with a stoppable condition (or install a signal handler) so
sim.stop() and return 0 can be reached for graceful shutdown.
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h`:
- Around line 96-103: transitionTo currently does a separate load and store
which can race; change it to a compare-and-exchange loop using
state_.compare_exchange_strong so the entire transition is atomic. Implement:
loop reading an expected (e.g., DriverState expected = state_.load()), check
isLegalTransition(expected, next) against that expected, then attempt
state_.compare_exchange_strong(expected, next); if it returns true return,
otherwise retry with the updated expected and re-check legality and repeat; if
legality fails throw the same runtime_error containing name_ and
toString(current). Use the existing symbols transitionTo, state_,
isLegalTransition, name_, and toString.
- Around line 77-81: The DriverBase subset constructor currently ignores the
robot_description allowing invalid joint names; update the DriverBase(const
model::RobotDescription& robot_description, const std::string& name,
std::vector<std::string> managed_joint_names) to validate each entry in
managed_joint_names_ against the robot_description's declared joints (e.g.,
using the RobotDescription API to obtain the set of valid joint names), and if
any managed_joint_names_ entry is missing/invalid, fail fast by throwing an
exception or returning an error (or logging and aborting) with a clear message
including the invalid name(s); keep robot_description non-stored but perform
this setup-time check inside the constructor before assigning
managed_joint_names_.
In `@motorium/motorium_hal/include/motorium_hal/RobotHAL.h`:
- Around line 90-101: The current getState() method can return HalState::ACTIVE
when drivers_ is empty because all_running stays true; modify getState() in
RobotHAL.h to first check if drivers_.empty() and immediately return
HalState::UNCONFIGURED, otherwise proceed with the existing loop; reference the
getState() method and drivers_ member and ensure you return
HalState::UNCONFIGURED (not ACTIVE) for an empty driver set so that ACTIVE is
only returned when at least one driver reports RUNNING.
- Around line 60-72: In RobotHAL constructor, validate drivers_ before using
them: iterate drivers_ and throw if any element is null (reject nullptr in
drivers vector), and while collecting managed joint names use a map or
unordered_map from joint name to owner (e.g., pointer/index of driver) instead
of a set so you can detect duplicates; if a joint is already owned by another
driver, throw a runtime_error identifying the duplicate joint and the
conflicting drivers (use driver identity like pointer or index and the joint
name). This preserves the existing robot_description_.getJointNames() check but
prevents null derefs and overlapping ownership that would cause process() to
forward conflicting commands.
In `@motorium/motorium_mujoco/CMakeLists.txt`:
- Line 16: The CMakeLists links magic_enum via magic_enum::magic_enum in the
target_link_libraries(${PROJECT_NAME} call but does not include it in the
exported set(dependencies ...) block; either add magic_enum to that dependencies
list (if it should be a public dependency) or change the target_link_libraries
invocation to use keyworded private linkage (e.g., replace the keyword-less link
of magic_enum::magic_enum with a PRIVATE link) so the link interface and
exported dependencies are consistent; update the occurrence around
target_link_libraries(${PROJECT_NAME} and the set(dependencies ...) block
accordingly.
In `@motorium/motorium_runtime/BUILD.bazel`:
- Around line 3-14: Add a cc_test target for the new motorium_runtime library to
provide coverage for RobotInstance (validate the constructor and that start/stop
delegate to the HAL/control as expected); create a test target (e.g., name =
"robot_instance_test") in the same BUILD.bazel that links against
:motorium_runtime and any test harness deps, and implement unit tests that
exercise RobotInstance's constructor, RobotInstance::start and
RobotInstance::stop behaviors (mock or fake motorium_hal/motorium_control as
needed) to ensure proper wiring and delegation from
RobotInstance.cpp/RobotInstance.h.
In `@motorium/motorium_runtime/include/motorium_runtime/CommandManager.h`:
- Around line 1-28: CommandManager.h is an empty public header with no
declarations or include guard; either delete this header from the public include
set until you actually add an API, or implement the intended CommandManager
interface now: add a proper include guard or `#pragma` once, declare the
CommandManager class/namespace and its public methods/types (e.g. class
CommandManager { public: /* ctor/dtor, register/execute methods */ };), and
export any needed symbols so the header is non-empty and usable by consumers.
In `@motorium/motorium_runtime/include/motorium_runtime/RobotInstance.h`:
- Around line 41-53: RobotInstance currently allows implicit copying which would
shallow-copy hal::RobotHAL and the controllers_ shared_ptrs, risking
double-start/stop; explicitly delete the copy constructor and copy assignment
(e.g., RobotInstance(const RobotInstance&) = delete; RobotInstance&
operator=(const RobotInstance&) = delete;) and optionally add move semantics if
desired, and implement a destructor ~RobotInstance() that calls stop() to ensure
RAII cleanup of the HAL/drivers; update declarations in the RobotInstance class
(referencing RobotInstance, hal::RobotHAL, controllers_, start(), stop(), and
~RobotInstance()) accordingly.
In `@motorium/motorium_runtime/src/RobotInstance.cpp`:
- Around line 34-45: controllers_ is stored but never used: implement a control
orchestration or document it. Either add a dedicated control loop started in
RobotInstance::start() (spawn a thread, set a running flag, iterate calling each
controller's computeJointControlAction(...) and pass results into
robot_hal_.process(...), and stop/join the thread in RobotInstance::stop()), or
explicitly annotate with a TODO comment in the constructor/start noting
controllers are not yet ticked. Also add start/stop guards (e.g., a bool
running_ or atomic to prevent double-start and make stop() idempotent and safe
from destructor paths) and ensure controllers_ and robot_hal_ interactions use
those guards.
In `@patches/magic_enum_module.patch`:
- Line 8: The patch adds a bazel_dep entry for rules_cc pinned to "0.2.1" which
mismatches the top-level/parent pin "0.2.16"; update the bazel_dep with the
rules_cc entry (the line containing bazel_dep(name = "rules_cc", version =
"...")) to use the same version as the parent (0.2.16) so the module's
MODULE.bazel matches the parent pin and avoids confusing or stale overrides.
---
Outside diff comments:
In `@motorium/motorium_control/include/motorium_control/JointPDController.h`:
- Around line 38-65: There are two conflicting Config types (the namespace-scope
Config with validate() and the nested JointPDController::Config with constructor
and validate()), which causes ambiguity and dead declarations; remove the
duplicate top-level struct Config (or if the top-level was intended, move the
nested Config implementation there) and ensure only JointPDController::Config
remains public and fully defined: delete the namespace-scope struct and any
forward-declared validate() for it, keep the nested Config declaration
(std::vector<std::string> joint_names; vector_t kp; vector_t kd; Config();
Config(std::vector<std::string>, std::vector<double>, std::vector<double>); void
validate() const;) and implement its validate() in the corresponding .cpp so
users construct motorium::control::JointPDController<...>::Config unambiguously.
In `@motorium/motorium_control/src/JointPDController.cpp`:
- Around line 43-49: The validate() in JointPDController<IsImplicit>::Config
currently only checks sizes; update it to also iterate over kp and kd and
MT_CHECK each element is finite (use std::isfinite) and non-negative (>= 0) to
reject negative, NaN, or infinite gains, emitting an informative message that
includes the offending index and value and references kp/kd/joint_names; also
add `#include` <cmath> if not already present so std::isfinite is available.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 296a57c5-248d-4212-919a-6b76be7d7152
⛔ Files ignored due to path filters (1)
MODULE.bazel.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
MODULE.bazelREADME.mddependencies.txtmotorium/examples/arx/BUILD.bazelmotorium/examples/arx/arm_mujoco_sim.cppmotorium/motorium_control/include/motorium_control/JointPDController.hmotorium/motorium_control/src/JointPDController.cppmotorium/motorium_control/test/testExplicitJointPDController.cppmotorium/motorium_control/test/testImplicitJointPDController.cppmotorium/motorium_hal/BUILD.bazelmotorium/motorium_hal/CMakeLists.txtmotorium/motorium_hal/include/motorium_hal/DriverBase.hmotorium/motorium_hal/include/motorium_hal/RobotHAL.hmotorium/motorium_mujoco/CMakeLists.txtmotorium/motorium_runtime/BUILD.bazelmotorium/motorium_runtime/include/motorium_runtime/CommandManager.hmotorium/motorium_runtime/include/motorium_runtime/RobotInstance.hmotorium/motorium_runtime/src/RobotInstance.cpppatches/magic_enum_build.patchpatches/magic_enum_module.patch
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/colcon_build_test.yml (1)
29-35: 🧹 Nitpick | 🔵 TrivialInclude
dependencies_ros2.txtin the APT cache key.The cache key only hashes
dependencies.txt. When you add/remove ROS 2 packages independencies_ros2.txtwithout touchingdependencies.txt, the cache will not be invalidated, risking stale/inconsistent APT state between runs.🔧 Proposed fix
- key: ${{ runner.os }}-apt-${{ hashFiles('dependencies.txt') }} + key: ${{ runner.os }}-apt-${{ hashFiles('dependencies.txt', 'dependencies_ros2.txt') }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/colcon_build_test.yml around lines 29 - 35, The APT cache key currently only hashes 'dependencies.txt' which can leave the cache stale when 'dependencies_ros2.txt' changes; update the "Cache APT packages" step (identified by the name "Cache APT packages" and the key expression starting with ${{ runner.os }}-apt-${{ hashFiles('dependencies.txt') }}) to include 'dependencies_ros2.txt' in the hashFiles call (so the key incorporates both files referencing /var/cache/apt) so changes to ROS2 package lists invalidate the cache appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@motorium/motorium_hal/CMakeLists.txt`:
- Around line 62-70: The ament_target_dependencies calls for test_driver_base
and test_robot_hal incorrectly include the legacy package name "absl" even
though Abseil is found via CONFIG and should be consumed via namespaced targets;
remove "absl" from both ament_target_dependencies(...) invocations and keep the
Abseil namespaced targets (e.g. absl::log, absl::check, absl::flat_hash_map) in
the corresponding target_link_libraries(...) lines so the tests still link
Abseil properly.
In `@motorium/motorium_hal/include/motorium_hal/RobotHAL.h`:
- Around line 32-57: The header is missing the <string_view> include used by
toString(HalState) which returns std::string_view; add `#include` <string_view> at
the top of RobotHAL.h so std::string_view is declared (affects the enum HalState
and the inline function toString(HalState)).
In `@motorium/motorium_hal/package.xml`:
- Around line 11-13: package.xml is missing the magic_enum build dependency
declared in CMakeLists.txt and headers; add a
<build_depend>magic_enum</build_depend> entry to the existing build_depend block
(where absl_vendor, motorium_core, motorium_model are declared) so rosdep/colcon
can resolve the rosdep key used in dependencies_ros2.txt and satisfy
find_package(magic_enum REQUIRED) and the `#include` <magic_enum/magic_enum.hpp>
usage.
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 80-86: Replace the fixed shared temp_dir_ in SetUp/TearDown with a
per-test unique directory to avoid races: in SetUp(), build temp_dir_ using
std::filesystem::temp_directory_path() combined with
std::filesystem::unique_path (e.g. std::filesystem::temp_directory_path() /
std::filesystem::unique_path("robot_hal_test-%%%%-%%%%")), then
create_directories(temp_dir_) and call writeTempXml(temp_dir_, ... ) as before;
leave TearDown() calling std::filesystem::remove_all(temp_dir_) so it only
deletes this test's unique directory. Ensure the change references the existing
SetUp, TearDown, temp_dir_, and writeTempXml symbols.
- Around line 121-123: The test currently throws from RobotDescription inside
makeDriver because duplicate JointDescription names are provided; change the
test to build a RobotDescription (or call makeDriver) with unique joint names,
then explicitly invoke DriverBase's subset constructor (or the
DriverBase-derived constructor used in makeDriver) passing a managed_joint_names
vector that contains duplicates so the exception is thrown by
DriverBase/RobotHAL; reference DriverBase, the subset constructor that accepts
managed_joint_names, makeDriver, and managed_joint_names in the test and assert
EXPECT_THROW(..., std::invalid_argument) when constructing the driver with
duplicate managed_joint_names.
---
Outside diff comments:
In @.github/workflows/colcon_build_test.yml:
- Around line 29-35: The APT cache key currently only hashes 'dependencies.txt'
which can leave the cache stale when 'dependencies_ros2.txt' changes; update the
"Cache APT packages" step (identified by the name "Cache APT packages" and the
key expression starting with ${{ runner.os }}-apt-${{
hashFiles('dependencies.txt') }}) to include 'dependencies_ros2.txt' in the
hashFiles call (so the key incorporates both files referencing /var/cache/apt)
so changes to ROS2 package lists invalidate the cache appropriately.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0db804f4-45f8-4204-8ad4-c23f7dd66aac
📒 Files selected for processing (10)
.github/workflows/colcon_build_test.ymldependencies_ros2.txtmotorium/motorium_hal/BUILD.bazelmotorium/motorium_hal/CMakeLists.txtmotorium/motorium_hal/include/motorium_hal/DriverBase.hmotorium/motorium_hal/include/motorium_hal/RobotHAL.hmotorium/motorium_hal/package.xmlmotorium/motorium_hal/test/testDriverBase.cppmotorium/motorium_hal/test/testRobotHAL.cppmotorium/motorium_runtime/include/motorium_runtime/RobotInstance.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
motorium/motorium_mujoco/src/MujocoDriver.cpp (2)
113-117:⚠️ Potential issue | 🟠 MajorDestructor calls
stop(), which (once it transitions state) may trigger illegal transitions from terminal states.After the state-machine hookup above,
~MujocoDriver()→stop()is problematic when the driver is already inUNINITIALIZED,FAULT, or other non-RUNNINGstates:MT_CHECKintransitionTowill abort the process during destruction. Guard the transitions instop()on the current state (as shown in thegetState() == RUNNINGguard suggested above), or makestop()a no-op when there is no active thread and the state isn'tRUNNING/STOPPING.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_mujoco/src/MujocoDriver.cpp` around lines 113 - 117, Destructor MujocoDriver::~MujocoDriver() calls stop() unconditionally which can invoke transitionTo and MT_CHECK when the driver is already in terminal states (UNINITIALIZED, FAULT), so change stop() or the destructor to guard against illegal state transitions: in MujocoDriver::~MujocoDriver() either check getState() == RUNNING (or getState() == STOPPING) before calling stop(), or modify stop() to be a no-op when current state is not RUNNING/STOPPING (use getState() and short-circuit to avoid calling transitionTo), ensuring MT_CHECK in transitionTo cannot abort during destruction; reference functions: MujocoDriver::~MujocoDriver(), stop(), transitionTo(), getState() and states RUNNING, STOPPING, UNINITIALIZED, FAULT, MT_CHECK.
439-455:⚠️ Potential issue | 🟠 Major
MujocoDriverinheritsDriverBase's state machine but never callstransitionTo, soRobotHAL::getState()will misreport this driver.
DriverBase::state_defaults toUNINITIALIZEDand is only advanced viatransitionTo().MujocoDriver::start()/stop()(andreset()) never transition, so the aggregatedHalStatecomputed inmotorium/motorium_hal/src/RobotHAL.cppwill always fall through toUNCONFIGUREDeven after the sim thread is running, andFAULT/shutdown will never be observed. Per the legal transitions inDriverBase.cpp,start()needs to walkUNINITIALIZED → CONFIGURED → READY → RUNNING, andstop()needs to goRUNNING → STOPPING → READY(or→ FAULTon error). Without this, the whole HAL lifecycle machinery introduced in this PR is bypassed for the only concrete driver.🔧 Minimum viable hookup
void MujocoDriver::initSim() { simulationStep(); sim_initialized_ = true; + transitionTo(DriverState::CONFIGURED); if (!config_.headless) { renderer_.reset(new MujocoRenderer(this)); renderer_->launchRenderThread(); } + transitionTo(DriverState::READY); } void MujocoDriver::start() { if (!sim_initialized_) initSim(); if (simulate_thread_.joinable()) { std::cerr << "WARNING: Tried to start simulation thread, but it is already running." << std::endl; return; } simulate_thread_ = std::jthread([this](std::stop_token st) { this->simulationLoop(st); }); + transitionTo(DriverState::RUNNING); } void MujocoDriver::stop() { + if (getState() == DriverState::RUNNING) { + transitionTo(DriverState::STOPPING); + } if (simulate_thread_.joinable()) { simulate_thread_.request_stop(); if (simulate_thread_.get_id() != std::this_thread::get_id()) { simulate_thread_.join(); } } + if (getState() == DriverState::STOPPING) { + transitionTo(DriverState::READY); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_mujoco/src/MujocoDriver.cpp` around lines 439 - 455, MujocoDriver currently never updates DriverBase::state_, so RobotHAL reports UNINITIALIZED/UNCONFIGURED; modify MujocoDriver::start() to call transitionTo() to advance through the legal sequence (UNINITIALIZED → CONFIGURED → READY → RUNNING) before/after launching simulate_thread_ (use transitionTo(CONFIGURED), transitionTo(READY), then launch thread and transitionTo(RUNNING)), and modify MujocoDriver::stop() to perform the RUNNING → STOPPING → READY transitions (call transitionTo(STOPPING), request_stop()/join thread, then transitionTo(READY)), and on any error during start/stop transition to FAULT via transitionTo(FAULT); also ensure reset() follows the appropriate transitions when implemented. Include calls to transitionTo using the existing transitionTo() method names so the HAL lifecycle reflects the actual thread state.
♻️ Duplicate comments (2)
motorium/motorium_hal/CMakeLists.txt (1)
32-39:⚠️ Potential issue | 🟡 Minor
abslindependenciespropagates the ament-target-deps issue to every downstream consumer viaament_export_dependencies.Past review already flagged
ament_target_dependencies(test_... absl)as incorrect sinceabslis a pure CMake CONFIG package with no legacy<pkg>_INCLUDE_DIRS/<pkg>_LIBRARIES. The same problem is replicated at Line 93 (ament_export_dependencies(${dependencies})), which exportsabslto downstream packages. Downstream packages that consumemotorium_halvia ament will then inherit the same brokenament_target_dependenciespath.Drop
abslfrom thedependenciesset and instead have each downstream target dofind_package(absl CONFIG REQUIRED)+target_link_libraries(... absl::…)directly, as is already done inside this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_hal/CMakeLists.txt` around lines 32 - 39, Remove 'absl' from the dependencies list variable so it is not exported to downstream packages (the variable named "dependencies" that is later used in ament_export_dependencies(${dependencies})); instead rely on local find_package(absl CONFIG REQUIRED) and explicit target_link_libraries(... absl::...) for targets that need Abseil (as already done in this CMakeLists.txt), and ensure no other ament_target_dependencies or ament_export_dependencies entries include 'absl'.motorium/motorium_hal/include/motorium_hal/RobotHAL.h (1)
32-43:⚠️ Potential issue | 🟡 MinorAdd the direct
<string_view>include.
toString(HalState)exposesstd::string_view, so this public header should include the declaring standard header explicitly instead of relying on transitive includes.🛠️ Proposed fix
`#include` <memory> `#include` <stdexcept> `#include` <string> +#include <string_view> `#include` <unordered_set> `#include` <vector>Read-only verification:
#!/bin/bash # Verify RobotHAL.h directly uses std::string_view and whether it explicitly includes <string_view>. fd -a 'RobotHAL.h' | xargs -I{} sh -c ' echo "--- {} ---" sed -n "30,60p" "{}" echo grep -n "#include <string_view>" "{}" || true 'Also applies to: 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_hal/include/motorium_hal/RobotHAL.h` around lines 32 - 43, The public header RobotHAL.h exposes std::string_view via the toString(HalState) declaration but does not directly include <string_view>; add a direct `#include` <string_view> at the top of RobotHAL.h (near the other standard includes) so the header does not rely on transitive includes and ensure the declaration of toString(HalState) compiles when this file is included on its own.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h`:
- Around line 62-65: The full-coverage DriverBase constructor currently copies
all joint names into managed_joint_names_ and then calls
validateManagedJointNames(robot_description) redundantly; remove that call (or
guard it) in the DriverBase(const model::RobotDescription&, const std::string&)
overload so names copied from robot_description are not re-validated. Locate the
constructor in class DriverBase and either delete the
validateManagedJointNames(...) invocation or add a conditional that only calls
validateManagedJointNames when managed_joint_names_ represents a subset (e.g.,
compare sizes or add an explicit flag), leaving validation intact for the
subset-based constructor/path.
- Around line 61-71: The two DriverBase constructors have inconsistent parameter
ordering (one is DriverBase(const model::RobotDescription&, const std::string&
name) and the other is DriverBase(const model::RobotDescription&,
std::vector<std::string> managed_joint_names, const std::string& name)), so
change the subset overload to place name in the same position as the
full-coverage ctor by using DriverBase(const model::RobotDescription&
robot_description, const std::string& name, std::vector<std::string>
managed_joint_names) and update the initializer to set name_(name) and
managed_joint_names_(std::move(managed_joint_names)), keeping the call to
validateManagedJointNames(robot_description); then update any call sites and
subclass forwarding that relied on the old parameter order (e.g., tests in
testDriverBase.cpp and any subclass constructors forwarding to DriverBase).
- Around line 53-55: Document the lifetime guarantee for the string_view
returned by toString(DriverState) by adding a brief comment above the inline
function stating that the std::string_view returned by
magic_enum::enum_name(state) points to static storage and is valid for the
program lifetime; reference the toString function, the DriverState enum, and
magic_enum::enum_name so future maintainers know the current implementation
relies on static storage and to avoid replacing it with computed strings without
updating callers or the contract.
In `@motorium/motorium_hal/src/DriverBase.cpp`:
- Around line 34-51: Update the transition matrix in isLegalTransition to allow
the missing real-world paths: in the DriverState::STOPPING case return true for
to == DriverState::READY || to == DriverState::FAULT (add STOPPING→FAULT), and
in the DriverState::RUNNING case allow to == DriverState::STOPPING || to ==
DriverState::FAULT || to == DriverState::READY (add RUNNING→READY). Also revisit
the DriverState::FAULT case and, if recovery semantics require it, document or
broaden permitted exits (currently to == DriverState::CONFIGURED) so fault
recovery can be expressed without MT_CHECK aborts.
- Around line 53-62: DriverBase::transitionTo currently checks isLegalTransition
against the evolving atomic state inside the CAS loop which can abort on benign
races; change it to check legality only against the originally observed state
and avoid re-evaluating after the first failed CAS. Concretely: read and store
an initial variable (e.g. observed = state_.load()), call
MT_CHECK(isLegalTransition(observed, next)) once (so illegal-at-issue-time
transitions still abort), then attempt the compare_exchange_weak using expected
initialized to observed; if the CAS succeeds return, but if it fails and
expected != observed (meaning another thread changed the state) treat that as a
benign concurrent transition and return (or return false) instead of
re-evaluating legality; do not call isLegalTransition again inside the loop. Use
the symbols DriverBase::transitionTo, state_, isLegalTransition, MT_CHECK and
compare_exchange_weak to locate and apply this change.
- Around line 64-68: The method DriverBase::validateManagedJointNames currently
only calls robot_description.validateName for each entry and misses duplicates
in managed_joint_names_; update it to detect duplicates by creating a
std::unordered_set<std::string> (e.g., named seen) and for each joint_name
attempt to insert it, and if insertion fails throw a descriptive exception
(std::invalid_argument or std::runtime_error) identifying the duplicate joint
name so the driver fails fast; after confirming uniqueness, continue to call
robot_description.validateName(joint_name). Remember to add `#include`
<unordered_set>.
In `@motorium/motorium_hal/src/RobotHAL.cpp`:
- Around line 43-50: RobotHAL::update currently iterates drivers_ and calls
setJointFeedbackAction/updateRobotState unconditionally; add a precondition that
checks this->getState() and only proceeds when it equals HalState::ACTIVE
(otherwise return early and optionally log a warning), so the command path is
gated by the HAL state; apply this check at the top of RobotHAL::update before
iterating drivers_ (references: RobotHAL::update, drivers_,
setJointFeedbackAction, updateRobotState, getState(), HalState::ACTIVE).
- Around line 52-64: RobotHAL::getState currently collapses
DriverState::STOPPING and DriverState::READY into HalState::CONFIGURED; update
getState to surface these distinct states instead of folding them: either extend
HalState (add READY and STOPPING) and return HalState::STOPPING if any
driver->getState() == DriverState::STOPPING (else return HalState::READY if any
== DriverState::READY), then fall back to existing
ACTIVE/CONFIGURED/UNCONFIGURED/FAULT logic, or implement a "worst-state wins"
policy (check for FAULT, then STOPPING, then UNINITIALIZED, then READY, then
RUNNING) and return the corresponding HalState; adjust the HalState enum and
callers of getState accordingly to handle the new values.
- Around line 78-93: The validateDriverCoverage method currently throws
std::runtime_error for null drivers and duplicate joints but uses MT_CHECK
(process-abort) for uncovered joints; make the behavior consistent by replacing
the MT_CHECK in RobotHAL::validateDriverCoverage with a throw std::runtime_error
carrying the same message, so uncovered joints (from
robot_description_.getJointNames()) produce a catchable exception like the other
error cases; ensure the message matches the existing MT_CHECK text and reference
drivers_, driver->getManagedJointNames(), and robot_description_.getJointNames()
to locate the check.
In `@motorium/motorium_hal/test/testDriverBase.cpp`:
- Around line 42-43: The TestDriver's stop() currently forces
transitionTo(DriverState::FAULT) which violates the DriverBase state machine;
change stop() so it does not jump straight to FAULT—either make stop() a no-op
or, better, check the current state and only transitionTo(DriverState::STOPPING)
when in RUNNING (otherwise do nothing). Update the stop() implementation
(referencing stop(), start(), transitionTo(), and
DriverState::{RUNNING,STOPPING,FAULT,UNINITIALIZED}) so normal shutdown follows
RUNNING → STOPPING → READY and calling stop() from UNINITIALIZED/STOPPING/FAULT
is safe.
- Around line 118-122: The EXPECT_DEATH assertion in TEST_F(DriverBaseTest,
IllegalTransitionKills) is using an empty regex which is too permissive; change
the expectation to match the specific MT_CHECK failure message emitted when
TestDriver::triggerTransition(DriverState::RUNNING) performs an illegal
transition (the MT_CHECK text that mentions illegal state transition or the
driver state), e.g. update the EXPECT_DEATH call for TestDriver in
DriverBaseTest to include a regex matching the MT_CHECK message so only the
intended check-triggered abort passes.
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 226-239: Test invokes RobotHAL::update while drivers remain
UNINITIALIZED; either enforce active state or drive drivers to RUNNING before
calling update. Modify the test (RobotHALTest::updateCallsAllDrivers) to assert
hal.getState() == HalState::ACTIVE and update RobotHAL::update to no-op or error
unless getState() == HalState::ACTIVE, or alternatively call
RobotHAL::triggerTransition (or use the existing triggerTransition helper) to
move drivers to RUNNING before hal.update(action, state) and then keep the
existing EXPECT_EQ checks for d1/d2 action_count_ and update_count_. Ensure
references to RobotHAL::update, RobotHAL::getState, HalState::ACTIVE,
triggerTransition, and makeDriver are used to locate the changes.
---
Outside diff comments:
In `@motorium/motorium_mujoco/src/MujocoDriver.cpp`:
- Around line 113-117: Destructor MujocoDriver::~MujocoDriver() calls stop()
unconditionally which can invoke transitionTo and MT_CHECK when the driver is
already in terminal states (UNINITIALIZED, FAULT), so change stop() or the
destructor to guard against illegal state transitions: in
MujocoDriver::~MujocoDriver() either check getState() == RUNNING (or getState()
== STOPPING) before calling stop(), or modify stop() to be a no-op when current
state is not RUNNING/STOPPING (use getState() and short-circuit to avoid calling
transitionTo), ensuring MT_CHECK in transitionTo cannot abort during
destruction; reference functions: MujocoDriver::~MujocoDriver(), stop(),
transitionTo(), getState() and states RUNNING, STOPPING, UNINITIALIZED, FAULT,
MT_CHECK.
- Around line 439-455: MujocoDriver currently never updates DriverBase::state_,
so RobotHAL reports UNINITIALIZED/UNCONFIGURED; modify MujocoDriver::start() to
call transitionTo() to advance through the legal sequence (UNINITIALIZED →
CONFIGURED → READY → RUNNING) before/after launching simulate_thread_ (use
transitionTo(CONFIGURED), transitionTo(READY), then launch thread and
transitionTo(RUNNING)), and modify MujocoDriver::stop() to perform the RUNNING →
STOPPING → READY transitions (call transitionTo(STOPPING), request_stop()/join
thread, then transitionTo(READY)), and on any error during start/stop transition
to FAULT via transitionTo(FAULT); also ensure reset() follows the appropriate
transitions when implemented. Include calls to transitionTo using the existing
transitionTo() method names so the HAL lifecycle reflects the actual thread
state.
---
Duplicate comments:
In `@motorium/motorium_hal/CMakeLists.txt`:
- Around line 32-39: Remove 'absl' from the dependencies list variable so it is
not exported to downstream packages (the variable named "dependencies" that is
later used in ament_export_dependencies(${dependencies})); instead rely on local
find_package(absl CONFIG REQUIRED) and explicit target_link_libraries(...
absl::...) for targets that need Abseil (as already done in this
CMakeLists.txt), and ensure no other ament_target_dependencies or
ament_export_dependencies entries include 'absl'.
In `@motorium/motorium_hal/include/motorium_hal/RobotHAL.h`:
- Around line 32-43: The public header RobotHAL.h exposes std::string_view via
the toString(HalState) declaration but does not directly include <string_view>;
add a direct `#include` <string_view> at the top of RobotHAL.h (near the other
standard includes) so the header does not rely on transitive includes and ensure
the declaration of toString(HalState) compiles when this file is included on its
own.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: dbb1ddc4-e119-4234-b63a-e8f15df5ff52
📒 Files selected for processing (17)
motorium/examples/arx/arm_mujoco_sim.cppmotorium/motorium_hal/BUILD.bazelmotorium/motorium_hal/CMakeLists.txtmotorium/motorium_hal/include/motorium_hal/DriverBase.hmotorium/motorium_hal/include/motorium_hal/RobotHAL.hmotorium/motorium_hal/package.xmlmotorium/motorium_hal/src/DriverBase.cppmotorium/motorium_hal/src/RobotHAL.cppmotorium/motorium_hal/test/testDriverBase.cppmotorium/motorium_hal/test/testRobotHAL.cppmotorium/motorium_model/include/motorium_model/RobotDescription.hmotorium/motorium_mujoco/BUILD.bazelmotorium/motorium_mujoco/CMakeLists.txtmotorium/motorium_mujoco/include/motorium_mujoco/MujocoDriver.hmotorium/motorium_mujoco/include/motorium_mujoco/MujocoRenderer.hmotorium/motorium_mujoco/src/MujocoDriver.cppmotorium/motorium_mujoco/src/MujocoRenderer.cpp
Comment on lines
+43
to
+50
| void RobotHAL::update(const model::RobotJointFeedbackAction& action, model::RobotState& robot_state) { | ||
| for (const auto& driver : drivers_) { | ||
| driver->setJointFeedbackAction(action); | ||
| } | ||
| for (const auto& driver : drivers_) { | ||
| driver->updateRobotState(robot_state); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
update() fans out unconditionally regardless of driver/HAL state.
There's no precondition on state here, so callers can invoke update() while drivers are UNINITIALIZED, FAULT, or STOPPING, and each driver's setJointFeedbackAction / updateRobotState will be called anyway. For the MujocoDriver this is harmless-ish (just copies into action_internal_), but for real hardware drivers added later, commanding torques to an uninitialized/faulted actuator stack is exactly the class of bug this HAL layer exists to prevent.
At minimum, gate on getState() == HalState::ACTIVE (or return early + log) so the state machine actually guards the command path it was built for.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/src/RobotHAL.cpp` around lines 43 - 50,
RobotHAL::update currently iterates drivers_ and calls
setJointFeedbackAction/updateRobotState unconditionally; add a precondition that
checks this->getState() and only proceeds when it equals HalState::ACTIVE
(otherwise return early and optionally log a warning), so the command path is
gated by the HAL state; apply this check at the top of RobotHAL::update before
iterating drivers_ (references: RobotHAL::update, drivers_,
setJointFeedbackAction, updateRobotState, getState(), HalState::ACTIVE).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
motorium/motorium_hal/test/testRobotHAL.cpp (1)
130-137:⚠️ Potential issue | 🟡 MinorTest name still doesn't reflect what's actually being tested.
Despite being named
ConstructorThrowsOnDuplicateJointInDriver, thestd::invalid_argumenthere is thrown byRobotDescription's constructor insidemakeDriver(when it receives duplicateJointDescriptionentries), not byDriverBaseorRobotHAL. NeitherDriverBase::validateManagedJointNamesnorRobotHAL::validateDriverCoverageis exercised by this path — the test actually covers the same territory as anyRobotDescriptionduplicate-joint test. Either rename it toRobotDescriptionThrowsOnDuplicateJoints(and move it to aRobotDescriptiontest file), or rework it to actually construct a driver with duplicatemanaged_joint_namesagainst a valid description — which is whatConstructorKillsOnDuplicateManagedJointInDriveralready does via the HAL path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_hal/test/testRobotHAL.cpp` around lines 130 - 137, The test named ConstructorThrowsOnDuplicateJointInDriver is misleading because the std::invalid_argument is thrown inside RobotDescription created by makeDriver (duplicate JointDescription), not by DriverBase/RobotHAL; either rename and move the test to RobotDescription tests as RobotDescriptionThrowsOnDuplicateJoints and assert the exception comes from RobotDescription, or change the test to build a valid RobotDescription and then construct a Driver (via makeDriver or a new helper) that contains duplicate managed_joint_names to exercise DriverBase::validateManagedJointNames and RobotHAL::validateDriverCoverage (i.e., ensure the duplicate is in the driver’s managed_joint_names rather than in the description), and update the test name to reflect the intended behavior (e.g., ConstructorKillsOnDuplicateManagedJointInDriver).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h`:
- Around line 32-56: The header is missing an explicit include for
std::string_view used by the inline function toString(DriverState) in
DriverBase.h; add `#include` <string_view> at the top of DriverBase.h (and also
add the same include to RobotHAL.h where std::string_view is used) so
toString(DriverState) and any other uses of std::string_view no longer rely on
transitive includes.
- Around line 86-98: name_ and managed_joint_names_ are mutable protected
members but are logically immutable after construction; change their declaration
to enforce immutability by either making them const (e.g., const std::string
name_; const std::vector<std::string> managed_joint_names_;) if they are
initialized in the constructor, or move them to private: and keep the existing
public accessors getName() and getManagedJointNames() so subclasses (e.g.,
MujocoDriver) cannot modify them; update constructors to initialize const
members or adjust access accordingly and ensure RobotHAL::validateDriverCoverage
continues to use the public const accessors.
In `@motorium/motorium_hal/src/RobotHAL.cpp`:
- Around line 52-70: The getState() logic currently treats an empty drivers_
vector as all drivers RUNNING (all_running defaults true) and returns
HalState::ACTIVE; to fix, short-circuit at the start of RobotHAL::getState(): if
drivers_.empty() return HalState::UNCONFIGURED (or call validateDriverCoverage()
and reject empty driver sets there), so that an empty/degenerate HAL reports
UNCONFIGURED instead of ACTIVE; update any related unit tests or callers that
assume non-empty drivers_.
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 125-160: The death tests use an empty regex which is too
permissive; update the EXPECT_DEATH assertions in tests
ConstructorKillsOnUncoveredJoint,
ConstructorKillsOnDuplicateManagedJointInDriver, ConstructorKillsOnNullDriver,
and ConstructorKillsOnOverlappingDrivers to match the specific MT_CHECK
substring emitted by RobotHAL::validateDriverCoverage (or the exact "MT_CHECK"
message produced on failure) instead of "" so only the intended validation
aborts are accepted; locate those EXPECT_DEATH calls in testRobotHAL.cpp and
replace the empty regex argument with a regex like "MT_CHECK" (or the precise
message text) to tighten the assertions.
In `@motorium/motorium_mujoco/include/motorium_mujoco/MujocoDriver.h`:
- Around line 67-77: The override updateRobotStateImpl in MujocoDriver is
currently public which bypasses DriverBase's RUNNING-state gate; move the
declaration of updateRobotStateImpl(model::RobotState& robot_state) from the
public: section into a protected: section (or add a protected: block) so only
subclasses and DriverBase's public updateRobotState can call the implementation;
keep the signature and destructor/start/stop visibility unchanged and ensure the
class still compiles with MujocoDriver::updateRobotStateImpl remaining an
override of DriverBase.
In `@motorium/motorium_mujoco/src/MujocoDriver.cpp`:
- Around line 441-462: The start()/stop() methods call transitionTo(...)
unconditionally and can trigger MT_CHECK aborts from illegal state transitions;
guard each attempted transition by checking the current state and/or
isLegalTransition before calling transitionTo (e.g., in MujocoDriver::start()
check current state != READY and isLegalTransition(current, READY) before
transitionTo(READY), and ensure start() returns early if already RUNNING or
simulate_thread_.joinable(); similarly in MujocoDriver::stop() verify the
current state allows STOPPING and return early if already
STOPPING/CONFIGURED/READY as appropriate), and also make the destructor-safe by
having stop() be a no-op when the current state would make CONFIGURED→STOPPING
illegal; use existing symbols transitionTo, simulate_thread_, simulationLoop,
and isLegalTransition/currentState accessor to locate and implement the guards.
---
Duplicate comments:
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 130-137: The test named ConstructorThrowsOnDuplicateJointInDriver
is misleading because the std::invalid_argument is thrown inside
RobotDescription created by makeDriver (duplicate JointDescription), not by
DriverBase/RobotHAL; either rename and move the test to RobotDescription tests
as RobotDescriptionThrowsOnDuplicateJoints and assert the exception comes from
RobotDescription, or change the test to build a valid RobotDescription and then
construct a Driver (via makeDriver or a new helper) that contains duplicate
managed_joint_names to exercise DriverBase::validateManagedJointNames and
RobotHAL::validateDriverCoverage (i.e., ensure the duplicate is in the driver’s
managed_joint_names rather than in the description), and update the test name to
reflect the intended behavior (e.g.,
ConstructorKillsOnDuplicateManagedJointInDriver).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 50b449f2-234b-4aed-815c-186d34b303d2
📒 Files selected for processing (8)
motorium/motorium_hal/include/motorium_hal/DriverBase.hmotorium/motorium_hal/include/motorium_hal/RobotHAL.hmotorium/motorium_hal/src/DriverBase.cppmotorium/motorium_hal/src/RobotHAL.cppmotorium/motorium_hal/test/testDriverBase.cppmotorium/motorium_hal/test/testRobotHAL.cppmotorium/motorium_mujoco/include/motorium_mujoco/MujocoDriver.hmotorium/motorium_mujoco/src/MujocoDriver.cpp
Comment on lines
+32
to
+56
| #include <atomic> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include <motorium_core/Check.h> | ||
| #include <motorium_model/RobotDescription.h> | ||
| #include <motorium_model/RobotJointFeedbackAction.h> | ||
| #include <motorium_model/RobotState.h> | ||
| #include <magic_enum/magic_enum.hpp> | ||
|
|
||
| namespace motorium::hal { | ||
|
|
||
| enum class DriverState { | ||
| UNINITIALIZED, | ||
| CONFIGURED, | ||
| READY, | ||
| RUNNING, | ||
| STOPPING, | ||
| FAULT, | ||
| }; | ||
|
|
||
| // Returns a view into static storage (magic_enum::enum_name guarantees program lifetime). | ||
| inline std::string_view toString(DriverState state) { | ||
| return magic_enum::enum_name(state); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing #include <string_view> for std::string_view.
toString(DriverState) at line 54 returns std::string_view, but this header only includes <atomic>, <string>, <vector>, and the project/magic_enum headers. Relying on <string> to transitively pull in <string_view> is non-portable (and the same latent bug exists in RobotHAL.h). Add the include explicitly.
`#include` <atomic>
`#include` <string>
+#include <string_view>
`#include` <vector>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #include <atomic> | |
| #include <string> | |
| #include <vector> | |
| #include <motorium_core/Check.h> | |
| #include <motorium_model/RobotDescription.h> | |
| #include <motorium_model/RobotJointFeedbackAction.h> | |
| #include <motorium_model/RobotState.h> | |
| #include <magic_enum/magic_enum.hpp> | |
| namespace motorium::hal { | |
| enum class DriverState { | |
| UNINITIALIZED, | |
| CONFIGURED, | |
| READY, | |
| RUNNING, | |
| STOPPING, | |
| FAULT, | |
| }; | |
| // Returns a view into static storage (magic_enum::enum_name guarantees program lifetime). | |
| inline std::string_view toString(DriverState state) { | |
| return magic_enum::enum_name(state); | |
| } | |
| `#include` <atomic> | |
| `#include` <string> | |
| `#include` <string_view> | |
| `#include` <vector> | |
| `#include` <motorium_core/Check.h> | |
| `#include` <motorium_model/RobotDescription.h> | |
| `#include` <motorium_model/RobotJointFeedbackAction.h> | |
| `#include` <motorium_model/RobotState.h> | |
| `#include` <magic_enum/magic_enum.hpp> | |
| namespace motorium::hal { | |
| enum class DriverState { | |
| UNINITIALIZED, | |
| CONFIGURED, | |
| READY, | |
| RUNNING, | |
| STOPPING, | |
| FAULT, | |
| }; | |
| // Returns a view into static storage (magic_enum::enum_name guarantees program lifetime). | |
| inline std::string_view toString(DriverState state) { | |
| return magic_enum::enum_name(state); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h` around lines 32 -
56, The header is missing an explicit include for std::string_view used by the
inline function toString(DriverState) in DriverBase.h; add `#include`
<string_view> at the top of DriverBase.h (and also add the same include to
RobotHAL.h where std::string_view is used) so toString(DriverState) and any
other uses of std::string_view no longer rely on transitive includes.
Comment on lines
+52
to
+70
| HalState RobotHAL::getState() const { | ||
| bool any_stopping = false; | ||
| bool any_uninitialized = false; | ||
| bool any_configured = false; | ||
| bool all_running = true; | ||
| for (const auto& driver : drivers_) { | ||
| DriverState ds = driver->getState(); | ||
| if (ds == DriverState::FAULT) return HalState::FAULT; | ||
| if (ds == DriverState::STOPPING) any_stopping = true; | ||
| if (ds == DriverState::UNINITIALIZED) any_uninitialized = true; | ||
| if (ds == DriverState::CONFIGURED) any_configured = true; | ||
| if (ds != DriverState::RUNNING) all_running = false; | ||
| } | ||
| if (any_stopping) return HalState::STOPPING; | ||
| if (any_uninitialized) return HalState::UNCONFIGURED; | ||
| if (any_configured) return HalState::CONFIGURED; | ||
| if (all_running) return HalState::ACTIVE; | ||
| return HalState::READY; | ||
| } |
Contributor
There was a problem hiding this comment.
Empty drivers_ incorrectly reports HalState::ACTIVE.
When drivers_ is empty, the loop is skipped: all_running stays true, and none of the early returns fire, so getState() returns HalState::ACTIVE. An unconfigured/empty HAL reporting as active inverts the safety-critical semantics of the state machine. Either reject empty driver lists in validateDriverCoverage() (arguably already implied, since an empty driver set cannot cover any joint unless the robot has zero joints — a degenerate case) or short-circuit to UNCONFIGURED at the top of getState().
🔧 Proposed fix
HalState RobotHAL::getState() const {
+ if (drivers_.empty()) return HalState::UNCONFIGURED;
bool any_stopping = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| HalState RobotHAL::getState() const { | |
| bool any_stopping = false; | |
| bool any_uninitialized = false; | |
| bool any_configured = false; | |
| bool all_running = true; | |
| for (const auto& driver : drivers_) { | |
| DriverState ds = driver->getState(); | |
| if (ds == DriverState::FAULT) return HalState::FAULT; | |
| if (ds == DriverState::STOPPING) any_stopping = true; | |
| if (ds == DriverState::UNINITIALIZED) any_uninitialized = true; | |
| if (ds == DriverState::CONFIGURED) any_configured = true; | |
| if (ds != DriverState::RUNNING) all_running = false; | |
| } | |
| if (any_stopping) return HalState::STOPPING; | |
| if (any_uninitialized) return HalState::UNCONFIGURED; | |
| if (any_configured) return HalState::CONFIGURED; | |
| if (all_running) return HalState::ACTIVE; | |
| return HalState::READY; | |
| } | |
| HalState RobotHAL::getState() const { | |
| if (drivers_.empty()) return HalState::UNCONFIGURED; | |
| bool any_stopping = false; | |
| bool any_uninitialized = false; | |
| bool any_configured = false; | |
| bool all_running = true; | |
| for (const auto& driver : drivers_) { | |
| DriverState ds = driver->getState(); | |
| if (ds == DriverState::FAULT) return HalState::FAULT; | |
| if (ds == DriverState::STOPPING) any_stopping = true; | |
| if (ds == DriverState::UNINITIALIZED) any_uninitialized = true; | |
| if (ds == DriverState::CONFIGURED) any_configured = true; | |
| if (ds != DriverState::RUNNING) all_running = false; | |
| } | |
| if (any_stopping) return HalState::STOPPING; | |
| if (any_uninitialized) return HalState::UNCONFIGURED; | |
| if (any_configured) return HalState::CONFIGURED; | |
| if (all_running) return HalState::ACTIVE; | |
| return HalState::READY; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/src/RobotHAL.cpp` around lines 52 - 70, The getState()
logic currently treats an empty drivers_ vector as all drivers RUNNING
(all_running defaults true) and returns HalState::ACTIVE; to fix, short-circuit
at the start of RobotHAL::getState(): if drivers_.empty() return
HalState::UNCONFIGURED (or call validateDriverCoverage() and reject empty driver
sets there), so that an empty/degenerate HAL reports UNCONFIGURED instead of
ACTIVE; update any related unit tests or callers that assume non-empty drivers_.
Comment on lines
+125
to
+160
| TEST_F(RobotHALTest, ConstructorKillsOnUncoveredJoint) { | ||
| auto d = makeDriver("driver", {"joint1"}); // joint2 not covered | ||
| EXPECT_DEATH(RobotHAL(xml_path_, {d}), ""); | ||
| } | ||
|
|
||
| TEST_F(RobotHALTest, ConstructorThrowsOnDuplicateJointInDriver) { | ||
| EXPECT_THROW( | ||
| { | ||
| auto d = makeDriver("driver", {"joint1", "joint1"}); | ||
| RobotHAL hal(xml_path_, {d}); | ||
| }, | ||
| std::invalid_argument); | ||
| } | ||
|
|
||
| TEST_F(RobotHALTest, ConstructorKillsOnDuplicateManagedJointInDriver) { | ||
| std::vector<JointDescription> joint_descs; | ||
| for (const auto& name : {"joint1", "joint2"}) { | ||
| JointDescription jd; | ||
| jd.name = name; | ||
| joint_descs.push_back(jd); | ||
| } | ||
| RobotDescription desc(joint_descs); | ||
| auto d = std::make_shared<TestDriver>(desc, "driver", std::vector<std::string>{"joint1", "joint1"}); | ||
| EXPECT_DEATH(RobotHAL(xml_path_, {d}), ""); | ||
| } | ||
|
|
||
| TEST_F(RobotHALTest, ConstructorKillsOnNullDriver) { | ||
| auto d = makeDriver("driver", {"joint1", "joint2"}); | ||
| EXPECT_DEATH(RobotHAL(xml_path_, {d, nullptr}), ""); | ||
| } | ||
|
|
||
| TEST_F(RobotHALTest, ConstructorKillsOnOverlappingDrivers) { | ||
| auto d1 = makeDriver("driver1", {"joint1", "joint2"}); | ||
| auto d2 = makeDriver("driver2", {"joint2"}); // joint2 covered twice | ||
| EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), ""); | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten the EXPECT_DEATH regexes to avoid masking unrelated aborts.
These four death tests (ConstructorKillsOnUncoveredJoint, ConstructorKillsOnDuplicateManagedJointInDriver, ConstructorKillsOnNullDriver, ConstructorKillsOnOverlappingDrivers) all pass "" as the expected regex, which matches any death output — including unrelated aborts (ASan, a CHECK elsewhere in construction, etc.). Matching the specific MT_CHECK substring emitted by RobotHAL::validateDriverCoverage makes these assertions resilient to regressions.
🔧 Proposed fix
- EXPECT_DEATH(RobotHAL(xml_path_, {d}), "");
+ EXPECT_DEATH(RobotHAL(xml_path_, {d}), "is not managed by any driver");
...
- EXPECT_DEATH(RobotHAL(xml_path_, {d}), "");
+ EXPECT_DEATH(RobotHAL(xml_path_, {d}), "is managed by multiple drivers");
...
- EXPECT_DEATH(RobotHAL(xml_path_, {d, nullptr}), "");
+ EXPECT_DEATH(RobotHAL(xml_path_, {d, nullptr}), "null driver");
...
- EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), "");
+ EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), "is managed by multiple drivers");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/test/testRobotHAL.cpp` around lines 125 - 160, The
death tests use an empty regex which is too permissive; update the EXPECT_DEATH
assertions in tests ConstructorKillsOnUncoveredJoint,
ConstructorKillsOnDuplicateManagedJointInDriver, ConstructorKillsOnNullDriver,
and ConstructorKillsOnOverlappingDrivers to match the specific MT_CHECK
substring emitted by RobotHAL::validateDriverCoverage (or the exact "MT_CHECK"
message produced on failure) instead of "" so only the intended validation
aborts are accepted; locate those EXPECT_DEATH calls in testRobotHAL.cpp and
replace the empty regex argument with a regex like "MT_CHECK" (or the precise
message text) to tighten the assertions.
Comment on lines
67
to
+77
| public: | ||
| MujocoSimInterface(const MujocoSimConfig& config, const model::RobotDescription& robot_description); | ||
| MujocoDriver(const MujocoSimConfig& config, const model::RobotDescription& robot_description); | ||
|
|
||
| /** Destructor */ | ||
| ~MujocoSimInterface(); | ||
| ~MujocoDriver(); | ||
|
|
||
| void start() override; | ||
|
|
||
| void stop() override; | ||
|
|
||
| void updateRobotState(model::RobotState& robot_state) override; | ||
| void updateRobotStateImpl(model::RobotState& robot_state) override; |
Contributor
There was a problem hiding this comment.
updateRobotStateImpl is exposed publicly, bypassing the RUNNING-state gate.
In DriverBase, updateRobotStateImpl is declared protected specifically so external callers must go through the non-virtual updateRobotState(...) which only delegates when state_ == DriverState::RUNNING (DriverBase.cpp lines 64-69). Overriding it in the public: section here widens its access and lets any caller invoke the raw implementation regardless of driver state, defeating the gatekeeper. Move the override under a protected: section.
🔧 Proposed fix
void stop() override;
- void updateRobotStateImpl(model::RobotState& robot_state) override;
-
void setJointFeedbackAction(const model::RobotJointFeedbackAction& action) override;
@@
const MujocoSimConfig& getConfig() const { return config_; }
private:
+ void updateRobotStateImpl(model::RobotState& robot_state) override;
+
void initSim();(Or use a protected: section if subclass access is intended.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_mujoco/include/motorium_mujoco/MujocoDriver.h` around lines
67 - 77, The override updateRobotStateImpl in MujocoDriver is currently public
which bypasses DriverBase's RUNNING-state gate; move the declaration of
updateRobotStateImpl(model::RobotState& robot_state) from the public: section
into a protected: section (or add a protected: block) so only subclasses and
DriverBase's public updateRobotState can call the implementation; keep the
signature and destructor/start/stop visibility unchanged and ensure the class
still compiles with MujocoDriver::updateRobotStateImpl remaining an override of
DriverBase.
Comment on lines
+441
to
462
| void MujocoDriver::start() { | ||
| if (!sim_initialized_) initSim(); | ||
| transitionTo(hal::DriverState::READY); | ||
|
|
||
| if (simulate_thread_.joinable()) { | ||
| std::cerr << "WARNING: Tried to start simulation thread, but it is already running." << std::endl; | ||
| return; | ||
| } | ||
| transitionTo(hal::DriverState::RUNNING); | ||
| simulate_thread_ = std::jthread([this](std::stop_token st) { this->simulationLoop(st); }); | ||
| } | ||
|
|
||
| void MujocoSimInterface::stop() { | ||
| void MujocoDriver::stop() { | ||
| transitionTo(hal::DriverState::STOPPING); | ||
| if (simulate_thread_.joinable()) { | ||
| simulate_thread_.request_stop(); | ||
| if (simulate_thread_.get_id() != std::this_thread::get_id()) { | ||
| simulate_thread_.join(); | ||
| } | ||
| } | ||
| transitionTo(hal::DriverState::READY); | ||
| } |
Contributor
There was a problem hiding this comment.
start()/stop() transitions don't guard against current state and will MT_CHECK-abort on unexpected lifecycle ordering.
Several realistic call sequences will now crash the process via the new isLegalTransition matrix:
- Destructor on a driver that never ran.
~MujocoDriver()(line 115) unconditionally callsstop(), which unconditionally runstransitionTo(STOPPING). If an exception is thrown downstream of construction (or the caller never callsstart()), the driver is inCONFIGURED, andCONFIGURED → STOPPINGis illegal →MT_CHECKaborts during stack unwinding/destruction. - Double
start(). After astart()/stop()cycle, the state isREADY(line 461). Callingstart()again hitstransitionTo(READY)on line 443, andREADY → READYis illegal → abort. The existingif (simulate_thread_.joinable())guard fires after the illegal transition. - Double
stop(). After astop(), state isREADY. A secondstop()runstransitionTo(STOPPING)— butREADY → STOPPINGis illegal → abort.
Gate each transition on the current state so lifecycle misuse (including destruction before start()) is a no-op rather than a process abort.
🔧 Suggested shape
void MujocoDriver::start() {
+ if (getState() == hal::DriverState::RUNNING) return;
if (!sim_initialized_) initSim();
- transitionTo(hal::DriverState::READY);
-
- if (simulate_thread_.joinable()) {
- std::cerr << "WARNING: Tried to start simulation thread, but it is already running." << std::endl;
- return;
- }
+ if (getState() == hal::DriverState::CONFIGURED) transitionTo(hal::DriverState::READY);
+ if (simulate_thread_.joinable()) {
+ std::cerr << "WARNING: Tried to start simulation thread, but it is already running." << std::endl;
+ return;
+ }
transitionTo(hal::DriverState::RUNNING);
simulate_thread_ = std::jthread([this](std::stop_token st) { this->simulationLoop(st); });
}
void MujocoDriver::stop() {
- transitionTo(hal::DriverState::STOPPING);
+ if (getState() != hal::DriverState::RUNNING) return;
+ transitionTo(hal::DriverState::STOPPING);
if (simulate_thread_.joinable()) {
simulate_thread_.request_stop();
if (simulate_thread_.get_id() != std::this_thread::get_id()) {
simulate_thread_.join();
}
}
transitionTo(hal::DriverState::READY);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_mujoco/src/MujocoDriver.cpp` around lines 441 - 462, The
start()/stop() methods call transitionTo(...) unconditionally and can trigger
MT_CHECK aborts from illegal state transitions; guard each attempted transition
by checking the current state and/or isLegalTransition before calling
transitionTo (e.g., in MujocoDriver::start() check current state != READY and
isLegalTransition(current, READY) before transitionTo(READY), and ensure start()
returns early if already RUNNING or simulate_thread_.joinable(); similarly in
MujocoDriver::stop() verify the current state allows STOPPING and return early
if already STOPPING/CONFIGURED/READY as appropriate), and also make the
destructor-safe by having stop() be a no-op when the current state would make
CONFIGURED→STOPPING illegal; use existing symbols transitionTo,
simulate_thread_, simulationLoop, and isLegalTransition/currentState accessor to
locate and implement the guards.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
motorium/motorium_hal/test/testRobotHAL.cpp (2)
90-96:⚠️ Potential issue | 🟡 MinorUse a unique temp directory per test.
temp_directory_path() / "robot_hal_test"is shared across concurrent test processes; one test’sTearDown()can delete another test’s XML.🔧 Proposed fix
`#include` <filesystem> `#include` <fstream> `#include` <memory> +#include <random> `#include` <vector>void SetUp() override { - temp_dir_ = std::filesystem::temp_directory_path() / "robot_hal_test"; - std::filesystem::create_directories(temp_dir_); + std::random_device rd; + do { + temp_dir_ = std::filesystem::temp_directory_path() / ("robot_hal_test_" + std::to_string(rd())); + } while (!std::filesystem::create_directory(temp_dir_)); xml_path_ = writeTempXml(temp_dir_, "robot.xml", {"joint1", "joint2"}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_hal/test/testRobotHAL.cpp` around lines 90 - 96, SetUp() currently uses a fixed temp_dir_ ("robot_hal_test") which can collide across concurrent tests; change SetUp() to create a unique directory name (e.g. use std::filesystem::unique_path with a pattern like "robot_hal_test-%%%%-%%%%") when assigning temp_dir_, then create that directory and call writeTempXml(temp_dir_, "robot.xml", {"joint1","joint2"}) as before; leave TearDown() removing temp_dir_ so each test cleans its own unique folder.
126-160:⚠️ Potential issue | 🟡 MinorTighten the death-test regexes.
The empty regex matches any abort, including unrelated
CHECKfailures or sanitizer crashes. Match the expected validation message instead.🔧 Proposed fix
TEST_F(RobotHALTest, ConstructorKillsOnUncoveredJoint) { auto d = makeDriver("driver", {"joint1"}); // joint2 not covered - EXPECT_DEATH(RobotHAL(xml_path_, {d}), ""); + EXPECT_DEATH(RobotHAL(xml_path_, {d}), "is not managed by any driver"); } @@ RobotDescription desc(joint_descs); auto d = std::make_shared<TestDriver>(desc, "driver", std::vector<std::string>{"joint1", "joint1"}); - EXPECT_DEATH(RobotHAL(xml_path_, {d}), ""); + EXPECT_DEATH(RobotHAL(xml_path_, {d}), "is managed by multiple drivers"); } TEST_F(RobotHALTest, ConstructorKillsOnNullDriver) { auto d = makeDriver("driver", {"joint1", "joint2"}); - EXPECT_DEATH(RobotHAL(xml_path_, {d, nullptr}), ""); + EXPECT_DEATH(RobotHAL(xml_path_, {d, nullptr}), "null driver"); } TEST_F(RobotHALTest, ConstructorKillsOnOverlappingDrivers) { auto d1 = makeDriver("driver1", {"joint1", "joint2"}); auto d2 = makeDriver("driver2", {"joint2"}); // joint2 covered twice - EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), ""); + EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), "is managed by multiple drivers"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_hal/test/testRobotHAL.cpp` around lines 126 - 160, Replace the empty EXPECT_DEATH regexes in these tests with tight regexes that match the RobotHAL validation messages; for each test update the EXPECT_DEATH in ConstructorKillsOnUncoveredJoint, ConstructorKillsOnDuplicateManagedJointInDriver, ConstructorKillsOnNullDriver, and ConstructorKillsOnOverlappingDrivers to assert the specific abort text emitted by the RobotHAL constructor (e.g., match the phrase "uncovered joint" for the uncovered joint test, "duplicate managed joint" or the exact wording used in RobotHAL for duplicate managed joints, "null driver" or the exact null-pointer message for the null driver test, and "overlapping" or the exact overlap message for overlapping drivers); locate the exact strings in the RobotHAL constructor/validation or TestDriver error messages and use those as the regex argument to EXPECT_DEATH instead of "".motorium/motorium_core/include/motorium_core/StateMachine.h (1)
56-64:⚠️ Potential issue | 🟠 MajorAvoid aborting on benign concurrent transitions.
compare_exchange_weakrefreshescurrenton failure, so the next loop can validatenextagainst a state the caller did not observe. Two legal requests from the same state can make the losing thread hitMT_CHECKon a follow-up transition and abort the process.🔧 One possible fix: validate once and no-op if another transition wins
void transitionTo(StateEnum next) { - StateEnum current = state_.load(std::memory_order_acquire); - while (true) { - MT_CHECK(isLegalTransition(current, next)) - << "Illegal state transition: " << magic_enum::enum_name(current) << " -> " << magic_enum::enum_name(next); - if (state_.compare_exchange_weak(current, next, std::memory_order_acq_rel)) { - return; - } + const StateEnum observed = state_.load(std::memory_order_acquire); + MT_CHECK(isLegalTransition(observed, next)) + << "Illegal state transition: " << magic_enum::enum_name(observed) << " -> " << magic_enum::enum_name(next); + + StateEnum expected = observed; + while (!state_.compare_exchange_weak(expected, next, std::memory_order_acq_rel, std::memory_order_acquire)) { + if (expected != observed) { + return; + } } }C++ std::atomic compare_exchange_weak expected argument updated on failure and spurious failure semantics🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@motorium/motorium_core/include/motorium_core/StateMachine.h` around lines 56 - 64, The current transitionTo method validates legality on every loop iteration which can abort when another thread wins a concurrent transition; change it to validate only once against the initially observed state and then attempt the compare_exchange_weak loop: load initial StateEnum observed = state_.load(...); MT_CHECK(isLegalTransition(observed, next)) << ...; then while (!state_.compare_exchange_weak(observed, next, ...)) { if (observed == next) return; /* reloads into observed automatically by compare_exchange_weak */ } return; Update the code in transitionTo to refer to state_, isLegalTransition, MT_CHECK and compare_exchange_weak accordingly so a losing concurrent thread will simply no-op instead of aborting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.bazelrc:
- Around line 5-6: The global .bazelrc entries setting --repo_env=CC and
--repo_env=CXX force every machine to use host clang; remove those global
--repo_env=CC/--repo_env=CXX lines and instead add an opt-in config section
(e.g. "build:clang" that sets --repo_env=CC=clang, --repo_env=CXX=clang++, and
any --cxxopt flags like -std=c++23), then update CI/workflows to invoke Bazel
with --config=clang where Clang is required or register and reference a proper
C++ toolchain in Bazel so non-Clang hosts won’t fail unexpectedly.
In `@motorium/motorium_core/package.xml`:
- Line 12: The package.xml currently lists the Debian/apt package name
"ros-jazzy-magic-enum" as a dependency; update the dependency entry in
package.xml to use the rosdep/ROS package key "magic_enum" instead (replace the
"ros-jazzy-magic-enum" token in the relevant <depend> / <exec_depend> /
<build_depend> element with "magic_enum") so ROS dependency resolution uses the
correct rosdep key.
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h`:
- Around line 32-35: The header Motorium HAL is using std::move (referenced
where the class/constructor or method in DriverBase.h performs std::move) but
does not include <utility>, causing non-portable reliance on transitive
includes; fix by adding `#include` <utility> to Motorium HAL's DriverBase.h so
std::move is declared before use (update the includes at the top of DriverBase.h
near other standard headers to ensure standalone compilation).
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 98-107: The helper makeDriver/TestDriver can create drivers that
manage joints not present in the HAL model; add a production guard in
RobotHAL::validateDriverCoverage to assert each driver-managed joint exists in
the loaded RobotDescription by calling robot_description_.containsJoint(joint)
and failing (MT_CHECK or equivalent) with a clear message if not; also add a
unit test that constructs a driver via makeDriver with
{"joint1","joint2","ghost_joint"} while xml_path_ only contains joint1/2 and
assert RobotHAL construction/validateDriverCoverage fails to ensure extra/ghost
joints are rejected.
---
Duplicate comments:
In `@motorium/motorium_core/include/motorium_core/StateMachine.h`:
- Around line 56-64: The current transitionTo method validates legality on every
loop iteration which can abort when another thread wins a concurrent transition;
change it to validate only once against the initially observed state and then
attempt the compare_exchange_weak loop: load initial StateEnum observed =
state_.load(...); MT_CHECK(isLegalTransition(observed, next)) << ...; then while
(!state_.compare_exchange_weak(observed, next, ...)) { if (observed == next)
return; /* reloads into observed automatically by compare_exchange_weak */ }
return; Update the code in transitionTo to refer to state_, isLegalTransition,
MT_CHECK and compare_exchange_weak accordingly so a losing concurrent thread
will simply no-op instead of aborting.
In `@motorium/motorium_hal/test/testRobotHAL.cpp`:
- Around line 90-96: SetUp() currently uses a fixed temp_dir_ ("robot_hal_test")
which can collide across concurrent tests; change SetUp() to create a unique
directory name (e.g. use std::filesystem::unique_path with a pattern like
"robot_hal_test-%%%%-%%%%") when assigning temp_dir_, then create that directory
and call writeTempXml(temp_dir_, "robot.xml", {"joint1","joint2"}) as before;
leave TearDown() removing temp_dir_ so each test cleans its own unique folder.
- Around line 126-160: Replace the empty EXPECT_DEATH regexes in these tests
with tight regexes that match the RobotHAL validation messages; for each test
update the EXPECT_DEATH in ConstructorKillsOnUncoveredJoint,
ConstructorKillsOnDuplicateManagedJointInDriver, ConstructorKillsOnNullDriver,
and ConstructorKillsOnOverlappingDrivers to assert the specific abort text
emitted by the RobotHAL constructor (e.g., match the phrase "uncovered joint"
for the uncovered joint test, "duplicate managed joint" or the exact wording
used in RobotHAL for duplicate managed joints, "null driver" or the exact
null-pointer message for the null driver test, and "overlapping" or the exact
overlap message for overlapping drivers); locate the exact strings in the
RobotHAL constructor/validation or TestDriver error messages and use those as
the regex argument to EXPECT_DEATH instead of "".
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3d4193a5-703f-4883-923c-c64e55714165
⛔ Files ignored due to path filters (1)
MODULE.bazel.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.bazelrcmotorium/motorium_core/BUILD.bazelmotorium/motorium_core/CMakeLists.txtmotorium/motorium_core/include/motorium_core/StateMachine.hmotorium/motorium_core/package.xmlmotorium/motorium_hal/include/motorium_hal/DriverBase.hmotorium/motorium_hal/src/DriverBase.cppmotorium/motorium_hal/test/testDriverBase.cppmotorium/motorium_hal/test/testRobotHAL.cppmotorium/motorium_mujoco/include/motorium_mujoco/MujocoRenderer.h
Comment on lines
+5
to
+6
| build --repo_env=CC=clang | ||
| build --repo_env=CXX=clang++ No newline at end of file |
Contributor
There was a problem hiding this comment.
Avoid forcing host Clang for every Bazel build.
These global --repo_env entries make all developers and CI agents depend on clang/clang++ being present on PATH and compatible with -std=c++23. Prefer an opt-in config or a registered C++ toolchain so non-Clang hosts do not fail unexpectedly.
Suggested opt-in `.bazelrc` config
-build --repo_env=CC=clang
-build --repo_env=CXX=clang++
+build:clang --repo_env=CC=clang
+build:clang --repo_env=CXX=clang++Then invoke Clang builds explicitly with:
bazel build --config=clang //...Verify the supported environments and CI setup expose compatible Clang binaries:
#!/bin/bash
# Read-only verification: inspect Bazel/CI config and check whether the current environment has Clang.
set -euo pipefail
echo "== Current environment compiler lookup =="
command -v clang || true
command -v clang++ || true
clang --version 2>/dev/null || true
clang++ --version 2>/dev/null || true
echo
echo "== Bazel rc files =="
fd -i '(^|.*)\.bazelrc($|.*)' -x sh -c '
echo "## $1"
sed -n "1,160p" "$1"
' sh {}
echo
echo "== CI/workflow references to clang or bazel config =="
fd -i '\.ya?ml$|\.toml$|\.json$' -x rg -n -C2 'clang|clang\+\+|bazel|--config=clang|repo_env=CC|repo_env=CXX' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.bazelrc around lines 5 - 6, The global .bazelrc entries setting
--repo_env=CC and --repo_env=CXX force every machine to use host clang; remove
those global --repo_env=CC/--repo_env=CXX lines and instead add an opt-in config
section (e.g. "build:clang" that sets --repo_env=CC=clang,
--repo_env=CXX=clang++, and any --cxxopt flags like -std=c++23), then update
CI/workflows to invoke Bazel with --config=clang where Clang is required or
register and reference a proper C++ toolchain in Bazel so non-Clang hosts won’t
fail unexpectedly.
| <license>BSD</license> | ||
|
|
||
| <build_depend>absl_vendor</build_depend> | ||
| <build_depend>ros-jazzy-magic-enum</build_depend> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dependency key validity for package.xml usage (read-only).
# Expected: `magic_enum` resolves; `ros-jazzy-magic-enum` does not.
set -euo pipefail
if command -v rosdep >/dev/null 2>&1; then
rosdep resolve magic_enum || true
rosdep resolve ros-jazzy-magic-enum || true
else
echo "rosdep not installed in this environment; verify locally with:"
echo " rosdep resolve magic_enum"
echo " rosdep resolve ros-jazzy-magic-enum"
fiRepository: manumerous/motorium
Length of output: 191
🏁 Script executed:
cat -n motorium/motorium_core/package.xmlRepository: manumerous/motorium
Length of output: 928
🌐 Web query:
magic_enum ROS Jazzy package rosdep
💡 Result:
magic_enum is an official ROS 2 package available for the Jazzy distribution (version 0.9.7). It provides a header-only C++17 library for static reflection of enums (to/from string, iteration) without macros or boilerplate. As a released ROS package listed on index.ros.org for Jazzy, the rosdep key "magic_enum" is defined in the official rosdistro/rosdep rules. You can use it in package.xml dependencies: magic_enum To install: rosdep install --from-paths src --ignore-src -r -y (after rosdep update). It is a binary package installable via apt on Debian-based systems (e.g., ros-jazzy-magic-enum in RoboStack), and source-built via colcon from the release repository https://github.com/ros2-gbp/magic_enum-release.git (upstream: https://github.com/Neargye/magic_enum.git). No external system dependencies are indicated; it's header-only. If rosdep resolve magic_enum fails, ensure rosdep is updated for Jazzy and sources.list.d includes official ROS repos.
Citations:
- 1: https://index.ros.org/r/magic_enum/
- 2: https://index.ros.org/p/magic_enum/
- 3: https://github.com/ros2-gbp/magic_enum-release
- 4: https://prefix.dev/channels/robostack-jazzy/packages/ros-jazzy-magic-enum
- 5: https://docs.ros.org/en/jazzy/Tutorials/Advanced/Supplementing-Custom-Rosdep-Keys.html
Use ROS package/rosdep key instead of apt package name
package.xml dependencies should reference ROS package names (rosdep keys), not Ubuntu apt package names. The current value ros-jazzy-magic-enum is the Debian-based binary package name and will not resolve correctly in ROS dependency resolution. The official rosdep key for ROS 2 Jazzy is magic_enum.
Proposed fix
- <build_depend>ros-jazzy-magic-enum</build_depend>
+ <build_depend>magic_enum</build_depend>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_core/package.xml` at line 12, The package.xml currently
lists the Debian/apt package name "ros-jazzy-magic-enum" as a dependency; update
the dependency entry in package.xml to use the rosdep/ROS package key
"magic_enum" instead (replace the "ros-jazzy-magic-enum" token in the relevant
<depend> / <exec_depend> / <build_depend> element with "magic_enum") so ROS
dependency resolution uses the correct rosdep key.
Comment on lines
+32
to
+35
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include <motorium_core/StateMachine.h> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
header="motorium/motorium_hal/include/motorium_hal/DriverBase.h"
echo "Checking std::move usage and explicit <utility> include in ${header}"
rg -n 'std::move|#include <utility>' "$header"
echo
echo "Expected before fix: std::move is present and `#include` <utility> is absent."Repository: manumerous/motorium
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
header="motorium/motorium_hal/include/motorium_hal/DriverBase.h"
# Get the full file to see includes
echo "=== Full include section of ${header} ==="
head -20 "$header"
echo
echo "=== All std::move occurrences ==="
rg -n 'std::move' "$header"
echo
echo "=== All include directives ==="
rg -n '^#include' "$header"Repository: manumerous/motorium
Length of output: 1566
Include <utility> for std::move.
Line 63 uses std::move, but this header does not include <utility>. Relying on transitive includes is non-portable and can break standalone header compilation on strict compiler configurations.
🛠️ Proposed fix
`#include` <string>
+#include <utility>
`#include` <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/include/motorium_hal/DriverBase.h` around lines 32 -
35, The header Motorium HAL is using std::move (referenced where the
class/constructor or method in DriverBase.h performs std::move) but does not
include <utility>, causing non-portable reliance on transitive includes; fix by
adding `#include` <utility> to Motorium HAL's DriverBase.h so std::move is
declared before use (update the includes at the top of DriverBase.h near other
standard headers to ensure standalone compilation).
Comment on lines
+98
to
+107
| std::shared_ptr<TestDriver> makeDriver(const std::string& name, std::vector<std::string> joints) { | ||
| std::vector<JointDescription> joint_descs; | ||
| for (const auto& j : joints) { | ||
| JointDescription jd; | ||
| jd.name = j; | ||
| joint_descs.push_back(jd); | ||
| } | ||
| RobotDescription desc(joint_descs); | ||
| return std::make_shared<TestDriver>(desc, name, joints); | ||
| } |
Contributor
There was a problem hiding this comment.
Add coverage for driver-managed joints outside the HAL model.
This helper can create a driver for {"joint1", "joint2", "ghost_joint"} while xml_path_ only contains joint1/joint2. With the current RobotHAL::validateDriverCoverage() logic, the extra managed joint is inserted but never checked against the HAL robot_description_, so construction can succeed with an invalid driver/model contract.
🧪 Suggested test plus production guard
TEST_F(RobotHALTest, ConstructorKillsOnOverlappingDrivers) {
auto d1 = makeDriver("driver1", {"joint1", "joint2"});
auto d2 = makeDriver("driver2", {"joint2"}); // joint2 covered twice
EXPECT_DEATH(RobotHAL(xml_path_, {d1, d2}), "");
}
+
+TEST_F(RobotHALTest, ConstructorKillsWhenDriverManagesUnknownJoint) {
+ auto d = makeDriver("driver", {"joint1", "joint2", "ghost_joint"});
+ EXPECT_DEATH(RobotHAL(xml_path_, {d}), "is not present in the robot model");
+}And in RobotHAL::validateDriverCoverage():
MT_CHECK(robot_description_.containsJoint(joint))
<< "[RobotHAL] Bad Configuration: Joint '" << joint << "' is not present in the robot model.";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@motorium/motorium_hal/test/testRobotHAL.cpp` around lines 98 - 107, The
helper makeDriver/TestDriver can create drivers that manage joints not present
in the HAL model; add a production guard in RobotHAL::validateDriverCoverage to
assert each driver-managed joint exists in the loaded RobotDescription by
calling robot_description_.containsJoint(joint) and failing (MT_CHECK or
equivalent) with a clear message if not; also add a unit test that constructs a
driver via makeDriver with {"joint1","joint2","ghost_joint"} while xml_path_
only contains joint1/2 and assert RobotHAL construction/validateDriverCoverage
fails to ensure extra/ghost joints are rejected.
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
1 participant
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
Tests
Chores