feat(odrive_native): ODrive native (Fibre) protocol server + real-fibre interop + Python client - #721
Conversation
…erver Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…raming Add the real-tool interop gate for components/odrive_native, mirroring how components/rtps is gated against real FastDDS/ROS 2: the genuine reference fibre client (pure-python legacy fibre from odriverobotics/ODrive @ fw-v0.5.1) connects to a host build of the odrive_native device shim over a PTY serial loopback, downloads endpoint 0, enumerates the tree, and reads/writes endpoints. - detail/odrive_native_stream.hpp: UART stream framing (odrive_crc8, stream_frame, StreamDeframer) verified byte-for-byte against the fw-v0.5.1 reference. - interop/: device shim (PTY, detail/-only, plain c++), real fibre client driver, run_interop.sh + run.sh runner, README, .gitignore for the fetched client/venv. - test/odrive_native_stream_test.cpp + pc/tests/odrive_native_golden.cpp: golden wire-format tests (CRC8/CRC16 goldens, exact frame bytes, deframe/packet round-trips); pc/CMakeLists.txt gives odrive_native_* targets the include dir. - .github/workflows/odrive_native_interop.yml: PASS/FAIL-gated CI. Fix (found by this harness): the endpoint canary json_crc is calc_crc16(json, init=PROTOCOL_VERSION=1), NOT the 0x1337 packet-CRC init -- this matches the fw-v0.5.1 firmware (endpoints_template.j2) and the reference client (discovery.py). Without it the real client's endpoint reads are all rejected. Corrected the core, host test, and PROTOCOL.md accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A clean, dependency-light Python client (odrivetool-equivalent) for the ODrive legacy native (Fibre endpoint) protocol, re-implementing the documented wire format with no dependency on the odrive/fibre pip package. - crc.py: CRC8/CRC16 (non-reflected, MSB-first) + constants - protocol.py: packet build/parse + little-endian type codecs - transport.py: Transport interface + serial stream-framing backend (StreamDeframer/stream_frame), with a clean seam for a future USB backend - device.py: Channel (seq + request/response), object tree with ergonomic attribute access (dev.axis0.controller.input_pos), get/set path helpers, dump(), connect()/find() - ascii.py: optional minimal ASCII-protocol helper (r/w/p/v/f) - tests/test_odrive.py + run.sh: CRC golden self-test plus end-to-end interop against the C++ device shim over a PTY (enumerate, read, write-then-read) Verified end-to-end: RESULT PASS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new odrive_native component implementing the legacy ODrive “native/Fibre endpoint” binary protocol, plus host tests, documentation, a Python client, and CI interop gating against the real reference fibre client.
Changes:
- Introduces
espp::OdriveNativeand a host-buildabledetail::OdriveNativeCore(CRC/packet/JSON/dispatch) with UART stream framing helpers. - Adds host golden tests, a PTY-based interop device shim + scripts, and a CI workflow to run real-fibre loopback interop.
- Adds a dependency-light Python client (
espp_odrive) with end-to-end tests against the C++ shim, and documentation/example wiring.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pc/tests/odrive_native_golden.cpp | Adds host “golden” wire-format regression test (CRC, framing, round-trip). |
| pc/CMakeLists.txt | Ensures odrive_native pc tests can include component headers directly. |
| doc/en/motor_control/odrive_native_example.md | Includes the component example README in the docs. |
| doc/en/motor_control/odrive_native.rst | Adds Sphinx page documenting the new component. |
| doc/en/motor_control/index.rst | Adds odrive_native to the motor_control docs index. |
| doc/Doxyfile | Adds odrive_native headers/example to Doxygen inputs. |
| components/odrive_native/test/odrive_native_stream_test.cpp | Adds host-buildable tests for serial stream framing/deframing. |
| components/odrive_native/test/odrive_native_host_test.cpp | Adds host-buildable tests for the packet core (endpoint-0, rw, canary). |
| components/odrive_native/python/tests/test_odrive.py | Adds Python CRC + end-to-end interop test against the C++ shim. |
| components/odrive_native/python/run.sh | Adds a runner that sets up/uses a venv and runs Python tests. |
| components/odrive_native/python/espp_odrive/transport.py | Implements transport abstraction + serial stream framing backend. |
| components/odrive_native/python/espp_odrive/protocol.py | Implements packet codec + primitive type codecs. |
| components/odrive_native/python/espp_odrive/device.py | Implements channel, object tree, connect/find, typed attr access. |
| components/odrive_native/python/espp_odrive/crc.py | Implements CRC8/CRC16 with documented constants. |
| components/odrive_native/python/espp_odrive/ascii.py | Adds optional minimal ASCII-protocol helper. |
| components/odrive_native/python/espp_odrive/init.py | Exposes Python client public API and version. |
| components/odrive_native/python/README.md | Documents installing/using the Python client and its tests. |
| components/odrive_native/python/.gitignore | Ignores Python venv/caches for the client subtree. |
| components/odrive_native/interop/run_interop.sh | Adds full interop harness: build tests/shim, fetch fibre, run loopback. |
| components/odrive_native/interop/run.sh | Adds thin wrapper entrypoint for the interop harness. |
| components/odrive_native/interop/odrive_native_interop_device.cpp | Implements PTY-based “device shim” using the host-buildable headers. |
| components/odrive_native/interop/odrive_fibre_client.py | Adds real reference-fibre client driver for interop gating. |
| components/odrive_native/interop/README.md | Documents interop harness pieces and how to run in CI/local. |
| components/odrive_native/interop/.gitignore | Ignores fetched reference repo and venv for interop runs. |
| components/odrive_native/include/odrive_native.hpp | Adds the espp::OdriveNative component wrapper over the wire core. |
| components/odrive_native/include/detail/odrive_native_stream.hpp | Adds UART/serial stream framing + deframer utilities. |
| components/odrive_native/include/detail/odrive_native_core.hpp | Adds the transport-agnostic wire core (CRC, endpoints, JSON, dispatch). |
| components/odrive_native/idf_component.yml | Adds ESP-IDF component manager metadata for odrive_native. |
| components/odrive_native/example/sdkconfig.defaults.esp32s3 | Adds esp32s3-specific sdkconfig default for console selection. |
| components/odrive_native/example/sdkconfig.defaults | Adds common sdkconfig defaults for example. |
| components/odrive_native/example/main/odrive_native_example.cpp | Adds ESP-IDF example demonstrating basic packet flow. |
| components/odrive_native/example/main/CMakeLists.txt | Adds example main component build file. |
| components/odrive_native/example/README.md | Documents building/running the example. |
| components/odrive_native/example/CMakeLists.txt | Adds example project build configuration. |
| components/odrive_native/README.md | Documents the new component at repo level. |
| components/odrive_native/PROTOCOL.md | Documents the authoritative wire protocol and constants. |
| components/odrive_native/CMakeLists.txt | Registers the new idf component. |
| .github/workflows/upload_components.yml | Adds odrive_native to component upload list. |
| .github/workflows/odrive_native_interop.yml | Adds CI workflow to run the real fibre interop harness. |
| .github/workflows/build.yml | Adds odrive_native example to CI build matrix. |
Suppressed comments (2)
components/odrive_native/python/tests/test_odrive.py:1
proc.stdout.readline()is blocking, so thedeadlineloop isn’t actually enforcing a timeout (it can hang forever if the device never prints a newline). Also,stderr=PIPEis never consumed, which can deadlock if the child writes enough to fill the pipe buffer. Useselect/poll(orcommunicate(timeout=...)) to implement a real timeout, and either mergestderrintostdoutor drainstderrin parallel.
components/odrive_native/python/run.sh:1- The script doesn’t use
set -e, so failures during venv creation or pip install can be accidentally ignored and the script may continue in a partially initialized state. Consider usingset -euo pipefail(or explicitly checking return codes) so setup failures fail fast with a clear error.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
components/odrive_native/python/tests/test_odrive.py:1
- Spawning the shim with
stderr=PIPEbut not continuously draining it can deadlock if the device emits enough stderr output to fill the pipe buffer. Consider redirecting stderr to stdout, toDEVNULL, or consuming stderr in a background thread while waiting for the PTY line.
components/odrive_native/include/detail/odrive_native_stream.hpp:76 - This function truncates
packet.size()into auint8_tlenwithout enforcing the statedlen < 128constraint. If a caller accidentally passes a larger packet, the frame will be malformed (length wrap) and CRCs won’t match expectations. Add an explicit guard forpacket.size() <= kStreamMaxPacket(orpacket.size() < 128) and fail fast (e.g., return empty, assert, or otherwise signal an error consistently with the component’s error-handling approach).
inline std::vector<uint8_t> stream_frame(std::span<const uint8_t> packet) {
std::vector<uint8_t> out;
const uint8_t len = static_cast<uint8_t>(packet.size());
out.reserve(packet.size() + 5);
out.push_back(kStreamSync);
out.push_back(len);
const uint8_t header[2] = {kStreamSync, len};
out.push_back(odrive_crc8(std::span<const uint8_t>(header, 2)));
out.insert(out.end(), packet.begin(), packet.end());
const uint16_t c = odrive_crc16(packet);
out.push_back(static_cast<uint8_t>((c >> 8) & 0xff)); // big-endian: high byte
out.push_back(static_cast<uint8_t>(c & 0xff)); // low byte
return out;
}
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ints - stream test: use vector::insert instead of a raw copy loop (fixes cppcheck constVariableReference + useStlAlgorithm, the 2 remaining static-analysis findings). - register_typed/register_bool_property: reject endpoints registered with neither getter nor setter, which would misrepresent access as "r" in the schema (Copilot review). No change to json_crc for real endpoints; host golden tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…orrect README - odrive_native: STL insert in stream test + reject accessor-less endpoints (mirrors #721; clears the 6 cppcheck findings on this branch). - example README: describe the actual wiring — CDC->OdriveAscii, vendor->OdriveNative (Fibre), plus a HID gamepad — instead of the stale 'both interfaces -> OdriveAscii' text (Copilot review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e bit PROTOCOL.md: unknown endpoints must be ignored (empty response). The dispatcher returned an ACK-only response when the expect-response bit was set on an unknown endpoint id; now it returns nothing, while known write-only endpoints still get their empty-data ACK. Adds a regression test (Copilot review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e bit Mirror #721: unknown endpoints return no response (per PROTOCOL.md) rather than an ACK-only; adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (3)
components/odrive_native/idf_component.yml:5
- Using the
git://scheme is insecure (no TLS) and is frequently blocked in CI/corporate networks. Prefer an HTTPS URL (e.g.https://github.com/esp-cpp/espp.git) to avoid MITM risk and improve reliability.
repository: "git://github.com/esp-cpp/espp.git"
components/odrive_native/python/espp_odrive/transport.py:80
- The deframer frequently deletes from the front of a
bytearray(del self._buf[0],del self._buf[:sync], etc.), which is O(n) per operation and can become quadratic under noisy input/resync conditions. Consider using a cursor (read index) with occasional compaction (similar to the C++StreamDeframer), or store the buffer in a deque/ring-buffer style structure.
sync = self._buf.find(SYNC_BYTE)
if sync < 0:
self._buf.clear()
break
if sync > 0:
del self._buf[:sync]
if len(self._buf) < 3:
break
length = self._buf[1]
if length >= MAX_PACKET_SIZE or crc8(bytes(self._buf[:3])) != 0:
# Bad header: drop the sync byte and hunt for the next one.
del self._buf[0]
continue
components/odrive_native/python/espp_odrive/transport.py:80
- The deframer frequently deletes from the front of a
bytearray(del self._buf[0],del self._buf[:sync], etc.), which is O(n) per operation and can become quadratic under noisy input/resync conditions. Consider using a cursor (read index) with occasional compaction (similar to the C++StreamDeframer), or store the buffer in a deque/ring-buffer style structure.
del self._buf[0]
continue
packets.append(packet)
del self._buf[:frame_len]
…onent) The #725 branch carried a stale odrive_native snapshot that predated several #721 review fixes: missing <algorithm>/<bit>/<type_traits> includes, the std::endian little-endian check, the ep_size response cap, and the O(n) deframer read-cursor rewrite (it still had the quadratic front-erase loop). Bring the whole component to #721's committed version so the two PRs ship identical source; also adds the espp_odrive Python client. Host tests + esp32s3 usb example build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-review: the wire protocol has no error channel and the ESP-free core has no logger, so a failed/rejected write or a canary-mismatched (desynced client) request was invisible to the device application. Add an optional error callback to the core (plain std::function, keeps the core host-buildable); espp::OdriveNative wires it to logger_.warn in its ctor. Also cap endpoint ids below 0x8000 (they'd collide with the expect-response bit and be unreachable) with a report through the same hook. No wire-format change; host golden tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… cppcheck style
Interop root cause: the reference fibre client enumerates candidate ports
with a plain os.listdir('/dev') (top-level entries only) + pyserial
comports(). macOS PTY slaves are top-level (/dev/ttysNNN) and are found;
Linux slaves are nested (/dev/pts/N) and can NEVER be discovered, so the
CI gate timed out on every Linux run while passing locally on macOS
(reproduced + fixed + verified 7/7 in an ubuntu:24.04 container). run_interop.sh
now aliases a /dev/pts/* slave to a top-level /dev symlink for the UNMODIFIED
reference client -- a workaround for its port-scan quirk only, nothing on the
wire changes. macOS behavior unchanged (re-verified 7/7).
Also clear the 5 style findings CI's cppcheck reports (newer than local):
CRC fold loops -> std::accumulate, endpoint/child lookups -> std::find_if,
and an inline suppression for a knownConditionTrueFalse false positive in
the deframer (the resync loop can exit 1-2 bytes before the buffer end).
Golden + host tests pass; wire format unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…yle) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Can you please expose the odrive native component to the x-plat lib so that it can be directly used within the pc tests / python code. this will make it more useful, clean up the test config code, and allow easier interop with tooling i think |
…st var detail/ lives INSIDE include/ (include/detail/*.hpp) — unlike the vendored root-level detail/ dirs elsewhere in espp — so registering "include" alone resolves the public header's #include "detail/...". Document that in CMakeLists; also drop the unused 'tree' local flagged by code quality. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… var) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done — odrive_native is now exposed through the x-platform library:
|
…thon) Per PR review: odrive_native is host-buildable, so make it a first-class part of the x-plat lib instead of a pc-harness special case. - lib: add odrive_native/include to ESPP_INCLUDES (header-only) and odrive_native.hpp to the espp.hpp umbrella; the exported espp::espp target now carries the headers. - python: hand-written pybind11 bindings (like rtps — the registration API uses std::function accessors with std::error_code& and std::span wire APIs that litgen cannot bind): espp.OdriveNative with pythonic register_*_property(path, getter, setter=None) for all 10 types, process_bytes(bytes)->bytes, json()/json_crc()/finalize(), plus module-level odrive_crc16/odrive_crc8/odrive_stream_frame and an OdriveStreamDeframer class. - pc: drop the odrive_native include-dir special case from the test harness (the lib provides it); extend the golden test with a lib_wrapper_roundtrip section exercising espp::OdriveNative through the installed package. - python/odrive_native_test.py: in-process loopback between the bound C++ server and the pure-python espp_odrive client codec (CRC goldens cross-checked between the two, endpoint-0 chunked JSON download, typed write-then-read, rejected-setter/bad-canary/unknown-endpoint semantics, UART stream framing round-trip). 21/21 checks pass locally; pc golden (incl. the new wrapper section) and the component host tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (4)
lib/python_bindings/odrive_native_bindings.cpp:1
- This binding checks
getter.is_none(), but the parameter type ispy::function, so passingNonefrom Python will fail the argument conversion before the lambda runs. If write-only endpoints are intended to be supported from Python, consider changing thegetterparameter type topy::object(withpy::arg("getter") = py::none()) and casting topy::functiononly when non-None; otherwise, remove the None-handling to avoid a misleading API.
lib/python_bindings/odrive_native_bindings.cpp:1 - For an empty vector,
v.data()may benullptr(implementation-dependent). Passing a null pointer topy::bytes(ptr, 0)can be risky depending on the underlying implementation. Consider explicitly handlingv.empty()by returningpy::bytes()to make this robust.
components/odrive_native/python/run.sh:1 - This script doesn’t use
set -e, so failures in venv creation or pip install can silently fall through and then run tests with a Python that still lackspyserial. Consider switching toset -euo pipefailor explicitly checking the venv/pip commands and exiting non-zero on failure so CI/local runs fail fast and predictably.
components/odrive_native/python/espp_odrive/device.py:64 - The size check is applied to
payload, but for the serial backend the entire packet must fit the stream framing limit (len(packet) < 128). As written, a payload that passes this check can still produce a packet that makestransport.stream_frame()raiseValueErrorat send time. Consider checkinglen(packet)(or enforcinglen(payload) <= 127 - 8for the current packet layout) before callingsend_packet.
if len(payload) >= 128:
raise OdriveError("payload larger than 127 bytes is not supported")
self._seq = (self._seq + 1) & 0x7FFF
# One bit is hardwired to 1 to avoid clashing with the ASCII protocol,
# mirroring the reference fibre client.
seq = self._seq | 0x80
packet = build_packet(seq, endpoint_id, output_len, payload,
expect_response, self.json_crc)
self._transport.send_packet(packet)
… espp package The odrivetool-equivalent client in components/odrive_native/python was only reachable via sys.path tricks. Ship it as a first-class part of the python distribution while keeping the component as the single source of truth: - pyproject: add components/odrive_native/python/espp_odrive to wheel.packages (top-level espp_odrive package in the wheel); anchor the sdist 'python/' exclude to the repo root so the component's python/ stays in the sdist; add the espp[serial] extra (pyserial is imported lazily by the serial transport only). - lib/espp.cmake: espp_install_python_module() stages espp_odrive next to the espp package in the standalone install prefix too, mirroring the wheel. - espp/__init__.py: expose it as espp.odrive (optional import) — so espp.OdriveNative is the bound C++ server and espp.odrive is the client. - python/odrive_native_test.py prefers the installed package (source-tree fallback kept); component README documents the packaging. Verified: PYTHONPATH=install gives import espp_odrive + espp.odrive, CRC goldens match between the binding and the client, and the loopback test passes 21/21 through the installed prefix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-up: the pure-python
Verified end-to-end through the installed prefix: |
…il.find_spec Clears the code-quality findings on the try-import availability probe (mixed import styles + unused import) and states the intent directly. Verified both paths: installed prefix (21/21) and source-tree fallback (21/21, with espp_odrive deliberately absent from the path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.
Suppressed comments (4)
lib/python_bindings/odrive_native_bindings.cpp:1
- The binding currently forces an extra copy on every call by converting
py::bytes -> std::string -> std::vector<uint8_t>, which can become a hot-path ifprocess_bytes()is called frequently. Consider accepting apy::buffer/py::bytesand using the buffer protocol (orPYBIND11_BYTES_AS_STRING_AND_SIZE) to create astd::span<const uint8_t>directly, avoiding the intermediate allocations/copies.
components/odrive_native/python/espp_odrive/device.py:118 RemoteProperty.get_value()does not enforce read access. This allowsDevice.get("...")/_resolve(...).get_value()to attempt reads of write-only properties (even though attribute access viaRemoteObject.__getattr__correctly raisesAttributeErrorfor write-only). Consider adding an explicitif not self.can_read: raise OdriveError(...)guard inget_value()(or inDevice.get()), so all read paths consistently respect the descriptor’s access flags.
def get_value(self):
if self._codec is None:
raise OdriveError("property %s has unsupported type %r" % (self._name, self._type))
data = self._channel.endpoint_operation(
self._id, b"", expect_response=True, output_len=self._codec.size)
return self._codec.decode(data)
components/odrive_native/include/detail/odrive_native_core.hpp:402
- The setter contract states “set
econ error, return true on ok”, but the core currently ignoresecentirely. This makes it easy for endpoint implementations to “signal error” viaecwithout actually failing the write, and the error detail can never reach the error callback. A concrete improvement is to treatecas authoritative for success (e.g., return false ifecis set), and/or change the internalEndpoint::deserializesignature to propagatestd::error_codeback toprocess_bytes()so the error callback can log the actual failure reason.
ep.deserialize = [setter](std::span<const uint8_t> s) -> bool {
T val{};
if (!read_le<T>(s, val))
return false;
std::error_code ec;
return setter(val, ec);
};
components/odrive_native/python/README.md:23
- The Python client README states “Python 3.8+”, but the repository/package metadata in
pyproject.tomlsetsrequires-python = ">=3.10". Please align this documentation with the actual supported Python version (either update the README to 3.10+ or adjust packaging constraints if 3.8/3.9 are truly supported).
- Python 3.8+
Summary
New
odrive_nativecomponent:espp::OdriveNative, a transport-agnostic server for the ODrive legacy native (Fibre endpoint) binary protocol — the protocolodrivetool/fibrespeak (packet-based; over the USB vendor interface). Sameprocess_bytes(span) -> bytesshape asodrive_ascii.register_float/int/uint/bool), sequential ids, auto-generated ODrive-schema JSON (endpoint 0) +json_crc, packet dispatch (canary check, chunked JSON read, typed LE codecs).PROTOCOL.md(from ODrive fw-v0.5.1). CRC16 (poly 0x3d65) verified against the reference.detail/odrive_native_stream.hpp).espp_odrivePython client (python/) — a clean, dependency-light re-implementation (stdlib + pyserial, noodrive/fibrepip dep): typed attribute access (dev.axis0.controller.input_pos = 3.14),dev.dump().Verification
interop/run.sh+.github/workflows/odrive_native_interop.yml, mirroring the rtps interop): the genuine referencefibreclient (ODrive fw-v0.5.1) connects to a host device shim over a PTY loopback, enumerates the tree, reads/writes — 7/7 PASS. This caught + fixed a real wire bug (the endpointjson_crccanary is seeded withPROTOCOL_VERSION, not the packet-CRC init).🤖 Generated with Claude Code