refactor(motor-control): shared MotorController concept + unified Axis-form API - #764
refactor(motor-control): shared MotorController concept + unified Axis-form API#764finger563 wants to merge 3 commits into
Conversation
…p266 Introduce espp::MotorAxis + the espp::MotorController compile-time concept (base_component/motor_controller.hpp): the common dual-channel surface both the serial (Basicmicro) and CANopen (Mcp266) drivers should present -- drive_duty/ drive_speed(Axis), read_encoder/read_speed(Axis, int32_t&), reset_estop, read_main_battery_voltage/read_temperature. Mcp266 now aliases Axis = MotorAxis and static_asserts conformance. Also the deferred mcp266 clarity/consistency tweaks: - configure_position_loop: ec moved to LAST (fallback_p now precedes it), with a 4-arg convenience overload using the default gain -- restoring the ec-trailing convention the rest of the codebase holds to. - set_position_limits -> set_software_position_limits, to disambiguate it from configure_position_loop's manufacturer PID clamp (docs cross-reference both). Basicmicro migration to the Axis-form surface + collision rename follows on this branch. Builds clean: mcp266 example on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrate Basicmicro's per-channel commands from the drive_m1_*/drive_m2_* method pairs to a single Axis-form taking espp::MotorAxis, so it presents the same channel-selected surface as espp::Mcp266 and satisfies espp::MotorController (static_assert added). Generic code can now drive either transport by axis. Per-channel commands collapsed (M1/M2 pair -> one Axis method that selects the command byte from the axis): drive_duty, drive_speed, drive_speed_accel, buffered_drive_speed_distance, buffered_drive_speed_accel_distance, set/read_velocity_pid, set/read_position_pid, read_encoder, read_speed. This removes ~200 lines of near-verbatim duplication. Naming / correctness: - The both-channels commands, which previously shared the drive_duty/drive_speed names with the (new) single-channel meaning, are renamed drive_both_duty / drive_both_speed / drive_both_speed_accel / buffered_drive_both_* so the unqualified name always means "one channel" (matching Mcp266 and killing the cross-component name collision). - e_stop_reset -> reset_estop (matches Mcp266 / the concept). - read_encoder now returns a SIGNED int32 count (was uint32): a quadrature encoder run in reverse reads as negative, as callers expect. A status-byte overload is retained; read_speed likewise keeps a direction-byte overload. - Protected wire helpers that collided by name with the new public methods are renamed *_raw (read_count_raw, read_speed_raw, set/read_velocity_pid_raw, set/read_position_pid_raw). Wire format is unchanged: every Axis method emits the exact same command byte and payload the old M1/M2 method did. Example + docs updated. Builds clean: basicmicro example on IDF v6.0.1 (esp32); host test unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces breaking API refactors across two motor drivers plus a new shared concept contract, which warrants final human review despite only a minor issue found.
Pull request overview
This PR unifies the dual-channel motor-controller APIs across espp::Basicmicro (packet-serial) and espp::Mcp266 (CANopen) by introducing a shared MotorAxis selector and enforcing a common MotorController concept contract, while refactoring Basicmicro’s API surface to be axis-driven and disambiguating “both channels” operations.
Changes:
- Added
components/base_component/include/motor_controller.hppdefiningespp::MotorAxisand theespp::MotorControllerconcept, and enforced conformance viastatic_assertin both drivers. - Refactored
Basicmicroto a unified Axis-form API (drive_duty(Axis, ...),read_encoder(Axis, ...), etc.) and renamed both-channel commands todrive_both_*/buffered_drive_both_*. - Aligned
Mcp266naming and signatures (Axisalias toMotorAxis,set_software_position_limits, andconfigure_position_loopparameter order + convenience overload), and updated docs/examples accordingly.
File summaries
| File | Description |
|---|---|
| doc/en/motor_control/basicmicro.rst | Updates the Basicmicro documentation snippet to use Axis-form API and signed encoder reads. |
| components/base_component/include/motor_controller.hpp | Introduces shared MotorAxis and MotorController concept for compile-time API conformance. |
| components/basicmicro/include/basicmicro.hpp | Refactors Basicmicro public API to Axis-form, renames both-channel commands, and adds concept conformance static_assert. |
| components/basicmicro/example/main/basicmicro_example.cpp | Updates example usage to the new Axis-form Basicmicro API and signed encoder reads. |
| components/mcp266/include/mcp266.hpp | Aligns Mcp266 to shared MotorAxis, adjusts configure_position_loop signature/overload, renames software limit API, and adds concept conformance static_assert. |
| components/mcp266/example/main/mcp266_example.cpp | Updates example to renamed software position-limits API. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
// :278 configure_position_loop — ec moves to LAST, fallback_p before it
mcp.configure_position_loop(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), rd_i32(pl, 9), ec)
// :285 set_position_limits -> set_software_position_limits
mcp.set_software_position_limits(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), ec)No protocol/wire change — same payload byte offsets, just the C++ argument order / method name. |
…ddress review Two #764 review follow-ups: 1. Relocate the shared interface. Per review, base_component is the CRTP/logger base-class home, not a shared-interface bucket. Move MotorAxis + the MotorController concept out to a new header-only component `motor_controller` -- mirroring how `bldc_types` holds the shared concepts for the BLDC family -- so the two sibling drivers share the contract without either depending on the other. New components/motor_controller: motor_controller.hpp (MotorAxis + the concept + an fmt formatter for MotorAxis), CMakeLists (REQUIRES format), manifest, and a doc page; basicmicro & mcp266 now REQUIRE motor_controller (CMake + idf_component.yml) -- the include and static_asserts are unchanged. Doxyfile + motor_control doc index updated. 2. mcp266 example: check set_software_position_limits()'s return (it was dropped, so an SDO timeout / invalid arg would leave limits unset while the example commanded moves regardless) and bail with a logged error like the neighboring configure_position_loop() call. Builds clean: basicmicro + mcp266 examples on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Consistency pass — shared
MotorControllerinterface, unified Axis-form APIThird and final PR of the basicmicro / canopen / mcp266 review series. Stacked on
#763 (clarity); rebase onto
mainonce #762 (bugs) and #763 (clarity) merge.The review found that the two drivers modelling the same MCP236/266 hardware —
espp::Basicmicro(packet serial) andespp::Mcp266(CANopen) — presentedgratuitously different APIs for the same operations, and that
Basicmicro'sdrive_duty(m1, m2)collided in meaning withMcp266'sdrive_duty(axis, duty)(one meant "both channels", the other "one channel"). This unifies them.
1. Shared
espp::MotorControllerconcept +espp::MotorAxisNew
base_component/include/motor_controller.hpp: a compile-time concept naming thecommon dual-channel surface both drivers should present —
drive_duty/drive_speedby
MotorAxis,read_encoder/read_speed(signedint32),reset_estop,read_main_battery_voltage,read_temperature. Both classesusing Axis = MotorAxisand
static_assertconformance, so generic code can drive either transport by axisand the contract is enforced at compile time.
2.
Basicmicro: Axis-form API (collapses ~200 lines of M1/M2 duplication)Every per-channel command pair (
drive_m1_*/drive_m2_*,read_encoder_m1/m2,read_encoder_speed_m1/m2,set/read_velocity_pid_m1/m2,set/read_position_pid_m1/m2,buffered variants) collapses to a single method taking
Axis, selecting the commandbyte from the axis.
drive_both_duty/drive_both_speed/drive_both_speed_accel/buffered_drive_both_*so the unqualified name alwaysmeans "one channel" (matching
Mcp266, killing the collision).e_stop_reset→reset_estop.read_encodernow returns a signedint32count (wasuint32) — reversequadrature reads negative, as callers expect; a status-byte overload is retained.
Wire format is unchanged — each Axis method emits the exact command byte + payload
the old M1/M2 method did.
3.
Mcp266alignment tidy-ups (deferred from the clarity PR)configure_position_loop:ecmoved to the last parameter (restoring thecodebase convention) with a convenience overload using the default position P gain.
set_position_limits→set_software_position_limits(disambiguates it from themanufacturer position-PID clamp).
Verification
basicmicroexample builds clean on IDF v6.0.1 (esp32); host test unchanged.mcp266example builds clean on IDF v6.0.1.static_assert(MotorController<...>)pass — the two drivers are nowinterchangeable through the concept.
Basicmicro:drive_m1_duty(d)→drive_duty(Axis::M1, d);drive_duty(m1, m2)→
drive_both_duty(m1, m2); same pattern for speed / PID / buffered / encoder reads;read_encoder_m1(uint32_t&, ...)→read_encoder(Axis::M1, int32_t&, ...);e_stop_reset→reset_estop.Mcp266:set_position_limits→set_software_position_limits;configure_position_loop(...)gains a required position-P argument beforeec(or use the 4-arg default-gain overload).
🤖 Generated with Claude Code