Conversation
reachy_mini_ble_test: read-only pytest smoke tests for the BLE
provisioning service, run over the air from a Linux host with BlueZ (e.g.
a Raspberry Pi) against a robot in radio range:
pip install reachy-mini pytest # --no-deps also works
pytest --pyargs reachy_mini_ble_test -v
Discovers the robot on its own (bluetoothctl scan), parses protocol
constants from the bluetooth_service.py shipped in the same wheel (fails
loudly on drift), and covers GATT layout, device info, status
characteristics, PING/ECHO, journal streaming, the public WiFi
provisioning surface, and auth gates. No PIN is ever sent; nothing on the
robot is mutated.
Connects via a minimal raw LE ATT client (att_client.py): the robot's
dual-mode advert makes BlueZ's Device1.Connect pick the BR/EDR bearer and
fail; the raw LE socket behaves like the LE-only phone/Web Bluetooth
clients and needs no privileges. The package imports nothing from
reachy_mini itself, so pip install --no-deps works on bare hosts.
Outside testpaths; CI never collects it. Validated: 15/15 on a Raspberry
Pi from an installed wheel against a live robot.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
70183d0 to
a4b2eea
Compare
FabienDanieau
left a comment
FabienDanieau
left a comment
There was a problem hiding this comment.
Thanks for this, the raw-ATT approach is the right call and the read-only/no-PIN discipline is exactly what a smoke suite against a shared robot needs. I ran it from two hosts against a live robot before reviewing; results below. Requesting changes for three things the runs surfaced, plus a layout ask. Line-level points are inline.
1. Skips hide real drift (blocking)
Two consecutive runs from the same laptop, same code, same robot, minutes apart:
run 1: 15 passed
run 2: 11 passed, 4 skipped
SKIPPED robot firmware predates the Hardware ID characteristic
SKIPPED robot firmware predates WIFI_STATUS (command echoed back)
SKIPPED robot firmware predates WIFI_KEYEX (command echoed back)
SKIPPED robot firmware predates WIFI_SCAN (command echoed back)
The robot's BLE service had regressed to an older build underneath us (its daemon was down, so check_and_update_bluetooth_service couldn't resync). The suite lost a characteristic and the whole WiFi surface, including test_wifi_status_public_omits_known_networks, and still exited 0. For a suite whose purpose is catching protocol drift, _skip_if_unsupported inverts the signal. Please make it fail by default and gate the skip behind an explicit opt-in (REACHY_BLE_ALLOW_OLD_FIRMWARE=1).
Related: three tests can't fail as written (test_system_status_is_online, test_robot_is_discoverable, test_advertises_status_service_uuid), see inline. The last one matters most because it measures BlueZ's cache rather than the advertisement.
2. Clean Raspberry Pi: 15 errors (blocking until re-validated)
Pi 4, Debian trixie, BlueZ 5.82, Python 3.13, never paired or bonded with the robot (verified empty cache). Discovery works; the LE connect is refused:
connect rc=-1 errno=115 (EINPROGRESS) -> poll -> SO_ERROR=111 (Connection refused)
15 errors in 98s
Ruled out: competing central (other host's BT off), active discovery, adapter state (power-cycled). A bonded laptop connects to the same robot fine. The remaining systematic difference is the bond, but the robot was running the old service build at the time (see 1), so I can't separate "bonding is an undocumented prerequisite" from "old firmware required pairing". Could you re-run on a never-bonded host against a robot on this branch's bluetooth_service.py and paste the output? The "15/15 from a Pi" in the description reads as the earlier bleak / tests/bluetooth_tests / run_on_pi.sh version, none of which is in this diff, so I'm treating it as unverified for the current code.
3. att_client.connect() mishandles EINPROGRESS (blocking)
Inline at att_client.py:148 and ble_link.py:163. Short version: EINPROGRESS is treated as fatal, then the random-address retry reports its error instead of the real one. That is why the Pi failure surfaced as a bare BlockingIOError.
4. Layout: put it with the other hardware tests
Please move this under tests/hardware/ble/ following #1393 (tests/hardware/, per-test @pytest.mark.ble, -m ble to select, not ble added to the marker exclusions in pytest.yml / allure.yml), rather than shipping it as a top-level package in the wheel. Reasons:
- Every
pip install reachy-minicurrently getsreachy_mini_ble_test/(verified in the built wheel), andREADME.mdis not in it while__init__.pypoints users at it. - We want to run this from the physical CI runner and publish results. Being pytest already gets us
--alluredirfor free; what's missing is an artifact hook like #1393'sREACHY_TEST_ARTIFACTS=<dir>: dump what the robot reported (GATT layout, DIS values, network status, available commands, hardware id, per-command round-trip times) to a JSON so the report has numbers, not just pass/fail. - The "install on a bare Pi" story becomes
git clone+uv sync+uv run pytest tests/hardware/ble -m ble, same runbook as #1393.find_spec("reachy_mini")keeps working for locating the firmware source without importing it.
Prerequisite worth stating in the README: the CI runner needs a BT adapter in radio range of reachy-mini-ci.
Smaller
- The "why raw ATT" paragraph is in three places (README,
att_client.py,ble_link.py); keep it inatt_client.pyand link from the other two. - The PR description describes the previous implementation (
tests/bluetooth_tests,run_on_pi.sh, bleak, able-testdependency group) and the template is unfilled.
Separately
The reason this client has to exist is that the robot's advert lacks the BR/EDR-Not-Supported flag, so every BlueZ central (any Linux desktop) picks the classic bearer and fails. That's a robot-side interop bug affecting users, not just tests. Is there an issue for it? If it gets fixed, att_client.py collapses to a few lines of bleak, so a pointer to that issue in the module docstring would be good.
| raise OSError(ctypes.get_errno(), os.strerror(ctypes.get_errno())) | ||
| peer = _sockaddr_l2(_bdaddr(self.address), peer_type) | ||
| if libc.connect(sock.fileno(), peer, len(peer)) != 0: | ||
| raise OSError(ctypes.get_errno(), os.strerror(ctypes.get_errno())) |
There was a problem hiding this comment.
Any non-zero return is treated as fatal, but on a clean Pi 4 (BlueZ 5.82) connect() returns EINPROGRESS (115), which means "in progress", not failure. Standard handling: on EINPROGRESS, select() for writability, then read SO_ERROR and raise OSError(so_err, ...) if non-zero. As written, a perfectly valid connection is thrown away, and the caller's retry hides the real error (see ble_link.py:163).
| raise AttError(req_op, handle, code) | ||
| # anything else (e.g. server-initiated exchange): ignore | ||
|
|
||
| def wait_notification(self, timeout: float) -> bytes: |
There was a problem hiding this comment.
wait_notification ignores the source handle. Fine today because only the response characteristic notifies, but silently wrong the day network status gets notify. One-line check against self._response_char.value_handle (or take the expected handle as a parameter).
| Characteristic(decl_handle, int(props), int(value_handle), uuid) | ||
| ) | ||
| offset += entry_len | ||
| start = decl_handle + 1 |
There was a problem hiding this comment.
If a Read By Type response yields no parsable entry, decl_handle is unchanged and start advances by one handle, so this loops up to ~65k round trips before giving up. Break when the inner loop appended nothing.
| _READ_BLOB_RSP, | ||
| ) | ||
| except AttError as e: | ||
| if e.code == _ATT_ERR_ATTRIBUTE_NOT_LONG: |
There was a problem hiding this comment.
BlueZ answers a Read Blob at offset == length with Invalid Offset (0x07), not only Attribute Not Long (0x0B). A value whose length is an exact multiple of mtu - 1 raises here instead of returning. Accept both codes.
|
|
||
| @staticmethod | ||
| def _bluetoothctl(*args: str, timeout: float) -> str: | ||
| return subprocess.run( |
There was a problem hiding this comment.
Return code and stderr are ignored. A missing bluetoothctl surfaces as a raw FileNotFoundError, a scan overrun as TimeoutExpired, both out of a session fixture with no hint. Wrap them in BleLinkError with a message.
| # -- discovery / GATT layout ------------------------------------------- | ||
|
|
||
|
|
||
| def test_robot_is_discoverable(link: ReachyBleLink) -> None: |
There was a problem hiding this comment.
Cannot fail: the link fixture already pytest.fails when discover() returns None, and the name check is what discovery matched on. Either drop it or make it assert something discovery did not already guarantee (e.g. the robot was seen in this scan, once discovery parses scan output).
|
|
||
| def test_system_status_is_online(link: ReachyBleLink) -> None: | ||
| """System status characteristic reads as Online.""" | ||
| assert link.read_char(SYSTEM_STATUS_UUID) == "Online" |
There was a problem hiding this comment.
Vacuous: bluetooth_service.py:560 is StaticCharacteristic(..., "Online", ...), a hardcoded literal. It cannot read anything else while the service is alive, and the read path is already covered by test_device_information_service. Suggest dropping it.
| assert "known" not in status, "saved networks leaked without authentication" | ||
|
|
||
|
|
||
| def test_wifi_keyex_returns_fresh_x25519_key(link: ReachyBleLink) -> None: |
There was a problem hiding this comment.
Docstring says "fresh" but the test never calls WIFI_KEYEX twice or compares kid / pk. Either compare two calls or rename to ..._returns_valid_x25519_key.
|
|
||
| def test_cmd_scripts_require_auth(link: ReachyBleLink) -> None: | ||
| """CMD_* scripts are refused without a session (nothing is executed).""" | ||
| assert link.command("CMD_RESTART_DAEMON") == AUTH_ERROR |
There was a problem hiding this comment.
Only auth test without _skip_if_unsupported. Intentional? (Fine if so, CMD_* predates everything else; just checking it isn't an oversight, especially once the guard flips to fail-by-default.)
|
|
||
| This package deliberately depends on nothing but the standard library (plus | ||
| pytest to run), so a minimal ``--no-deps`` install is enough on a bare test | ||
| host like a Raspberry Pi. See README.md in this directory for scope, |
There was a problem hiding this comment.
README.md is not in the wheel (package-data only covers reachy_mini, MANIFEST.in only assets/templates/descriptions), so for anyone who pip installs this, the pointer dangles. Moot if the suite moves to tests/hardware/ as suggested in the review body; otherwise add it to [tool.setuptools.package-data].
2 participants
New tests/bluetooth_tests suite: read-only smoke tests for the BLE provisioning service, run from an external Linux host with a Bluetooth adapter (e.g. a Raspberry Pi) against a robot in radio range. The suite discovers the robot on its own (advertised name / status-service UUID), imports protocol constants from the firmware source so it fails loudly on protocol drift, and covers GATT layout, the Device Information service, status characteristics, PING/ECHO round-trips, the journal streaming lifecycle, the public WiFi provisioning surface (including the saved-networks-must-not-leak invariant), and the auth gates on privileged commands. No PIN is ever sent (the robot-global wrong-PIN lockout is never armed) and nothing on the robot is mutated.
Scanning goes through BlueZ (bleak), but the connection uses a minimal ATT client over a raw LE L2CAP socket (att_client.py): the robot's dual-mode advertisement makes BlueZ's Device1.Connect pick the classic BR/EDR bearer and fail with br-connection-profile-unavailable, while phones and Web Bluetooth are LE-only and never hit this. The raw socket behaves like them, works unprivileged, and tests the robot exactly as shipped.
Kept outside testpaths with a new 'ble' marker and a 'ble-test' dependency group (bleak); run_on_pi.sh rsyncs the repo to a Pi, bootstraps a minimal venv (handles hosts without python3-venv/pip), and runs the suite. Validated end-to-end: 15/15 passing from a Raspberry Pi against a live robot.
Issue
Closes #
Description
Testing
Tested on
--sim)--mockup-sim)AI assistance