Skip to content

fix(drivers/booster): read_mode passes GetMode its out-parameter - #3400

Open
shipitfast wants to merge 2 commits into
strands-labs:mainfrom
shipitfast:fix/booster-get-mode-out-param
Open

shipitfast wants to merge 2 commits into
strands-labs:mainfrom
shipitfast:fix/booster-get-mode-out-param

Conversation

@shipitfast

Copy link
Copy Markdown
Contributor

What

BoosterDriver.read_mode now calls the vendor binding the way it is declared, GetMode(get_mode_response) -> int, and reads the mode from the filled GetModeResponse when the code is 0.

Why

booster_robotics_sdk_python binds B1LocoClient::GetMode(GetModeResponse&): one required out-parameter, an integer status back. The driver called self._client.GetMode() with no argument, so on a real T1 every read_mode raised and get_status died with it. With a vendor-faithful double, main fails at:

TypeError: _FakeLocoClient.GetMode() missing 1 required positional argument: 'get_mode_response'
strands_robots/drivers/booster.py:1019: TypeError

Tests

test_read_mode_passes_the_vendor_out_parameter_and_reads_it_back, test_a_nonzero_get_mode_code_is_no_reading; ruff, mypy clean on touched files.

The vendor binding is GetMode(get_mode_response) -> int, so calling it
with no argument raised TypeError on every real T1 and get_status died.
read_mode now hands it a GetModeResponse and reads the mode back from it.
Names the GetMode out-parameter fix in the reader's voice.

@cagataycali cagataycali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for looking at this seam. I measured the premise against the published binding before reading the diff, and the two disagree in a way that reverses the fix.

GetMode(GetModeResponse&) -> int is the C++ signature. The Python binding does not expose it that way: it wraps the out-parameter form as a no-argument query that returns the response. Read from the wheels on PyPI (the package the driver imports, pip index versions booster_robotics_sdk_python lists 1.6.1 as latest):

version evidence GetMode as bound
1.6.1 B1LocoClient.GetMode.__doc__ GetMode(self: B1LocoClient) -> GetModeResponse
1.5.6 symbol in the extension .so b1py_detail::QueryNoArg<&B1LocoClient::GetMode(GetModeResponse&)>::call(B1LocoClient&)
both vendor's bundled python_example/sdk_pybind_b1_example.py:702 gm = client.GetMode() / gm.mode

So GetModeResponse is the return value and the call takes no argument; the vendor's own examples call it exactly as main does at booster.py:1018. The QueryNoArg symbol is the deliberate adapter for the out-parameter convention, so the C++ header is the wrong reference for the Python surface.

What this diff would do on a real T1: self._client.GetMode(response) raises TypeError (pybind11 refuses the extra positional), which the except (RuntimeError, OSError) does not catch, so read_mode raises and get_status dies with it - the failure the description attributes to main, produced by the change instead.

The tests pass because _FakeLocoClient.GetMode was rewritten to the C++ shape in the same diff, so the driver is graded against a double that was changed to agree with it. The fixture on main (zero-arg, returns an object with .mode) matches the binding; if you have a real T1 raising TypeError at that line, the thing worth capturing is type(self._client).GetMode.__doc__ and the installed version, because that would be a binding not on PyPI and the driver would need to admit both shapes rather than swap one for the other.

As it stands I do not think this should land. Happy to re-read against a measured signature if one differs from the above.

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summary

This PR changes BoosterDriver.read_mode to call B1LocoClient.GetMode(response) with a GetModeResponse out-parameter and to treat the integer return as a status code, updating the test double to match. I am abstaining from a verdict on this head: the existing CHANGES_REQUESTED review on this same commit (7f17ea11) disputes the PR's premise about the Python binding's GetMode signature, and no new commits have landed since. One data point that is new relative to that review: the concern also holds on booster_robotics_sdk_python 1.6.3 (released after 1.6.1, the version it measured) — B1LocoClient.GetMode.__doc__ on the 1.6.3 wheel still reads GetMode(self) -> GetModeResponse. I have no findings beyond what that review already covers; deferring to the open thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drivers Native hardware drivers P1 Next

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants