feat(basicmicro): MCP/RoboClaw packet-serial motor controller component - #729
Conversation
Add espp::Basicmicro, a driver for Basicmicro MCP236 / MCP266 (and other
RoboClaw-family) brushed DC motor controllers speaking their PACKET SERIAL
protocol, typically over UART.
Wire core (include/detail/basicmicro_core.hpp, zero ESP dependencies,
host-buildable with only the C++20 std library):
- CRC16 replicating the manual's reference implementation byte-for-byte
(CRC-16/XMODEM: poly 0x1021, init 0, non-reflected, MSB-first)
- Packet building: write packets [addr, cmd, payload, CRC16 big-endian],
read requests [addr, cmd] (no CRC per the manual)
- Reply validation with the CRC seeded over the sent address + command
bytes plus the reply data (manual section 2.2.7)
- Big-endian ("high byte first") u8/u16/u32/i16/i32 codec helpers
- Command enum with every value verified against the MCP Series User
Manual (sections 2.2.12, 2.3.1, 2.4.8, 2.4.9) and a Status bit-mask
enum for command 90
Component (include/basicmicro.hpp, espp::Basicmicro : BaseComponent):
- Transport-agnostic via injected write/read std::functions; each
transaction (request + ACK/reply) runs under an internal mutex so
concurrent callers serialize; no exceptions (std::error_code)
- Duty (32/33/34), speed (35/36/37), speed+accel (38/39/40), buffered
speed/accel/distance (41-46) drive commands and buffer readback (47)
- Encoders: counts (16/17/78), speeds (18/19/79), reset (20), modes (91)
- Velocity PID set/read (28/29, 55/56) with 16.16 fixed-point conversion
- Telemetry: firmware version (21), battery voltages (24/25), currents
(49), PWMs (48), temperatures (82/83), status (90)
- Management: write settings to EEPROM (94, sent without CRC per the
manual), E-Stop reset (200)
- The >=10 ms receive timeout doubles as the documented packet-buffer
recovery mechanism (manual section 2.2.4)
Also:
- Host test with golden CRC vectors (incl. the 0x31C3 "123456789" check
value and full real packets), codec round-trips, packet build and
reply accept/reject cases
- ESP32 UART example: firmware version, battery voltage, status, then a
gentle duty ramp on M1 with encoder readback, then stop + telemetry
- Docs (Doxyfile, motor_control rst + toctree) and CI registration
(build matrix, component upload list)
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 espp::Basicmicro component implementing Basicmicro MCP/RoboClaw packet-serial protocol support, plus docs, example, CI integration, and host-side wire-core tests.
Changes:
- Introduces a host-buildable wire core (
basicmicro_core.hpp) and the mainespp::Basicmicrodriver API (basicmicro.hpp). - Adds an ESP-IDF UART example project and integrates it into build/docs/doxygen/component upload workflows.
- Adds host-side unit tests validating CRC, codecs, packet building, and reply validation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/en/motor_control/index.rst | Adds Basicmicro docs page to the motor control doc index. |
| doc/en/motor_control/basicmicro_example.md | Includes the component example README into docs. |
| doc/en/motor_control/basicmicro.rst | Introduces Basicmicro component documentation + toctree to the example page. |
| doc/Doxyfile | Registers Basicmicro headers and example source for Doxygen generation. |
| components/basicmicro/test/basicmicro_host_test.cpp | Adds host-buildable golden tests for CRC/codecs/packet validation. |
| components/basicmicro/include/detail/basicmicro_core.hpp | Adds protocol wire-core helpers (CRC/codec/packet build/validate) and enums. |
| components/basicmicro/include/basicmicro.hpp | Adds the transport-agnostic Basicmicro driver API built on the wire core. |
| components/basicmicro/idf_component.yml | Adds component-manager manifest for publishing. |
| components/basicmicro/example/sdkconfig.defaults | Adds example sdkconfig defaults for task stack sizing. |
| components/basicmicro/example/main/basicmicro_example.cpp | Adds ESP-IDF UART example demonstrating common commands and telemetry. |
| components/basicmicro/example/main/CMakeLists.txt | Registers the example’s main component. |
| components/basicmicro/example/README.md | Documents wiring/build/flash steps for the example. |
| components/basicmicro/example/CMakeLists.txt | Adds standalone example project configuration. |
| components/basicmicro/README.md | Adds component-level README with features/protocol/testing notes. |
| components/basicmicro/CMakeLists.txt | Registers the component with IDF build system. |
| .github/workflows/upload_components.yml | Adds basicmicro to the component upload workflow list. |
| .github/workflows/build.yml | Adds the Basicmicro example to the CI build matrix. |
💡 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! ✅ |
- example: hoist the UART constants to file scope so the captureless transport lambdas reference them without capture-semantics ambiguity (the previous function-local static constexpr form was well-formed, but file scope removes all doubt). - core: include <cstddef> explicitly (size_t in public inline helpers). - ctor: enforce the documented contracts — timeout below the protocol's 10 ms packet-clear window is clamped up (warn), and an address outside 0x80-0x87 is clamped into range (warn). - manifest: https:// repository URL (github disabled the git:// protocol). Host tests ALL PASSED; esp32 example builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
components/basicmicro/include/basicmicro.hpp:803
read_exact()breaks out of the loop on the first 0-byte read. If the transport returns 0 for a short per-call timeout while more bytes may arrive before the overalldeadline, this will prematurely fail reads for multi-byte replies that arrive in fragments. Instead of breaking onn == 0, continue looping untildeadlineis reached (optionally with a small backoff/yield if needed), and only time out once the overall deadline elapses.
bool read_exact(std::span<uint8_t> buf, std::error_code &ec) {
const auto deadline = std::chrono::steady_clock::now() + config_.timeout;
size_t got = 0;
while (got < buf.size()) {
const auto now = std::chrono::steady_clock::now();
if (now >= deadline)
break;
const auto remaining = std::chrono::duration_cast<std::chrono::milliseconds>(deadline - now);
const size_t n =
config_.read(buf.subspan(got), std::max(remaining, std::chrono::milliseconds(1)));
if (n == 0)
break; // the read function timed out
got += n;
}
if (got < buf.size()) {
logger_.debug("timed out reading reply ({}/{} bytes)", got, buf.size());
ec = std::make_error_code(std::errc::timed_out);
return false;
}
ec.clear();
return true;
}
…eview, CI) - Every transaction entry point now validates that Config::write AND Config::read were set, failing with invalid_argument instead of letting an empty std::function throw std::bad_function_call (which would violate the component's no-exceptions contract). - Zero-initialize all raw reply buffers passed to read_exact/read_command (clears CI cppcheck's uninitvar warnings; they are output buffers the transport fills, but cppcheck cannot see through the std::function). Host tests ALL PASSED; esp32 example builds; cppcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
components/basicmicro/example/main/basicmicro_example.cpp:88
- After any transient failure during the upward ramp, the next loop restarts at
max_duty; if communication recovers, the motor jumps directly to 12.5% instead of ramping down from the last successfully commanded duty. Track that duty and use it as the downward loop's starting point.
for (int16_t duty = 0; duty <= max_duty; duty = static_cast<int16_t>(duty + step)) {
if (!mcp.drive_m1_duty(duty, ec)) {
logger.error("drive_m1_duty({}) failed: {}", duty, ec.message());
break;
components/basicmicro/example/main/basicmicro_example.cpp:109
- A failed final stop is silently ignored, after which the example enters its infinite telemetry loop while the motor may remain at its last commanded duty. Treat stop failure as a safety-critical error: log it and retry/abort normal execution rather than continuing as though the ramp completed.
// make sure the motor is stopped
if (mcp.drive_m1_duty(0, ec))
logger.info("Motor stopped");
components/basicmicro/include/basicmicro.hpp:891
- Commands 18/19 return a 32-bit speed magnitude plus a separate direction byte; the official Basicmicro API likewise exposes this field as
uint32_t. Decoding it as signed makes magnitudes with bit 31 set negative even though direction is already reported separately. Change this helper and both public output parameters touint32_t(and update the example variable).
bool read_speed(Command cmd, int32_t &qpps, uint8_t &direction, std::error_code &ec) {
uint8_t data[5] = {};
if (!read_command(cmd, data, ec))
return false;
qpps = detail::read_i32_be(data, 0);
components/basicmicro/example/main/basicmicro_example.cpp:49
pdMS_TO_TICKStruncates. With a 100 Hz tick rate, a remaining timeout such as 19 ms becomes one 10 ms tick; if no byte arrives in that shorter interval,read_exacttreats the callback's zero return as the final timeout even though its deadline has not elapsed. Round the duration up to the next tick so this transport honors the requested timeout.
This issue also appears in the following locations of the same file:
- line 85
- line 107
const int read =
uart_read_bytes(uart_port, data.data(), data.size(), pdMS_TO_TICKS(timeout.count()));
return read < 0 ? 0 : static_cast<size_t>(read);
…t retry (PR #729 review) The manual leaves the status width unstated ('[Status, CRC]'), but current MCP firmware returns a 32-bit word — the official Basicmicro Arduino library reads it with Read4. read_status() now reads 4 bytes first; if that transaction fails (older firmware replying 16-bit ends the reply mid-read), the receive timeout has already spanned the controller's 10 ms packet-clear window, so a single 16-bit retry is performed for legacy units. Status out-param and mask enum widened to uint32_t (the manual's documented masks occupy the low 16 bits); example updated. Host tests ALL PASSED; esp32 example builds; cppcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Description
New
espp::Basicmicrocomponent for Basicmicro MCP236/MCP266 (and the wider RoboClaw family) brushed-DC motor controllers, speaking their packet serial protocol over UART — phase 1 of MCP support (phase 2: CANopen/DS402 overespp::Twaiis in a parallel PR).detail/basicmicro_core.hpp, zero ESP deps): CRC-16/XMODEM exactly per the manual's reference algorithm, big-endian codecs, packet build + reply validation (reply CRC seeded with sent address+command per §2.2.7; read requests carry no CRC, matching the wire spec).espp::Basicmicro(BaseComponent,std::error_code,std::functionwrite/read transport, serialized transactions): duty/speed/accel drives (32–40), buffered speed(+accel)+distance moves (41–47), encoders (16–20/78/79/91), velocity PID get/set (28/29/55/56 — including the protocol's D,P,I order-on-write vs P,I,D-on-read quirk and 16.16 fixed-point scaling), firmware version / battery / currents / PWMs / temperatures / status, EEPROM write, E-stop reset. Every command number and payload layout verified against the MCP user manual; commands whose manual entries are ambiguous or contain documentation errors (e.g. 133/134's copy-paste error) were deliberately omitted rather than guessed — noted in the source.Testing
🤖 Generated with Claude Code