This repository was archived by the owner on Jul 26, 2026. It is now read-only.
test: add a launch_testing integration test of the dummy bringup - #114
Merged
Conversation
Regression tests for the canonical port_name parameter (#87, #86): usb_port is still accepted with a one-time deprecation warning, port_name wins when both are given, and a missing port parameter is reported as an init error instead of an uncaught exception. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
When torque_enable is false the plugin never turns servo torque on: on_activate skips the torque-on and the mode-switch sequencing skips the re-enable, while explicit torque-off requests still go through. This supports leader arms in teleoperation setups that must stay freely movable. Re-implements #106 on the driver-abstraction base. Co-authored-by: Maverobot <Maverobot@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
…t torque-off still runs Two gaps from the initial torque_enable implementation: - perform_command_mode_switch() only cleared the fault latch when torque_enabled_ was true, but with torque_enable=false torque can never become enabled, so any transient mode-switch failure latched switch_failed_ permanently and every later write() escalated into on_error(), tearing the component down for a leader arm that is behaving exactly as configured. The latch now also clears on a successful switch when torque_enable_param_ is false, since staying de-energized is the intended healthy outcome in that configuration. - added a test asserting that torque-off requests (on_deactivate) still reach the driver when torque_enable is false, pinning the safety- relevant half of the guard that was previously unasserted. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
read() now holds the last-known state and returns OK for up to read_error_tolerance - 1 consecutive read_states failures (hardware parameter, default 5); the Nth consecutive failure returns return_type::ERROR so the controller manager can react (on_error on jazzy+). Any successful read resets the counter. Fixes the spurious component shutdowns on momentarily noisy buses reported in #88. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
write() now sends nothing (and returns OK) until read() has succeeded once after activation; that first success also re-syncs commands to the actual servo state via reset_command(). A bus where reads fail can therefore never produce a zero/NaN goal position and the arm no longer slams into itself on flaky startup. The has_valid_state_ latch lives in the shared read_joint_states() helper so it fires from both read() and on_activate()'s initial read without duplicating the logic. on_activate() now treats that initial read as best-effort: activation succeeds even when it fails, relying on the write guard to keep the bus silent until a read lands. Driver write failures are now logged and counted against a new write_error_tolerance hardware parameter (default 5, separate from read_error_tolerance) instead of being silently ignored. Fixes #92. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
baud_rate, read_error_tolerance and write_error_tolerance each had the same find -> stoi/try-catch -> range-check -> ERROR skeleton inlined in init_impl, and the queued gear_ratio/offset/Return_Delay_Time tasks would each have copied it again. Factor the shape into parse_int_param(params, name, out, min_value); callers keep the parameter-specific "is it required" logic (baud_rate still errors when absent and not in dummy mode) on top of it. Also give baud_rate an explicit lower bound: it is passed on as the uint32_t argument to DynamixelWorkbench::init(), so a zero or negative value would be meaningless or silently wrap. Behavior is otherwise unchanged: identical log wording, identical CallbackReturn::ERROR outcomes on parse failure or an out-of-range value, and an absent key still keeps the compiled-in default. Also add a write_error_tolerance = 1 boundary test: the first write failure must escalate to ERROR immediately, with no one-failure grace period. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
gear_ratio (default 1.0) is the number of motor revolutions per joint revolution. The plugin converts between joint-side and motor-side values on every read/write path: positions and velocities are divided by the ratio on read and multiplied on write, efforts are multiplied on read and divided on write, composing with the existing torque_constant scaling. PWM duty ratios are left unconverted. The conversion helpers also take an offset term, added by Task 7. Negative ratios invert the direction; zero is rejected at init via the new parse_double_param helper, a sibling of parse_int_param. Re-implements #95 on the driver-abstraction base. Closes #94. Co-authored-by: Tacha-S <13605820+Tacha-S@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
offset (default 0.0, joint-side radians) shifts the reported position: joint_reported = raw - offset, and position commands add the offset back before being sent. Combined with gear_ratio the pipeline is joint = motor / gear_ratio - offset. Velocity, effort, and PWM are unaffected. parse_double_param and parse_int_param now take an optional joint name, included in their error messages for per-joint parameters (and omitted for hardware-level ones like baud_rate), so a typo'd gear_ratio or offset value is traceable on a multi-joint robot. This re-implements the intent of the URDF <calibration rising> support from #96 as a plain <ros2_control> param instead, since HardwareInfo exposes <ros2_control> params uniformly on all four supported distros. Closes #93. Co-authored-by: Tacha-S <13605820+Tacha-S@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Task 7 review flagged that PWM's independence from offset (and, transitively, gear_ratio) was only stated in comments, never exercised by a test -- offset's correction explicitly required this to be asserted, not just structurally implied. Add PwmIsUnaffectedByGearRatioAndOffset to ParamsRobustnessTest: a joint configured for pwm control, with a non-default gear_ratio AND offset, must see its commanded duty ratio reach write_pwms() unchanged. Covering both parameters in one test closes the equivalent gap left open for gear_ratio alone by the prior task's review. Verified the assertion is load-bearing by temporarily reintroducing an offset subtraction on the pwm write path and confirming the test fails before reverting. ParamsRobustnessTest gains command-interface export and a set_command_value() helper (mirroring the existing state_value() pattern) since driving write() into the PWM branch requires setting joint.command.pwm directly -- reset_command() always resets it to 0.0, so there is no other way to give it a distinguishing non-zero value. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Return_Delay_Time joins kExtraJointParameters, so a joint-level <param name="Return_Delay_Time">0</param> is written to the control table on configure. Setting it to 0 removes the 500 us default reply delay per servo and is the main mitigation for the read latency reported in #90. Re-implements #105. Co-authored-by: Ignacio Davila <99193391+IDavGal@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Two <joint> entries sharing one Dynamixel id gave the plugin two independent command, state and control-mode records for a single servo, which then fought each other on every write cycle. The configuration is now rejected during on_init, naming the id and both joints, instead of silently misbehaving at runtime. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
A NaN or infinite command reached convertRadian2Value, convertVelocity2Value, convertCurrent2Value or duty_to_pwm_ticks and was narrowed to int32_t, which is undefined behavior: an unspecified goal value went to a torqued servo and FE_INVALID was raised, with no error reported. All four write paths now refuse the whole batch and report which id carried the non-finite value. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Adds a regression test for the ids/values size guard added alongside the finite-value check, so the length-mismatch branch is exercised and not just implemented. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
A single plugin-wide torque flag could not express the state a partial failure leaves behind: energizing joint 1 and failing on joint 2 left the flag false while joint 1 was under power, so the next mode switch skipped the mandatory torque-off leg and the firmware rejected the Operating_Mode write. Torque state now lives on each joint, updated as each driver call returns, and the mode-switch fault latch clears only when every joint is energized again. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
test_dynamixel_hardware.cpp uses hardware_interface::HW_IF_* at eleven sites but never included the header that defines them. jazzy and rolling supply it transitively through hardware_interface/hardware_component_interface.hpp, which humble has no equivalent of, so only humble failed to compile -- and every task since the breakage was introduced verified on jazzy alone. The header path, namespace and symbol spellings are identical on all four target distros, so a plain include is correct and no compat.hpp gate applies. Naming the parameters of the ReadReturns action type is a second humble-only fix: uncrustify 0.72 (humble) and 0.78 (jazzy and later) disagree about the space in a function type ending in "&)", and no spelling of that construct satisfies both. Naming the parameters removes the construct they disagree on. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
A rejected set_torque(id, true) was still recorded as energized, on the reasoning that assuming the worst kept the next switch from skipping the mandatory torque-off leg. That assumption is conservative for the mode write but optimistic for the fault latch, and the latch is what reads the flag: two joints switched, joint 2's re-enable rejected, switch_failed_ latched, then a later switch on joint 1 alone found all_torque_enabled() true, cleared the fault, and write() resumed sync-writing goals to a limp servo. The torque-off leg no longer skips a joint it believes is already off -- a joint that is merely not known to be energized may still be torqued, and the redundant torque-off is idempotent -- so nothing depends on the pessimistic value any more, and the flag is now set only after the driver confirms torque on. Restoring torque remains limited to the joints that were confirmed energized on entry, so a mode switch still never powers up servos left limp by a deactivation, a torque_enable=false configuration or an earlier failure. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Deleting "all_torque_enabled() ||" from the latch-clearing condition left the whole suite green, so the true branch was unpinned. It is reachable: an activation rejected on a joint that was ALREADY confirmed energized leaves that joint's flag untouched, so every joint reads energized while on_activate returns ERROR without clearing the latch, and the next successful switch legitimately clears it. Verified by mutation -- the new test fails with the disjunct removed and passes with it restored. Also corrects two pieces of prose the per-joint change had made wrong. The mode-switch failure log told operators torque would be restored "by a successful switch or by re-activating the component", but a switch only restores joints that were energized when it began, so re-activation is the answer except under torque_enable=false, where staying de-energized is intended and a successful switch does clear the error. And a test named for the removed policy of assuming a rejected torque-on had energized the servo now says what it really pins: the servo that did come back on is still tracked. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
update_legacy_heuristic() called apply_mode_switch() directly, so a mode-switch failure it triggered returned return_type::ERROR for one write() cycle but never set switch_failed_. The affected joint stayed de-energized while write() silently re-attempted the same switch on every later cycle instead of latching the fault like the perform_command_mode_switch() path already does. Extract apply_mode_switch_or_latch(), which applies the switch via apply_mode_switch() and, on failure, sets switch_failed_ and logs the existing operator-facing recovery message. Both perform_command_mode_switch() and update_legacy_heuristic() now route through this single helper, so a failure on either path latches identically. Clearing switch_failed_ on a successful switch remains perform_command_mode_switch()'s job alone, unchanged. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Adds a `## Hardware parameters` reference covering every hardware-level and per-joint parameter, including the ones introduced by this branch: torque_enable, read_error_tolerance, write_error_tolerance, gear_ratio, offset and Return_Delay_Time. Subsections spell out the gear/offset conversion contract, the two independent failure budgets, the write guard that keeps the bus silent until the first successful read, and the driver's refusal of non-finite command batches. Adds a `### Read latency and Return_Delay_Time` subsection explaining how Return_Delay_Time and the USB latency timer dominate read latency, and what to set them to (#90). Corrects statements this branch made false: the mode-switch paragraph claimed torque is re-enabled for "exactly the joints that change" and that a later switch can recover a latched fault, which per-joint torque tracking and the confirmed-only restore rule no longer allow; and the per-joint intro claimed there are only two optional parameters. Also drops the stale joint_ids reference, which no longer names a parameter the plugin reads. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Joint id parsing cast std::stoi() straight to uint8_t with no bounds check, so an out-of-range value silently wrapped instead of failing init: "300" became 44, "-1" became 255. The duplicate-id scan then compared the wrapped values, so ids that only collided after truncation (e.g. 1 and 257) were reported as a duplicate pair naming the wrong id. Reuse parse_int_param() (min_value 0) for the non-numeric and negative cases, then add an explicit upper-bound check against the largest valid individual servo id. Per dynamixel_sdk/packet_handler.h (Protocol 2.0): MAX_ID is 0xFC (252), id 253 is unused, and 254 is BROADCAST_ID, so 252 is the ceiling. The range check runs before the duplicate-id scan, so an out-of-range id is now reported for what it is instead of a false collision. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
torque_constant used a bare std::stod() inside try/catch and then rejected only <= 0.0. std::stod parses "nan" and "inf" per strtod, and neither satisfies that comparison (NaN <= 0.0 is false, inf > 0.0 is true), so both slipped through. "nan" silently degraded the joint to raw-mA effort with no warning, since the downstream > 0.0 guards in effort_command_to_motor/effort_state_from_motor also fail for NaN. "inf" zeroed every effort command on write and published +-inf/NaN on the effort state. Extend parse_double_param() with a DoubleParamRule enum expressing the three acceptance rules already in use: kFinite (offset, any finite value), kFiniteNonZero (gear_ratio, finite and non-zero, negatives legal), and kFinitePositive (torque_constant, finite and strictly positive). gear_ratio and offset now pass the rule that reproduces their previous allow_zero behavior exactly, with unchanged messages; torque_constant is routed through the helper for the first time, keeping its original wording (joint name, "(Nm/A > 0)"). Update the two README lines that called torque_constant's positivity requirement out without mentioning finiteness, and the Hardware parameters id row now that it is range-checked. Also fix two unrelated documentation inaccuracies raised alongside these defects: Return_Delay_Time is an EEPROM register, not RAM like its neighbors in that table, so it survives a mode change (the plugin still rewrites it regardless); and the finite-command check in write() happens before gear_ratio/offset/torque_constant conversion, not before every conversion -- the driver-boundary unit conversion is what runs after it. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
With torque_enable=false no joint can ever be energized: set_torque_all() returns before touching the bus and apply_mode_switch()'s re-enable leg is gated off. Every goal sync-write write() issued in that configuration was therefore a bus round-trip the servo could not act on, storing a goal that reset_joint_command() would overwrite the moment torque ever did come on. That is exactly backwards for the configuration the parameter exists for -- a leader arm in teleoperation, back-driven by hand while its states are published -- where read latency is the only thing that matters (#90). Those writes also fed handle_write_result(), so failures on writes with no purpose could escalate write() to ERROR and take the leader arm down. Only the four driver write_*() calls are skipped. tick(), the legacy heuristic and the batching loop's prev_command bookkeeping all still run: the heuristic compares each cycle's command against the previous one, so a loop that stopped recording them would pin a legacy joint in whichever mode it entered. The write-failure counter is left untouched rather than reset through handle_write_result(true), mirroring the has_valid_state_ guard above it -- a cycle that issued no driver call is neither evidence of a healthy bus nor a failure. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
read() and write() escalating to return_type::ERROR after N consecutive failures is the intended fix for #88, and it is right: a bus that has stopped answering must not look healthy. But it is also a runtime behavior change that lands on every existing user with no URDF change -- before this branch read() logged and returned OK forever, and write() returned OK regardless of what the driver reported -- and it routes the component through on_error, which disables torque and disconnects, deactivating controllers mid-motion. Both tolerances were floored at 1, so an operator on a marginal USB adapter or an electrically noisy bus -- exactly the population #90 is about -- had no configuration that restored the old ride-through behavior. 0 is now accepted on both and disables escalation for that direction. A disabled tolerance still counts and still warns: silence about a dying bus would be worse than the behavior it restores. The warning renders the denominator as "disabled" rather than "/0", and the counter saturates at INT_MAX because nothing but a successful cycle resets it any more. Negative values stay rejected -- they express nothing 0 does not. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
ensure_finite_commands() tested std::isfinite() on the double, and the very next thing every conversion does is static_cast<float>(values[i]). Any finite double above ~3.4e38 becomes inf as a float, and convertRadian2Value() / convertVelocity2Value() / convertCurrent2Value() assign their result to an int32_t -- converting a non-finite float to an integer type is undefined behavior, in a 100 Hz control loop. The window is reachable two ways: a diverging controller passing through ~1e39 for a single cycle before it reaches NaN, or an accepted pathological parameter -- gear_ratio 1e300 is finite and non-zero, so to_motor_position() produces 1e300, and torque_constant 1e-300 is finite and positive, so effort_command_to_motor() produces 1e303. Narrowing before the check closes it for all three conversions. NaN and the infinities narrow to themselves, so they are still caught, and the whole-batch-refusal semantics and the id/index/value error shape are unchanged. write_pwms() was never at risk, since duty_to_pwm_ticks() clamps to [-1, 1] first, but it shares the guard: a duty ratio that far out of range is now refused as the caller bug it is rather than silently clamped to full duty. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
The extra control-table parameters are rewritten after every mode change because a mode change resets them, and the comment in apply_mode_switch() said as much: "Extra control-table parameters live in RAM and are reset by a mode change." That stopped being true when Return_Delay_Time was appended to the list. It sits at X-series control-table address 9, in the EEPROM area -- as does Operating_Mode at 11, which is exactly why on_configure() has to set the operating mode explicitly -- and EEPROM survives a mode change. So every switch issued one extra per-joint blocking itemWrite() round-trip, on a path reachable from write(), to restore a value that was never lost. Endurance is not the concern (mode switches are human-driven and rare); the write is simply useless and the code's own comment had become wrong. Each table entry now records whether it lives in RAM, and write_extra_joint_params() takes a scope: on_configure() writes everything, since that is the one point where the servo's EEPROM is not known to match the URDF, and apply_mode_switch() writes only what the mode change reset. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Add a launch_testing integration test that brings the plugin up through ros2_control_node in dummy mode, with a two-joint test robot, the joint_state_broadcaster and a position-only joint_trajectory_controller. Beyond asserting that the nodes and the controllers come up, the test publishes a JointTrajectory and polls /joint_states until the emulated joints reach the goal. DummyDriver only moves its emulated servos from write_positions(), so convergence exercises the whole write()/read() round trip instead of merely proving that the processes started. The fixtures are located relative to __file__ because launch_testing runs the test from the source tree, so they need no install rules. This commit only adds the files; nothing runs them yet. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Register test/launch/test_dummy_bringup.launch.py through an add_ros_isolated_launch_test() helper, so that every run gets its own ROS_DOMAIN_ID, and cap it at TIMEOUT 120 so a wedged bringup fails instead of hanging the job. controller_manager has no installable rolling binaries during the Ubuntu-resolute transition, so the registration sits behind find_package(controller_manager QUIET): the test runs on humble and jazzy and auto-skips on rolling, leaving the rest of the suite intact there. Declare the new test-only dependencies in package.xml so that CI's rosdep installs them; the development images already carrying them is not the same as CI having them. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Move find_package(ament_cmake_ros REQUIRED) and find_package(launch_testing_ament_cmake REQUIRED), together with the add_ros_isolated_launch_test() helper that depends on them, inside the if(controller_manager_FOUND) block. The guard exists because rolling's dependency set is incomplete during the Ubuntu-resolute transition. Leaving REQUIRED lookups outside it turned exactly that condition into a hard configure failure: the same gap that hides controller_manager can just as easily hide these, and the graceful skip would have become a broken package on the one distro the guard was written for. Verified with cmake --trace-source over this file: on rolling only the find_package(controller_manager QUIET) line runs, while jazzy still reaches both REQUIRED lookups and the registration. humble and jazzy keep running the launch test; rolling keeps skipping it. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Add an else() branch to the controller_manager guard that logs the skip at STATUS level. The guard was silent, so a rosdep regression on a distro that is meant to run the launch test would have quietly unregistered it and left CI green with the integration coverage gone; the message makes the skip greppable, and its absence on humble and jazzy is now the positive signal that the test is still registered. STATUS rather than WARNING keeps rolling's build free of a permanent stderr block that people would learn to ignore. Declare the six packages the launch test imports directly: builtin_interfaces, launch_ros, launch_testing, rclpy, sensor_msgs and trajectory_msgs. They reached CI only transitively through the controllers and launch_testing_ros, which is the same "declare what you use" gap the other test_depend entries exist to close. Give check_node_running an explicit 15 s timeout instead of inheriting the 5 s default; it is the first check after ReadyToTest() and so absorbs the whole ros2_control_node startup on a cold runner. Correct the test_d comment, which claimed the goal appears once the trajectory finishes. Convergence within 0.05 rad is reached mid-spline, before time_from_start elapses. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Raise the dummy bringup launch test from TIMEOUT 120 to 180 and record the arithmetic next to the value. unittest runs all four sub-tests regardless of whether an earlier one failed, so the worst case is the sum of their deadlines rather than the largest: about 100-105 s once launch startup and teardown are counted. At 120 that left roughly 15% headroom, and exceeding it makes ctest report a bare Timeout, throwing away the assertion messages the test exists to produce. A passing run still takes about 3 s, so the happy path is unaffected. Document, at the publish site, an intermittent failure seen on back-to-back local runs. run_test_isolated.py takes its domain from domain_coordinator, which releases the ID on exit and immediately reissues it, so leftover discovery state can satisfy get_subscription_count() from a dead endpoint; the single publish then goes nowhere and the poll expires against a healthy bringup. Measured 0 failures in 6 independent invocations against 1 in ~40 back-to-back runs, and CI never recycles a domain, so test_d is left unchanged. The comment names the remedy that was not applied, re-publishing inside the poll, together with its behavioural cost. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
youtalk
marked this pull request as ready for review
July 25, 2026 22:44
From ros2_control 6.x on, a controller node no longer inherits the parameter file handed to ros2_control_node, so joint_trajectory_controller loaded with an empty `joints` list and refused to initialise: Invalid value set during initialization for parameter 'joints': Length of parameter 'joints' is '0' but must be greater than '0' That failed test_b and test_d on rolling and lyrical while humble and jazzy stayed green. Passing --param-file to both spawners fixes it; the flag exists on humble, jazzy and rolling alike, so one form covers every target distro and no compat branch is needed. The launch test was previously skipped on rolling because the local dev image had no controller_manager. ros2_control 6.8.0 rolling binaries were published on 2026-07-25, so CI's rosdep now installs them, the find_package guard succeeds and the test really runs there. The local rolling image was extended to match CI before this fix was verified. Verified in the containers, clean rebuild of the package each time: rolling 200 tests / 0 errors / 0 failures, humble 201 / 0 / 0, jazzy 201 / 0 / 0. Rolling gains the launch test it used to skip. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
youtalk
added a commit
that referenced
this pull request
Jul 25, 2026
#115) * test: cover port_name/usb_port fallback and deprecation warning Regression tests for the canonical port_name parameter (#87, #86): usb_port is still accepted with a one-time deprecation warning, port_name wins when both are given, and a missing port parameter is reported as an init error instead of an uncaught exception. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: add torque_enable hardware parameter When torque_enable is false the plugin never turns servo torque on: on_activate skips the torque-on and the mode-switch sequencing skips the re-enable, while explicit torque-off requests still go through. This supports leader arms in teleoperation setups that must stay freely movable. Re-implements #106 on the driver-abstraction base. Co-authored-by: Maverobot <Maverobot@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: clear switch_failed_ latch when torque_enable is false and assert torque-off still runs Two gaps from the initial torque_enable implementation: - perform_command_mode_switch() only cleared the fault latch when torque_enabled_ was true, but with torque_enable=false torque can never become enabled, so any transient mode-switch failure latched switch_failed_ permanently and every later write() escalated into on_error(), tearing the component down for a leader arm that is behaving exactly as configured. The latch now also clears on a successful switch when torque_enable_param_ is false, since staying de-energized is the intended healthy outcome in that configuration. - added a test asserting that torque-off requests (on_deactivate) still reach the driver when torque_enable is false, pinning the safety- relevant half of the guard that was previously unasserted. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: tolerate transient read failures via read_error_tolerance read() now holds the last-known state and returns OK for up to read_error_tolerance - 1 consecutive read_states failures (hardware parameter, default 5); the Nth consecutive failure returns return_type::ERROR so the controller manager can react (on_error on jazzy+). Any successful read resets the counter. Fixes the spurious component shutdowns on momentarily noisy buses reported in #88. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: guard write() until the first successful read write() now sends nothing (and returns OK) until read() has succeeded once after activation; that first success also re-syncs commands to the actual servo state via reset_command(). A bus where reads fail can therefore never produce a zero/NaN goal position and the arm no longer slams into itself on flaky startup. The has_valid_state_ latch lives in the shared read_joint_states() helper so it fires from both read() and on_activate()'s initial read without duplicating the logic. on_activate() now treats that initial read as best-effort: activation succeeds even when it fails, relying on the write guard to keep the bus silent until a read lands. Driver write failures are now logged and counted against a new write_error_tolerance hardware parameter (default 5, separate from read_error_tolerance) instead of being silently ignored. Fixes #92. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * refactor: extract parse_int_param to dedupe hardware-parameter parsing baud_rate, read_error_tolerance and write_error_tolerance each had the same find -> stoi/try-catch -> range-check -> ERROR skeleton inlined in init_impl, and the queued gear_ratio/offset/Return_Delay_Time tasks would each have copied it again. Factor the shape into parse_int_param(params, name, out, min_value); callers keep the parameter-specific "is it required" logic (baud_rate still errors when absent and not in dummy mode) on top of it. Also give baud_rate an explicit lower bound: it is passed on as the uint32_t argument to DynamixelWorkbench::init(), so a zero or negative value would be meaningless or silently wrap. Behavior is otherwise unchanged: identical log wording, identical CallbackReturn::ERROR outcomes on parse failure or an out-of-range value, and an absent key still keeps the compiled-in default. Also add a write_error_tolerance = 1 boundary test: the first write failure must escalate to ERROR immediately, with no one-failure grace period. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: add per-joint gear_ratio parameter gear_ratio (default 1.0) is the number of motor revolutions per joint revolution. The plugin converts between joint-side and motor-side values on every read/write path: positions and velocities are divided by the ratio on read and multiplied on write, efforts are multiplied on read and divided on write, composing with the existing torque_constant scaling. PWM duty ratios are left unconverted. The conversion helpers also take an offset term, added by Task 7. Negative ratios invert the direction; zero is rejected at init via the new parse_double_param helper, a sibling of parse_int_param. Re-implements #95 on the driver-abstraction base. Closes #94. Co-authored-by: Tacha-S <13605820+Tacha-S@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: add per-joint offset parameter offset (default 0.0, joint-side radians) shifts the reported position: joint_reported = raw - offset, and position commands add the offset back before being sent. Combined with gear_ratio the pipeline is joint = motor / gear_ratio - offset. Velocity, effort, and PWM are unaffected. parse_double_param and parse_int_param now take an optional joint name, included in their error messages for per-joint parameters (and omitted for hardware-level ones like baud_rate), so a typo'd gear_ratio or offset value is traceable on a multi-joint robot. This re-implements the intent of the URDF <calibration rising> support from #96 as a plain <ros2_control> param instead, since HardwareInfo exposes <ros2_control> params uniformly on all four supported distros. Closes #93. Co-authored-by: Tacha-S <13605820+Tacha-S@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: assert pwm is unaffected by gear_ratio and offset Task 7 review flagged that PWM's independence from offset (and, transitively, gear_ratio) was only stated in comments, never exercised by a test -- offset's correction explicitly required this to be asserted, not just structurally implied. Add PwmIsUnaffectedByGearRatioAndOffset to ParamsRobustnessTest: a joint configured for pwm control, with a non-default gear_ratio AND offset, must see its commanded duty ratio reach write_pwms() unchanged. Covering both parameters in one test closes the equivalent gap left open for gear_ratio alone by the prior task's review. Verified the assertion is load-bearing by temporarily reintroducing an offset subtraction on the pwm write path and confirming the test fails before reverting. ParamsRobustnessTest gains command-interface export and a set_command_value() helper (mirroring the existing state_value() pattern) since driving write() into the PWM branch requires setting joint.command.pwm directly -- reset_command() always resets it to 0.0, so there is no other way to give it a distinguishing non-zero value. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: write Return_Delay_Time from joint parameters Return_Delay_Time joins kExtraJointParameters, so a joint-level <param name="Return_Delay_Time">0</param> is written to the control table on configure. Setting it to 0 removes the 500 us default reply delay per servo and is the main mitigation for the read latency reported in #90. Re-implements #105. Co-authored-by: Ignacio Davila <99193391+IDavGal@users.noreply.github.com> Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: reject duplicate joint ids at initialization Two <joint> entries sharing one Dynamixel id gave the plugin two independent command, state and control-mode records for a single servo, which then fought each other on every write cycle. The configuration is now rejected during on_init, naming the id and both joints, instead of silently misbehaving at runtime. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: reject non-finite commands before tick conversion A NaN or infinite command reached convertRadian2Value, convertVelocity2Value, convertCurrent2Value or duty_to_pwm_ticks and was narrowed to int32_t, which is undefined behavior: an unspecified goal value went to a torqued servo and FE_INVALID was raised, with no error reported. All four write paths now refuse the whole batch and report which id carried the non-finite value. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: cover ids/values length mismatch in ensure_finite_commands Adds a regression test for the ids/values size guard added alongside the finite-value check, so the length-mismatch branch is exercised and not just implemented. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: track servo torque state per joint A single plugin-wide torque flag could not express the state a partial failure leaves behind: energizing joint 1 and failing on joint 2 left the flag false while joint 1 was under power, so the next mode switch skipped the mandatory torque-off leg and the firmware rejected the Operating_Mode write. Torque state now lives on each joint, updated as each driver call returns, and the mode-switch fault latch clears only when every joint is energized again. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * build: restore the humble build of the test suite test_dynamixel_hardware.cpp uses hardware_interface::HW_IF_* at eleven sites but never included the header that defines them. jazzy and rolling supply it transitively through hardware_interface/hardware_component_interface.hpp, which humble has no equivalent of, so only humble failed to compile -- and every task since the breakage was introduced verified on jazzy alone. The header path, namespace and symbol spellings are identical on all four target distros, so a plain include is correct and no compat.hpp gate applies. Naming the parameters of the ReadReturns action type is a second humble-only fix: uncrustify 0.72 (humble) and 0.78 (jazzy and later) disagree about the space in a function type ending in "&)", and no spelling of that construct satisfies both. Naming the parameters removes the construct they disagree on. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: record joint torque only once the driver confirms it A rejected set_torque(id, true) was still recorded as energized, on the reasoning that assuming the worst kept the next switch from skipping the mandatory torque-off leg. That assumption is conservative for the mode write but optimistic for the fault latch, and the latch is what reads the flag: two joints switched, joint 2's re-enable rejected, switch_failed_ latched, then a later switch on joint 1 alone found all_torque_enabled() true, cleared the fault, and write() resumed sync-writing goals to a limp servo. The torque-off leg no longer skips a joint it believes is already off -- a joint that is merely not known to be energized may still be torqued, and the redundant torque-off is idempotent -- so nothing depends on the pessimistic value any more, and the flag is now set only after the driver confirms torque on. Restoring torque remains limited to the joints that were confirmed energized on entry, so a mode switch still never powers up servos left limp by a deactivation, a torque_enable=false configuration or an earlier failure. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: pin the all_torque_enabled branch of the latch clear Deleting "all_torque_enabled() ||" from the latch-clearing condition left the whole suite green, so the true branch was unpinned. It is reachable: an activation rejected on a joint that was ALREADY confirmed energized leaves that joint's flag untouched, so every joint reads energized while on_activate returns ERROR without clearing the latch, and the next successful switch legitimately clears it. Verified by mutation -- the new test fails with the disjunct removed and passes with it restored. Also corrects two pieces of prose the per-joint change had made wrong. The mode-switch failure log told operators torque would be restored "by a successful switch or by re-activating the component", but a switch only restores joints that were energized when it began, so re-activation is the answer except under torque_enable=false, where staying de-energized is intended and a successful switch does clear the error. And a test named for the removed policy of assuming a rejected torque-on had energized the servo now says what it really pins: the servo that did come back on is still tracked. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: latch switch_failed_ when the legacy heuristic's switch fails update_legacy_heuristic() called apply_mode_switch() directly, so a mode-switch failure it triggered returned return_type::ERROR for one write() cycle but never set switch_failed_. The affected joint stayed de-energized while write() silently re-attempted the same switch on every later cycle instead of latching the fault like the perform_command_mode_switch() path already does. Extract apply_mode_switch_or_latch(), which applies the switch via apply_mode_switch() and, on failure, sets switch_failed_ and logs the existing operator-facing recovery message. Both perform_command_mode_switch() and update_legacy_heuristic() now route through this single helper, so a failure on either path latches identically. Clearing switch_failed_ on a successful switch remains perform_command_mode_switch()'s job alone, unchanged. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * docs: document hardware parameters and read-latency guidance Adds a `## Hardware parameters` reference covering every hardware-level and per-joint parameter, including the ones introduced by this branch: torque_enable, read_error_tolerance, write_error_tolerance, gear_ratio, offset and Return_Delay_Time. Subsections spell out the gear/offset conversion contract, the two independent failure budgets, the write guard that keeps the bus silent until the first successful read, and the driver's refusal of non-finite command batches. Adds a `### Read latency and Return_Delay_Time` subsection explaining how Return_Delay_Time and the USB latency timer dominate read latency, and what to set them to (#90). Corrects statements this branch made false: the mode-switch paragraph claimed torque is re-enabled for "exactly the joints that change" and that a later switch can recover a latched fault, which per-joint torque tracking and the confirmed-only restore rule no longer allow; and the per-joint intro claimed there are only two optional parameters. Also drops the stale joint_ids reference, which no longer names a parameter the plugin reads. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: reject out-of-range Dynamixel ids at initialization Joint id parsing cast std::stoi() straight to uint8_t with no bounds check, so an out-of-range value silently wrapped instead of failing init: "300" became 44, "-1" became 255. The duplicate-id scan then compared the wrapped values, so ids that only collided after truncation (e.g. 1 and 257) were reported as a duplicate pair naming the wrong id. Reuse parse_int_param() (min_value 0) for the non-numeric and negative cases, then add an explicit upper-bound check against the largest valid individual servo id. Per dynamixel_sdk/packet_handler.h (Protocol 2.0): MAX_ID is 0xFC (252), id 253 is unused, and 254 is BROADCAST_ID, so 252 is the ceiling. The range check runs before the duplicate-id scan, so an out-of-range id is now reported for what it is instead of a false collision. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: reject non-finite torque_constant values at initialization torque_constant used a bare std::stod() inside try/catch and then rejected only <= 0.0. std::stod parses "nan" and "inf" per strtod, and neither satisfies that comparison (NaN <= 0.0 is false, inf > 0.0 is true), so both slipped through. "nan" silently degraded the joint to raw-mA effort with no warning, since the downstream > 0.0 guards in effort_command_to_motor/effort_state_from_motor also fail for NaN. "inf" zeroed every effort command on write and published +-inf/NaN on the effort state. Extend parse_double_param() with a DoubleParamRule enum expressing the three acceptance rules already in use: kFinite (offset, any finite value), kFiniteNonZero (gear_ratio, finite and non-zero, negatives legal), and kFinitePositive (torque_constant, finite and strictly positive). gear_ratio and offset now pass the rule that reproduces their previous allow_zero behavior exactly, with unchanged messages; torque_constant is routed through the helper for the first time, keeping its original wording (joint name, "(Nm/A > 0)"). Update the two README lines that called torque_constant's positivity requirement out without mentioning finiteness, and the Hardware parameters id row now that it is range-checked. Also fix two unrelated documentation inaccuracies raised alongside these defects: Return_Delay_Time is an EEPROM register, not RAM like its neighbors in that table, so it survives a mode change (the plugin still rewrites it regardless); and the finite-command check in write() happens before gear_ratio/offset/torque_constant conversion, not before every conversion -- the driver-boundary unit conversion is what runs after it. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: skip goal sync-writes when torque_enable is false With torque_enable=false no joint can ever be energized: set_torque_all() returns before touching the bus and apply_mode_switch()'s re-enable leg is gated off. Every goal sync-write write() issued in that configuration was therefore a bus round-trip the servo could not act on, storing a goal that reset_joint_command() would overwrite the moment torque ever did come on. That is exactly backwards for the configuration the parameter exists for -- a leader arm in teleoperation, back-driven by hand while its states are published -- where read latency is the only thing that matters (#90). Those writes also fed handle_write_result(), so failures on writes with no purpose could escalate write() to ERROR and take the leader arm down. Only the four driver write_*() calls are skipped. tick(), the legacy heuristic and the batching loop's prev_command bookkeeping all still run: the heuristic compares each cycle's command against the previous one, so a loop that stopped recording them would pin a legacy joint in whichever mode it entered. The write-failure counter is left untouched rather than reset through handle_write_result(true), mirroring the has_valid_state_ guard above it -- a cycle that issued no driver call is neither evidence of a healthy bus nor a failure. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * feat: accept a zero error tolerance as "never escalate" read() and write() escalating to return_type::ERROR after N consecutive failures is the intended fix for #88, and it is right: a bus that has stopped answering must not look healthy. But it is also a runtime behavior change that lands on every existing user with no URDF change -- before this branch read() logged and returned OK forever, and write() returned OK regardless of what the driver reported -- and it routes the component through on_error, which disables torque and disconnects, deactivating controllers mid-motion. Both tolerances were floored at 1, so an operator on a marginal USB adapter or an electrically noisy bus -- exactly the population #90 is about -- had no configuration that restored the old ride-through behavior. 0 is now accepted on both and disables escalation for that direction. A disabled tolerance still counts and still warns: silence about a dying bus would be worse than the behavior it restores. The warning renders the denominator as "disabled" rather than "/0", and the counter saturates at INT_MAX because nothing but a successful cycle resets it any more. Negative values stay rejected -- they express nothing 0 does not. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: check command finiteness in the type the conversion uses ensure_finite_commands() tested std::isfinite() on the double, and the very next thing every conversion does is static_cast<float>(values[i]). Any finite double above ~3.4e38 becomes inf as a float, and convertRadian2Value() / convertVelocity2Value() / convertCurrent2Value() assign their result to an int32_t -- converting a non-finite float to an integer type is undefined behavior, in a 100 Hz control loop. The window is reachable two ways: a diverging controller passing through ~1e39 for a single cycle before it reaches NaN, or an accepted pathological parameter -- gear_ratio 1e300 is finite and non-zero, so to_motor_position() produces 1e300, and torque_constant 1e-300 is finite and positive, so effort_command_to_motor() produces 1e303. Narrowing before the check closes it for all three conversions. NaN and the infinities narrow to themselves, so they are still caught, and the whole-batch-refusal semantics and the id/index/value error shape are unchanged. write_pwms() was never at risk, since duty_to_pwm_ticks() clamps to [-1, 1] first, but it shares the guard: a duty ratio that far out of range is now refused as the caller bug it is rather than silently clamped to full duty. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * fix: rewrite only the RAM control-table parameters after a mode change The extra control-table parameters are rewritten after every mode change because a mode change resets them, and the comment in apply_mode_switch() said as much: "Extra control-table parameters live in RAM and are reset by a mode change." That stopped being true when Return_Delay_Time was appended to the list. It sits at X-series control-table address 9, in the EEPROM area -- as does Operating_Mode at 11, which is exactly why on_configure() has to set the operating mode explicitly -- and EEPROM survives a mode change. So every switch issued one extra per-joint blocking itemWrite() round-trip, on a path reachable from write(), to restore a value that was never lost. Endurance is not the concern (mode switches are human-driven and rare); the write is simply useless and the code's own comment had become wrong. Each table entry now records whether it lives in RAM, and write_extra_joint_params() takes a scope: on_configure() writes everything, since that is the one point where the servo's EEPROM is not known to match the URDF, and apply_mode_switch() writes only what the mode change reset. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: add a dummy-mode bringup launch test for DynamixelHardware Add a launch_testing integration test that brings the plugin up through ros2_control_node in dummy mode, with a two-joint test robot, the joint_state_broadcaster and a position-only joint_trajectory_controller. Beyond asserting that the nodes and the controllers come up, the test publishes a JointTrajectory and polls /joint_states until the emulated joints reach the goal. DummyDriver only moves its emulated servos from write_positions(), so convergence exercises the whole write()/read() round trip instead of merely proving that the processes started. The fixtures are located relative to __file__ because launch_testing runs the test from the source tree, so they need no install rules. This commit only adds the files; nothing runs them yet. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: register the dummy bringup launch test with ctest Register test/launch/test_dummy_bringup.launch.py through an add_ros_isolated_launch_test() helper, so that every run gets its own ROS_DOMAIN_ID, and cap it at TIMEOUT 120 so a wedged bringup fails instead of hanging the job. controller_manager has no installable rolling binaries during the Ubuntu-resolute transition, so the registration sits behind find_package(controller_manager QUIET): the test runs on humble and jazzy and auto-skips on rolling, leaving the rest of the suite intact there. Declare the new test-only dependencies in package.xml so that CI's rosdep installs them; the development images already carrying them is not the same as CI having them. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: scope the launch-test find_package calls to the guard Move find_package(ament_cmake_ros REQUIRED) and find_package(launch_testing_ament_cmake REQUIRED), together with the add_ros_isolated_launch_test() helper that depends on them, inside the if(controller_manager_FOUND) block. The guard exists because rolling's dependency set is incomplete during the Ubuntu-resolute transition. Leaving REQUIRED lookups outside it turned exactly that condition into a hard configure failure: the same gap that hides controller_manager can just as easily hide these, and the graceful skip would have become a broken package on the one distro the guard was written for. Verified with cmake --trace-source over this file: on rolling only the find_package(controller_manager QUIET) line runs, while jazzy still reaches both REQUIRED lookups and the registration. humble and jazzy keep running the launch test; rolling keeps skipping it. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: announce the launch-test skip and declare its direct deps Add an else() branch to the controller_manager guard that logs the skip at STATUS level. The guard was silent, so a rosdep regression on a distro that is meant to run the launch test would have quietly unregistered it and left CI green with the integration coverage gone; the message makes the skip greppable, and its absence on humble and jazzy is now the positive signal that the test is still registered. STATUS rather than WARNING keeps rolling's build free of a permanent stderr block that people would learn to ignore. Declare the six packages the launch test imports directly: builtin_interfaces, launch_ros, launch_testing, rclpy, sensor_msgs and trajectory_msgs. They reached CI only transitively through the controllers and launch_testing_ros, which is the same "declare what you use" gap the other test_depend entries exist to close. Give check_node_running an explicit 15 s timeout instead of inheriting the 5 s default; it is the first check after ReadyToTest() and so absorbs the whole ros2_control_node startup on a cold runner. Correct the test_d comment, which claimed the goal appears once the trajectory finishes. Convergence within 0.05 rad is reached mid-spline, before time_from_start elapses. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: raise the launch-test timeout and record the domain-ID race Raise the dummy bringup launch test from TIMEOUT 120 to 180 and record the arithmetic next to the value. unittest runs all four sub-tests regardless of whether an earlier one failed, so the worst case is the sum of their deadlines rather than the largest: about 100-105 s once launch startup and teardown are counted. At 120 that left roughly 15% headroom, and exceeding it makes ctest report a bare Timeout, throwing away the assertion messages the test exists to produce. A passing run still takes about 3 s, so the happy path is unaffected. Document, at the publish site, an intermittent failure seen on back-to-back local runs. run_test_isolated.py takes its domain from domain_coordinator, which releases the ID on exit and immediately reissues it, so leftover discovery state can satisfy get_subscription_count() from a dead endpoint; the single publish then goes nowhere and the poll expires against a healthy bringup. Measured 0 failures in 6 independent invocations against 1 in ~40 back-to-back runs, and CI never recycles a domain, so test_d is left unchanged. The comment names the remedy that was not applied, re-publishing inside the poll, together with its behavioural cost. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * test: pass the controller parameters to the spawners explicitly From ros2_control 6.x on, a controller node no longer inherits the parameter file handed to ros2_control_node, so joint_trajectory_controller loaded with an empty `joints` list and refused to initialise: Invalid value set during initialization for parameter 'joints': Length of parameter 'joints' is '0' but must be greater than '0' That failed test_b and test_d on rolling and lyrical while humble and jazzy stayed green. Passing --param-file to both spawners fixes it; the flag exists on humble, jazzy and rolling alike, so one form covers every target distro and no compat branch is needed. The launch test was previously skipped on rolling because the local dev image had no controller_manager. ros2_control 6.8.0 rolling binaries were published on 2026-07-25, so CI's rosdep now installs them, the find_package guard succeeds and the test really runs there. The local rolling image was extended to match CI before this fix was verified. Verified in the containers, clean rebuild of the package each time: rolling 200 tests / 0 errors / 0 failures, humble 201 / 0 / 0, jazzy 201 / 0 / 0. Rolling gains the launch test it used to skip. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * ci: add Mergify backport automation Three label-driven backport rules (backport-humble, backport-jazzy, backport-lyrical) with base=rolling, a conflict nag for authors, a maintainer ping for conflicted Mergify backports, and a reminder comment for pull requests not targeting rolling. Mirrors the ros-controls configuration. Schema verified against the current Mergify docs (pull_request_rules, conditions operators, and the backport/comment action keys used here are all current). backport-lyrical stays dormant until the lyrical branch and the backport-* labels both exist; neither the labels nor the app installation are created by this commit. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * docs: backfill changelog for 1.0.0 Accumulate the modernization campaign under the standard Forthcoming heading: CI overhaul (#109), build modernization (#110), driver abstraction (#111), the control-mode rework (#112), the parameter and read/write robustness work (#113), integration tests (#114), and this PR's Mergify automation and README rewrite. catkin_prepare_release rewrites Forthcoming to 1.0.0 at release time. This PR's own bullet has no issue/pull-request number yet, since it is not opened until after this commit (the branch's push and draft-PR creation happen separately); add the number once it exists, following the existing precedent of the unnumbered CMake-hotfix bullet in the 0.6.1 section. Contributors are cross-referenced against the community PRs/issues whose features this campaign actually reimplements, not commit trailers: none of the campaign's commits carry Co-authored-by lines, so that lookup path (as originally planned) returns nothing. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * docs: rewrite README for the 1.0.0 line Restructure the README around what a first-time user needs in order: what the plugin does, which distro to install, how to configure it, how to run it, and how to build and test it. New sections: CI badges and a supported-distro/branch table, a feature summary, a distro-neutral installation section (the old one told users to put Rolling on Ubuntu 22.04), a control-mode reference listing the control-table item each mode requires of the servo model, notes on the framework-side rw_rate and is_async attributes, a migration note for 0.x URDFs, container-based build and test instructions, and the Mergify backport workflow for contributors. The parameter tables, the gearing/offset, error-tolerance and read-latency sections and the mode-switch description are carried over from the previous pass, which verified them against the source. Three stale claims are dropped: the Ubuntu 22.04 line, the quoted example diff that contradicted the usb_port deprecation notice, and an unmeasured assertion about the loop rate a 4-5 servo bus sustains. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * ci: clone the examples repository's main branch in the nightly job The downstream-examples job cloned dynamixel_hardware_examples with -b ${{ matrix.branch }}, but that repository has exactly one branch, main. Every matrix entry (humble, jazzy, lyrical, rolling) therefore failed at the checkout step with "Remote branch <distro> not found in upstream origin", so the downstream build has never actually run. Clone main unconditionally. The two repositories deliberately use different branch models -- one branch per distro here, a single main branch there -- so the fix is to stop parameterising the clone rather than to create branches in the examples repository. A comment records that asymmetry at the clone, since it is what produced the bug. matrix.branch now appears only on the two dynamixel_hardware checkouts, which are the only branch-per-distro operations in the workflow. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * docs: correct the README's release and build guidance The apt line implied a binary for every supported distro. rosdistro master has no release section for dynamixel_hardware in humble, so ros-humble-dynamixel-hardware does not exist; name the distros that do have one (jazzy, kilted, lyrical, rolling) and send humble users to the source build. Kilted had fallen out of the document entirely. It is deliberately not part of the 1.0.0 line, but it is a supported distro with a released 0.6.0 binary, so say so rather than leaving its users with no answer. The source build cloned -b $ROS_DISTRO, which fails outright on lyrical (that branch is cut as part of this release) and silently yields the 0.6.x line on humble and jazzy, neither of which contains anything this README describes. Clone the default rolling branch instead and explain why one branch serves every distro. Also: baud_rate is parsed and range-checked before the dummy driver takes over, so it is not "irrelevant" under use_dummy; only plain position mode clamps to +-pi, so name the three modes that do not; and split the container block, whose in-container lines carried a '#' prompt that made them read as shell comments. The launch-test paragraph now describes a test that is present on this branch, so it names the file, the isolation runner and the skip message verbatim. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * docs: correct the changelog contributor list and #113 bullet The contributor list was derived on the premise that no commit in this repository carries a Co-authored-by trailer. It does: a per-commit sweep of the Forthcoming range (f008ad6..HEAD) yields exactly eight names, and 010f774 names Ignacio Davila in a trailer. Fix two defects against that ground truth. IDavGal was the GitHub handle, not the name in the trailer. Betacrucis has no git evidence anywhere in the repository -- author, committer or trailer -- and is an issue-only reporter, which is the same reason the list already excludes BenKlee; applying that rule to one name and not the other was the actual inconsistency. The #113 bullet also listed every finalized parameter except write_error_tolerance, which the code enforces and the README documents as a first-class parameter. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> * ci: make the Mergify backport-action note stand alone The header comment pointed at the README for the korthout/backport- action fallback, but the README rewrite deliberately dropped that line: it is a maintainer contingency, not user-facing documentation. That left "see README" resolving to nothing. State the fallback in the comment itself, where the person who would act on it is already reading. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> --------- Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp> Co-authored-by: Maverobot <Maverobot@users.noreply.github.com> Co-authored-by: Tacha-S <13605820+Tacha-S@users.noreply.github.com> Co-authored-by: Ignacio Davila <99193391+IDavGal@users.noreply.github.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Adds a
launch_testingintegration test that brings up the whole stack in dummy mode and proves thewrite()/read()round trip end to end.Until now the package's tests all exercised
DynamixelHardwarethrough a gmock driver. Nothing verified that the plugin actually loads under a realcontroller_manager, that its exported interfaces are claimable by real controllers, or that a trajectory command travels throughwrite(), into the emulated servo, back out throughread()and onto/joint_states.Changes
dynamixel_hardware/test/launch/test_robot.urdf.xacro— a minimal two-joint robot with ause_dummyxacro arg defaulting totrue.dynamixel_hardware/test/launch/test_controllers.yaml— a 100 Hzcontroller_managerwithjoint_state_broadcasterand a position-onlyjoint_trajectory_controller.dynamixel_hardware/test/launch/test_dummy_bringup.launch.py— the launch test:ros2_control_nodefed byrobot_state_publisherwith the xacro-processed description, controller spawners,ReadyToTest, then four assertions — thecontroller_managernode is running, both controllers reachactive,/joint_statesis published, and a JTC trajectory converges to its goal within tolerance.add_ros_isolated_launch_test(isolatedROS_DOMAIN_IDper test) withTIMEOUT 180, plus the<test_depend>entries CI's rosdep needs.What the convergence assertion actually proves
/joint_statesis published byjoint_state_broadcasterfrom state interfaces, andDummyDriver::write_positionsis the only writer of the emulated servo position — so the goal can only appear there by travelling through the plugin'swrite(), into the emulated servo, and back out throughread(). A missing joint yieldsfloat('inf')rather than a default, so absent data and mismatched joint names fail rather than pass, and an expired deadline is a failure, not a pass.This was verified by mutation rather than by argument. Three independent broken-plugin states were observed and each fails a different subset: with the plugin unloadable only the node check passes; with
write()neutered (via a temporarytorque_enable=false, which makes the emulated robot freeze) the first three pass and only convergence fails, reportinglast seen: {'joint1': 0.0, 'joint2': 0.0}; withread()returning NaN convergence fails too. A liveness-only test would have passed all three.Distro coverage
The test is registered behind
find_package(controller_manager QUIET), so a runner withoutros2_controlskips it cleanly rather than failing the build. The skip announces itself with amessage(STATUS ...)— deliberatelySTATUSrather thanWARNING, so it does not produce a permanent--- stderrblock — because a silent guard means that if rosdep ever fails to installcontroller_manager, the integration test would quietly stop being registered and CI would stay green without it.The test currently runs on all four target distros.
ros2_control6.8.0 rolling binaries were published on 2026-07-25, which closed the gap that this guard was originally written for; the guard stays because it is the difference between a graceful skip and a hard build failure if that gap ever reopens.Testing
Clean-build container gate on this branch head, package build directories removed each time: rolling 200 tests / 0 errors / 0 failures, humble 201 / 0 / 0, jazzy 201 / 0 / 0. 10/10 consecutive launch-test runs on humble and jazzy at 2.9–4.6 s against the 180 s timeout.
Known local-only flake, documented in the test
One humble run in roughly forty expired the convergence poll on a fully healthy bringup. Traced to
domain_coordinator.domain_id()releasing its ID on process exit and reissuing the same one to the next process, so residual DDS state from the previous run satisfiesget_subscription_count() > 0from a dead endpoint and the singlepublish()goes nowhere. It appears only on back-to-back local invocations — zero failures in six independent invocations, which is the pattern CI runs. The mechanism, the recognisable symptom and the unapplied remedy (re-publishing inside the poll, which would make the JTC replace its active trajectory every iteration and change what the test exercises) are all recorded in a comment at the publish site, so that if CI ever does show it, the diagnosis is already there.