Skip to content

fix(mcp): take the achieved position from the controller's result #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jlongvalRobotiq wants to merge 1 commit into
base: feat/mcp
Choose a base branch
from
+18 −0
Open

fix(mcp): take the achieved position from the controller's result #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions mcp/gripper_mcp/ros_messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ def advertised_type(names_and_types, action_name: str, known: dict) -> str | Non

def position_of(state, joint: str, fallback: float) -> float:
names = list(state.name)
if not names and len(state.position) == 1:
return state.position[0]
Comment on lines +108 to +109

@ebarnett3 ebarnett3 Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Low — the unnamed-position rule is the action result's contract, but lives in the joint_states lookup

mcp/gripper_mcp/ros_messages.py:108-109. position_of is also called on joint_states samples (ros_backend.py:208, :283). A JointState on that topic always names its joints, so the branch is dead there and reads as a generic heuristic. Nothing says why it exists. Either add a one-line why (# parallel_gripper_action_controller fills result.state.position but never result.state.name) or split it out so the rule sits on the result path only:

def result_position_of(state, joint: str, fallback: float) -> float:
    if not state.name:
        return state.position[0] if len(state.position) == 1 else fallback
    return position_of(state, joint, fallback)

and call it from _result_position (ros_backend.py:334); move the two new tests with it.

if joint not in names:
return fallback
return state.position[names.index(joint)]
2 changes: 2 additions & 0 deletions mcp/gripper_mcp/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ def classify(
return "refused"
if motion.timed_out:
return "incomplete"
if motion.reached_goal:

@ebarnett3 ebarnett3 Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Low — service.py module docstring now contradicts classify (service.py:15-21, outside the diff)

mcp/gripper_mcp/service.py:15-21. The paragraph still says the verdict is read "from where the fingers ended up against where they were sent" and that the controller's flags do "not steer the outcome". After this PR reached_goal steers first. Rewrite the paragraph to the new rule, e.g.: "The controller's reached_goal is the verdict when it is set: its goal tolerance is wider than the datasheet's closed tolerance, so the position comparison would otherwise call a finished move an object. When it is not set, the verdict is read from where the fingers ended up against where they were sent, never from the stalled flag (#29: on the real driver that flag trips on every goal), which is passed through to the caller as-is." Keep the gOBJ stopgap sentence. Since #92 fixes the "no velocity is ever computed" cause, prefer wording that does not assert it as current fact.

return "reached"
if stopped_on_something(commanded_mm, achieved_mm, stroke):

@ebarnett3 ebarnett3 Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3. Low — the object-detection floor is now the controller's goal_tolerance, not closed_tolerance_mm

mcp/gripper_mcp/service.py:222 with grippers/robotiq_description/config/robotiq_controllers*.yaml (goal_tolerance: 0.02 rad ≈ 2.1 mm). A stop within 2.1 mm of the target is reached; closed_tolerance_mm (1.5 mm) only matters when the controller did not reach. Not a regression (a 1 mm stop was already reached), and the PR body says so, but the datasheet yaml now documents a tolerance that is not the effective one. A sentence in the docstring or the datasheet comment is enough for this PR.

Don't align the two numbers: the position comparison itself is on its way out. Once the controller takes stall and reached from the gripper's object status (#96, follow-up to #92), reached_goal means "at requested position" and stalled means "object detected" straight from the firmware, and classify should trust both flags and drop closed_tolerance_mm and the #29 rule. That is the consumer-side half of that issue, not a change here.

return "stopped_on_object"
return "reached"
12 changes: 12 additions & 0 deletions mcp/tests/test_ros_messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,18 @@ def test_a_state_without_the_joint_yields_the_fallback():
assert position_of(state, "knuckle_joint", FALLBACK_RAD) == FALLBACK_RAD


def test_a_result_naming_no_joint_but_carrying_one_position_is_that_joint():
state = FakeState([], [KNUCKLE_RAD])

assert position_of(state, "knuckle_joint", FALLBACK_RAD) == KNUCKLE_RAD


def test_a_result_naming_no_joint_with_no_position_yields_the_fallback():
state = FakeState([], [])

assert position_of(state, "knuckle_joint", FALLBACK_RAD) == FALLBACK_RAD


class FakeCancelResponse:
def __init__(self, return_code):
self.return_code = return_code
Expand Down
2 changes: 2 additions & 0 deletions mcp/tests/test_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ def test_each_gripper_opens_to_its_own_model_width():
(motion(reached_goal=False, stalled=True), 85.0, 49.0, "stopped_on_object"),
(motion(reached_goal=True), 30.0, 30.0, "reached"),
(motion(reached_goal=False, stalled=True), 30.0, 30.0, "reached"),
(motion(reached_goal=True), 85.0, 83.1, "reached"),

@ebarnett3 ebarnett3 Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: mcp/tests/test_service.py:226 — the new row's id, "the controller reached within its own tolerance, wider than ours", is the clearest statement of the rule in the PR; the docstring fix in finding 1 could reuse it.

],
ids=[
"refused",
Expand All @@ -233,6 +234,7 @@ def test_each_gripper_opens_to_its_own_model_width():
"open blocked part-way",
"move landed",
"move landed but the driver says stalled (#29)",
"the controller reached within its own tolerance, wider than ours",
],
)
def test_classify_decides_from_position_not_the_stall_flag(

@ebarnett3 ebarnett3 Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (test_service.py:242): mcp/tests/test_service.py:242 — test_classify_decides_from_position_not_the_stall_flag now holds a row decided by reached_goal, not position. Rename to test_classify_trusts_reached_goal_then_position or move the new row to its own test.

Expand Down
Loading