From ce753882ac9157b1de815a4999ac235b47bd42d3 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 31 Jul 2026 15:06:57 +0100 Subject: [PATCH] Move jog and arm-pose onto the shared CLI plumbing (they abort on exit today) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tools destroy their node while rclpy.spin() still holds it, which pulls the executor out from under itself: the interpreter calls std::terminate and the process exits 134 with "terminate called without an active exception" — after the tool has already done its work. So the run succeeds and the operator sees a crash. This was believed latent. It is not. With no stack running to talk to, `jog` (stdin closed) and `arm-pose list` each aborted 3 of 3 runs on this branch's parent, and exit 0 on 3 of 3 after. The 0.2 s sleep that used to mask it in jog went away with the ros2_control fold (#74), so nothing has been hiding it since; it simply had not been looked for, because the arm is already limp by then and the message scrolls past. The fix is ordering, and it belongs in one place rather than in each main(): shut the context down, *join the spin thread*, and only then destroy. cli.spin_background/cli.shutdown are taken verbatim from the arm/virtual-leader -teleop branch, where they were written for the same bug in episode_record and have not landed yet — byte-identical, so that branch rebases onto this without a conflict. The argument half turned out to be the mirror image of what was expected. Neither tool uses parse_known_args (nothing in mote_arm does); arm_pose uses a strict parse_args over the whole command line, so ROS's own arguments are not discarded, they are fatal — `arm-pose list --ros-args -p use_sim_time:=true` dies with "unrecognized arguments". cli.parse cuts the `--ros-args ... --` block out first and then parses strictly, so ROS arguments pass through and a mistyped safety flag (`--max_travel`, `--speeed`) is still an error rather than silently dropped onto the default. jog takes no arguments of its own — its one input is the robot_yaml ROS parameter — so there is nothing there to parse; it gets the shutdown half only. arm_pose's parser moves into build_parser() so the strictness is testable without standing a node up. test_cli.py pins both properties. The abort is checked by watching a child process's exit status, because nothing in-process can catch an abort. Verified: 178 mote_arm tests pass, lint clean, and the two reproductions above re-run green. Not verified: anything on the real arm — BENCH.md steps 5-7 want a re-run, and step 7 now says to check the exit status rather than only the message (it was also still quoting the pre-#74 "torque off" wording). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013uSB4sBqvYnsZRAEqLR4U8 --- CLAUDE.md | 14 ++++++ mote_arm/BENCH.md | 26 ++++++++-- mote_arm/README.md | 27 ++++++++++ mote_arm/mote_arm/arm_pose.py | 26 +++------- mote_arm/mote_arm/cli.py | 81 ++++++++++++++++++++++++++++++ mote_arm/mote_arm/jog.py | 23 ++------- mote_arm/test/test_cli.py | 92 +++++++++++++++++++++++++++++++++++ 7 files changed, 246 insertions(+), 43 deletions(-) create mode 100644 mote_arm/mote_arm/cli.py create mode 100644 mote_arm/test/test_cli.py diff --git a/CLAUDE.md b/CLAUDE.md index fb09e20..887f692 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -294,6 +294,20 @@ section. Contains: - `jog` (CLI, `pixi run arm-jog`) — interactive per-joint jog; a *client of `arm_controller`* (publishes clamped single-point trajectories, limps on exit). It never opens the bus, so there is no contention to guard against. +- **Every arm CLI exits and parses through `cli.py`**, because both properties + fail silently when hand-rolled. `cli.shutdown(node, spinner)` shuts the + context down, **joins the spin thread, and only then destroys the node**: + destroying a node `spin()` still holds aborts the interpreter (exit 134, + "terminate called without an active exception") *after* the tool has done its + work, so the run succeeds and the process still crashes — measured on `jog` + and `arm-pose list`, 3 of 3 runs each, with no hardware attached. This is not + a rare race, so a new arm CLI must not hand-roll the teardown. + `cli.parse(parser)` cuts the `--ros-args ... --` block out and then parses + strictly: `ros2 run` hands the tool ROS's arguments too, so a plain + `parse_args` rejects `--ros-args` outright while `parse_known_args` — the + usual workaround — silently discards a mistyped `--max-travel` or `--speed` + and drives on the default. `test_cli.py` pins both, the abort via a child + process's exit status since nothing in-process can catch it. - `arm_check` (`pixi run arm-check`) — standalone read-only enumeration/health + `--save-zero` calibration snapshot. Run with the driver stopped (same port). - **`zero` is not `home`.** `robot.yaml`'s `arm.joints[].zero` is the encoder diff --git a/mote_arm/BENCH.md b/mote_arm/BENCH.md index fc2775f..59157ce 100644 --- a/mote_arm/BENCH.md +++ b/mote_arm/BENCH.md @@ -323,12 +323,28 @@ motion stops at the configured `max` and the driver logs many times you press `+`. Repeat toward the lower limit. **This is acceptance criterion 2.** -## Step 7 — torque-off on exit +## Step 7 — torque-off on exit, and a clean exit -In `arm-jog`, type `quit`. **Expected:** `limping arm (torque off) and -exiting...`; the arm goes back-drivable immediately. Stop `arm` (Ctrl-C) and -confirm it also logs a clean shutdown and leaves the arm limp. **Nothing should -move on startup or shutdown.** +In `arm-jog`, type `quit`. **Expected:** `limping arm (deactivating +arm_controller) and exiting...`; the arm goes back-drivable immediately. Stop +`arm` (Ctrl-C) and confirm it also logs a clean shutdown and leaves the arm +limp. **Nothing should move on startup or shutdown.** + +Then check the exit *status*, not just the message — the arm is already limp by +the time the process falls over, so an abort here is invisible unless looked +for: + +``` +pixi run arm-jog # 'quit' at the prompt +echo $? # expect 0 +pixi run arm-pose list +echo $? # expect 0 +``` + +**Expected:** `0` from both, and no `terminate called without an active +exception` on stderr. A `134` is the destroy-while-spinning abort (see README, +"Exits and arguments"); it means the tool did its job and then crashed on the +way out. --- diff --git a/mote_arm/README.md b/mote_arm/README.md index bbbf541..217b474 100644 --- a/mote_arm/README.md +++ b/mote_arm/README.md @@ -168,6 +168,7 @@ conversions are verified without hardware. | `config.py` | Parses the `arm:` section; encoder<->radian conversion + soft-limit clamping. | | `bus.py` | `FeetechBus` — thin `scservo_sdk` wrapper (ping, read position/health, torque, position goal, homing offset). Lazy SDK import. | | `control.py` | The one place that knows how to talk to `arm_controller`: single-point trajectories, and activation as the torque switch. | +| `cli.py` | The plumbing every arm CLI shares: strict argument parsing with ROS's own arguments cut out first, and a shutdown that stops spinning before it destroys the node. Both are properties that fail silently otherwise — see "Exits and arguments" below. | | `arm_launch.py` (in `mote_bringup`) | Bench bring-up — the same controller_manager, URDF and `controllers.yaml` as a mission, without the lidar/camera/Nav2. `pixi run arm`. | | `jog` (CLI) | Interactive per-joint jog. A *client of the controller* — publishes clamped trajectories, never opens the bus. `pixi run arm-jog`. | | `arm_check` (tool) | Standalone enumeration + health + zero snapshot. Read-only, but opens the bus: run with the control stack stopped. `pixi run arm-check`. | @@ -175,6 +176,32 @@ conversions are verified without hardware. | `arm_offsets` (tool) | Read/back up/restore/set the servos' position-correction offsets. The recovery path if a calibration is interrupted. `pixi run arm-offsets`. | | `poses.py` / `arm_pose` | Teach and replay named poses, and narrow limits to a working envelope. `pixi run arm-pose save\|list\|go\|limits\|delete`. | +## Exits and arguments + +Two things every arm CLI needs, both of which fail *quietly* when hand-rolled, +so they live in `cli.py` and nowhere else. + +**Destroying a node that `spin()` still holds aborts the process.** The +executor is pulled out from under itself and the interpreter calls +`std::terminate`: exit 134, `terminate called without an active exception`, +after the tool has already done its work. The fix is ordering — shut the +context down, *join the spin thread*, and only then destroy — which is what +`cli.shutdown(node, spinner)` is for. Measured on this arm's CLIs with no +hardware attached: `jog` (stdin closed) and `arm-pose list` each aborted 3 of 3 +runs before, and exited 0 on 3 of 3 after. It is not a rare race — with no +stack running to talk to, it reproduced every time. `test_cli.py` watches a +child process's exit status, because nothing in-process can catch an abort. + +**ROS's arguments arrive mixed in with the tool's own.** `ros2 run` hands the +executable the whole command line, so a plain `parse_args` rejects +`--ros-args` outright (`arm-pose list --ros-args -p use_sim_time:=true` used to +die with "unrecognized arguments") while the usual workaround, +`parse_known_args`, silently discards anything it does not recognise — which on +a *safety* flag means a mistyped `--max-travel` or `--speed` does nothing and +says nothing, and the arm moves under the default instead. `cli.parse` cuts the +`--ros-args ... --` block out first and then parses what is left strictly, so +ROS arguments pass through and a typo is still an error. + ## `zero` and `home` are different things Worth stating plainly, because the two were both called "home" until 2026-07-28 diff --git a/mote_arm/mote_arm/arm_pose.py b/mote_arm/mote_arm/arm_pose.py index e20d1bd..f149978 100644 --- a/mote_arm/mote_arm/arm_pose.py +++ b/mote_arm/mote_arm/arm_pose.py @@ -34,11 +34,10 @@ import time import rclpy -from rclpy.executors import ExternalShutdownException from rclpy.node import Node from sensor_msgs.msg import JointState -from mote_arm import config, poses +from mote_arm import cli, config, poses from mote_arm.control import ArmControl @@ -277,7 +276,7 @@ def _stream(node: PoseClient, start: dict, goals: dict, args) -> None: print(f" holding at {err:.4f} rad of residual droop") -def main() -> None: +def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(description="Teach and replay arm poses") sub = parser.add_subparsers(dest="cmd", required=True) @@ -339,30 +338,21 @@ def main() -> None: ) p_go.add_argument("--timeout", type=float, default=5.0) p_go.set_defaults(func=_cmd_go) + return parser + - args = parser.parse_args() +def main() -> None: + args = cli.parse(build_parser()) rclpy.init() node = PoseClient() - - def _spin() -> None: - try: - rclpy.spin(node) - except (KeyboardInterrupt, ExternalShutdownException): - pass - except Exception: # noqa: BLE001 - context torn down by SIGINT - if rclpy.ok(): - raise - - threading.Thread(target=_spin, daemon=True).start() + spinner = cli.spin_background(node) try: args.func(node, args) except KeyboardInterrupt: print("\ninterrupted", file=sys.stderr) finally: - if rclpy.ok(): - rclpy.shutdown() - node.destroy_node() + cli.shutdown(node, spinner) if __name__ == "__main__": diff --git a/mote_arm/mote_arm/cli.py b/mote_arm/mote_arm/cli.py new file mode 100644 index 0000000..5b5fa79 --- /dev/null +++ b/mote_arm/mote_arm/cli.py @@ -0,0 +1,81 @@ +"""Command-line plumbing shared by the arm tools: arguments, and clean exits. + +``ros2 run`` hands the executable everything on the line, ROS's own arguments +included, so a node with its own flags sees both. The usual answer, +``parse_known_args``, silently discards whatever it does not recognise — which +means a mistyped flag on a *safety* option (``--max-travel``, ``--speed-scale``) +does nothing at all and says nothing about it. + +So instead: cut the ``--ros-args ... --`` block out first, then parse what is +left strictly. A flag we do not know is then an error, as it should be. +""" + +from __future__ import annotations + +import sys +import threading +from collections.abc import Sequence + +import rclpy +from rclpy.executors import ExternalShutdownException +from rclpy.node import Node + + +def user_args(argv: Sequence[str] | None = None) -> list[str]: + """Everything on the command line that is not a ROS argument. + + ``--ros-args`` opens the ROS block and a bare ``--`` closes it; that bare + separator is also what argparse would otherwise take as "no more options", + turning ``-- --camera`` into a positional it cannot place. + """ + argv = list(sys.argv[1:] if argv is None else argv) + kept: list[str] = [] + in_ros_block = False + for arg in argv: + if arg == "--ros-args": + in_ros_block = True + elif arg == "--": + in_ros_block = False + elif not in_ros_block: + kept.append(arg) + return kept + + +def parse(parser, argv: Sequence[str] | None = None): + """Parse this tool's own arguments, strictly, ignoring ROS's.""" + return parser.parse_args(user_args(argv)) + + +def spin_background(node: Node) -> threading.Thread: + """Spin a node on a worker thread while the main thread runs a CLI.""" + + def _spin() -> None: + try: + rclpy.spin(node) + except (KeyboardInterrupt, ExternalShutdownException): + pass + except Exception: # noqa: BLE001 + # SIGINT tears the rcl context down underneath spin(), which + # surfaces as different exception types depending on how the node + # was launched. "The context is gone" is the ordinary stop it is; + # anything that failed while it was still up is a real error. + if rclpy.ok(): + raise + + thread = threading.Thread(target=_spin, daemon=True) + thread.start() + return thread + + +def shutdown(node: Node, spinner: threading.Thread) -> None: + """Stop spinning, then destroy the node — in that order. + + Destroying a node that ``spin()`` still holds pulls it out from under the + executor mid-callback, and the process aborts with "terminate called + without an active exception" instead of exiting. Shutting the context down + first makes spin return; joining it is what guarantees it has. + """ + if rclpy.ok(): + rclpy.shutdown() + spinner.join(timeout=2.0) + node.destroy_node() diff --git a/mote_arm/mote_arm/jog.py b/mote_arm/mote_arm/jog.py index c3b8ff6..27abd9d 100644 --- a/mote_arm/mote_arm/jog.py +++ b/mote_arm/mote_arm/jog.py @@ -22,11 +22,10 @@ import time import rclpy -from rclpy.executors import ExternalShutdownException from rclpy.node import Node from sensor_msgs.msg import JointState -from mote_arm import config +from mote_arm import cli, config from mote_arm.config import JointSpec from mote_arm.control import ArmControl @@ -192,22 +191,7 @@ def _repl(node: JogClient) -> None: def main() -> None: rclpy.init() node = JogClient() - - def _spin() -> None: - # SIGINT surfaces here as ExternalShutdownException; the REPL thread - # owns the exit path, so this one just stops quietly. - try: - rclpy.spin(node) - except (KeyboardInterrupt, ExternalShutdownException): - pass - except Exception: # noqa: BLE001 - # Context torn down by SIGINT; a real error is one that happened - # while the context was still valid. - if rclpy.ok(): - raise - - spin = threading.Thread(target=_spin, daemon=True) - spin.start() + spinner = cli.spin_background(node) try: _repl(node) except KeyboardInterrupt: @@ -215,8 +199,7 @@ def _spin() -> None: finally: print("\nlimping arm (deactivating arm_controller) and exiting...") node.arm.set_holding(False) - rclpy.shutdown() - node.destroy_node() + cli.shutdown(node, spinner) if __name__ == "__main__": diff --git a/mote_arm/test/test_cli.py b/mote_arm/test/test_cli.py new file mode 100644 index 0000000..bf0543f --- /dev/null +++ b/mote_arm/test/test_cli.py @@ -0,0 +1,92 @@ +"""The shared CLI plumbing: strict argument parsing, and exits that are exits. + +Both properties fail *silently* if they regress, which is why they are pinned +here. A mistyped safety flag that argparse drops on the floor changes nothing +and says nothing; and a node destroyed while ``spin()`` still holds it aborts +the process after the useful work is done, so the operator sees a crash on a +run that in fact succeeded. +""" + +import os +import random +import subprocess +import sys +import textwrap + +import pytest + +from mote_arm import cli +from mote_arm.arm_pose import build_parser + + +def test_user_args_drops_the_ros_block(): + assert cli.user_args(["go", "home", "--ros-args", "-p", "x:=1"]) == ["go", "home"] + + +def test_bare_separator_closes_the_ros_block(): + """``--`` ends the ROS block; what follows is ours again. + + argparse would read that bare ``--`` as "no more options" and turn a flag + after it into a positional it cannot place, so it must not survive. + """ + argv = ["go", "home", "--ros-args", "-p", "x:=1", "--", "--yes"] + assert cli.user_args(argv) == ["go", "home", "--yes"] + + +def test_no_ros_block_is_left_alone(): + assert cli.user_args(["go", "home", "--yes"]) == ["go", "home", "--yes"] + + +def test_parse_accepts_a_safety_flag_past_a_ros_block(): + args = cli.parse( + build_parser(), + ["go", "home", "--max-travel", "1.25", "--ros-args", "-p", "x:=1"], + ) + assert args.name == "home" + assert args.max_travel == 1.25 + + +@pytest.mark.parametrize("flag", ["--max_travel", "--maxtravel", "--speeed"]) +def test_a_mistyped_safety_flag_is_an_error(flag): + """Not a warning, and above all not silence: the arm would move anyway.""" + with pytest.raises(SystemExit) as exc: + cli.parse(build_parser(), ["go", "home", flag, "0.9"]) + assert exc.value.code != 0 + + +# Destroying a node the executor still holds aborts the interpreter itself +# (SIGABRT, "terminate called without an active exception"), so no in-process +# assertion can catch it — the test has to watch a child's exit status. +CHILD = """\ +import threading +import rclpy +from rclpy.node import Node +from mote_arm import cli + +rclpy.init() +node = Node("cli_shutdown_probe") +node.create_timer(0.001, lambda: None) +spinner = cli.spin_background(node) +threading.Event().wait(0.4) +cli.shutdown(node, spinner) +print("clean") +""" + + +def test_shutdown_exits_cleanly(): + env = dict(os.environ) + env["ROS_DOMAIN_ID"] = str(random.randint(60, 100)) + env["PYTHONPATH"] = os.pathsep.join(sys.path) + done = subprocess.run( + [sys.executable, "-c", textwrap.dedent(CHILD)], + capture_output=True, + text=True, + timeout=60, + env=env, + ) + assert done.returncode == 0, ( + f"exit {done.returncode} (-6/134 is the abort this guards against)\n" + f"{done.stderr}" + ) + assert "clean" in done.stdout + assert "terminate called" not in done.stderr