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