From 4a4c66c8e58c67cf3c68d910280a0aef254dd52b Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Wed, 2 Sep 2026 22:04:42 -0500
Subject: [PATCH 1/7] refactor(motor-control): add shared MotorController
 concept; align mcp266

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>
---
 .../include/motor_controller.hpp              | 50 +++++++++++++++++++
 .../mcp266/example/main/mcp266_example.cpp    |  2 +-
 components/mcp266/include/mcp266.hpp          | 29 ++++++++---
 3 files changed, 73 insertions(+), 8 deletions(-)
 create mode 100644 components/base_component/include/motor_controller.hpp

diff --git a/components/base_component/include/motor_controller.hpp b/components/base_component/include/motor_controller.hpp
new file mode 100644
index 0000000000..2c34cf43a4
--- /dev/null
+++ b/components/base_component/include/motor_controller.hpp
@@ -0,0 +1,50 @@
+#pragma once
+
+#include <concepts>
+#include <cstdint>
+#include <system_error>
+
+namespace espp {
+
+/// \brief Motor channel selector shared by the espp motor-controller drivers.
+/// \details M1 / M2 are the two output channels of a dual-channel controller
+///          (e.g. a Basicmicro MCP236/MCP266). Drivers that model this hardware
+///          family select a channel with this type so generic code can drive
+///          either transport (packet-serial espp::Basicmicro or CANopen
+///          espp::Mcp266) uniformly.
+enum class MotorAxis : uint8_t { M1 = 0, M2 = 1 };
+
+/// \brief Compile-time contract for a dual-channel motor controller.
+/// \details Both espp::Basicmicro (packet serial) and espp::Mcp266 (CANopen)
+///          model the same MCP236/266 hardware and satisfy this concept, so
+///          generic code can command either by axis. Every operation follows the
+///          espp convention: it returns \c true on success and sets \p ec on
+///          failure (leaving it cleared on success). Units are encoder counts
+///          (position), counts/s (velocity / "qpps"), a signed duty (±32767),
+///          volts, and degrees Celsius.
+/// \note Conformance is about the API surface, not that every command is active
+///       on every device in its current mode: a controller may accept a duty /
+///       speed command it cannot act on (e.g. Mcp266's manufacturer speed/duty
+///       mirror is inert on firmware where only CiA 402 position mode drives).
+///       Position control and per-device configuration are intentionally NOT
+///       part of this common surface -- they differ too much between transports.
+template <typename T>
+concept MotorController = requires(T t, MotorAxis axis, int16_t duty, int32_t qpps, int32_t count,
+                                   float value, std::error_code &ec) {
+  /// Open-loop signed duty on one channel.
+  { t.drive_duty(axis, duty, ec) } -> std::same_as<bool>;
+  /// Closed-loop signed speed (counts/s) on one channel.
+  { t.drive_speed(axis, qpps, ec) } -> std::same_as<bool>;
+  /// Read the signed encoder count of one channel.
+  { t.read_encoder(axis, count, ec) } -> std::same_as<bool>;
+  /// Read the signed encoder speed (counts/s) of one channel.
+  { t.read_speed(axis, qpps, ec) } -> std::same_as<bool>;
+  /// Clear a latched emergency stop.
+  { t.reset_estop(ec) } -> std::same_as<bool>;
+  /// Read the main battery / supply voltage (volts).
+  { t.read_main_battery_voltage(value, ec) } -> std::same_as<bool>;
+  /// Read the board temperature (degrees Celsius).
+  { t.read_temperature(value, ec) } -> std::same_as<bool>;
+};
+
+} // namespace espp
diff --git a/components/mcp266/example/main/mcp266_example.cpp b/components/mcp266/example/main/mcp266_example.cpp
index 08c145776f..52c29a8420 100644
--- a/components/mcp266/example/main/mcp266_example.cpp
+++ b/components/mcp266/example/main/mcp266_example.cpp
@@ -102,7 +102,7 @@ extern "C" void app_main(void) {
     logger.error("Failed to configure M1 position loop: {}", ec.message());
     return;
   }
-  mcp.set_position_limits(Axis::M1, -20'000, 20'000, ec);
+  mcp.set_software_position_limits(Axis::M1, -20'000, 20'000, ec);
 
   // Run a small profile-position sequence and report arrival.
   static constexpr int32_t targets[] = {10'000, -10'000, 0};
diff --git a/components/mcp266/include/mcp266.hpp b/components/mcp266/include/mcp266.hpp
index 240405d337..dcde434130 100644
--- a/components/mcp266/include/mcp266.hpp
+++ b/components/mcp266/include/mcp266.hpp
@@ -12,6 +12,7 @@
 #include "canopen_client.hpp"
 #include "detail/mcp266_core.hpp"
 #include "ds402.hpp"
+#include "motor_controller.hpp"
 
 namespace espp {
 
@@ -52,8 +53,8 @@ namespace espp {
 /// \snippet mcp266_example.cpp mcp266 example
 class Mcp266 : public BaseComponent {
 public:
-  /// \brief Motor channel selector.
-  enum class Axis { M1, M2 };
+  /// \brief Motor channel selector (shared across the espp motor drivers).
+  using Axis = MotorAxis;
 
   /// \brief Configuration for the Mcp266 controller.
   struct Config {
@@ -146,11 +147,11 @@ class Mcp266 : public BaseComponent {
   /// \param axis The motor channel.
   /// \param min_pos Minimum commandable position.
   /// \param max_pos Maximum commandable position.
-  /// \param ec Set on failure.
   /// \param fallback_p Position P gain to seed when the stored gain is zero.
+  /// \param ec Set on failure.
   /// \return True on success.
-  bool configure_position_loop(Axis axis, int32_t min_pos, int32_t max_pos, std::error_code &ec,
-                               int32_t fallback_p = kDefaultPositionP) {
+  bool configure_position_loop(Axis axis, int32_t min_pos, int32_t max_pos, int32_t fallback_p,
+                               std::error_code &ec) {
     ec.clear();
     AxisState &a = axis_state(axis);
     std::array<int32_t, 7> readback{};
@@ -193,13 +194,23 @@ class Mcp266 : public BaseComponent {
     return true;
   }
 
-  /// \brief Set the CiA 402 software position limits (0x607D:1/:2) for an axis.
+  /// \brief Configure an axis's position loop using the default coarse fallback
+  ///        P gain (see the fallback_p overload). Convenience for the common case.
+  bool configure_position_loop(Axis axis, int32_t min_pos, int32_t max_pos, std::error_code &ec) {
+    return configure_position_loop(axis, min_pos, max_pos, kDefaultPositionP, ec);
+  }
+
+  /// \brief Set the CiA 402 software position limits (object 0x607D:1/:2) for an
+  ///        axis — a per-move envelope enforced by the drive's trajectory
+  ///        generator. Distinct from configure_position_loop()'s min/max, which
+  ///        writes the manufacturer position-PID MinPos/MaxPos clamp.
   /// \param axis The motor channel.
   /// \param min_pos Lower limit.
   /// \param max_pos Upper limit.
   /// \param ec Set on failure.
   /// \return True on success.
-  bool set_position_limits(Axis axis, int32_t min_pos, int32_t max_pos, std::error_code &ec) {
+  bool set_software_position_limits(Axis axis, int32_t min_pos, int32_t max_pos,
+                                    std::error_code &ec) {
     ec.clear();
     if (min_pos > max_pos) {
       ec = std::make_error_code(std::errc::invalid_argument);
@@ -422,4 +433,8 @@ class Mcp266 : public BaseComponent {
   AxisState m2_;
 };
 
+static_assert(
+    MotorController<Mcp266>,
+    "Mcp266 must satisfy the espp::MotorController concept (kept in sync with Basicmicro)");
+
 } // namespace espp

From 321c357480e1e52469448f3721a0e9f2d6964d80 Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Wed, 2 Sep 2026 22:12:48 -0500
Subject: [PATCH 2/7] refactor(basicmicro): Axis-form API; satisfy shared
 MotorController

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>
---
 .../example/main/basicmicro_example.cpp       |  18 +-
 components/basicmicro/include/basicmicro.hpp  | 351 +++++++-----------
 doc/en/motor_control/basicmicro.rst           |   9 +-
 3 files changed, 146 insertions(+), 232 deletions(-)

diff --git a/components/basicmicro/example/main/basicmicro_example.cpp b/components/basicmicro/example/main/basicmicro_example.cpp
index 18ebd46188..ae0e735274 100644
--- a/components/basicmicro/example/main/basicmicro_example.cpp
+++ b/components/basicmicro/example/main/basicmicro_example.cpp
@@ -79,33 +79,33 @@ extern "C" void app_main(void) {
 
   // gentle speed ramp on M1 (up to ~12.5% duty) with encoder readback, then
   // back down to a stop. Duty-cycle drive works without a tuned velocity PID;
-  // if your encoders + PID are configured, try drive_m1_speed() instead.
+  // if your encoders + PID are configured, try drive_speed(Axis::M1, ...)
+  // instead. The channel is selected with the shared espp::MotorAxis enum.
+  using Axis = espp::Basicmicro::Axis;
   static constexpr int16_t max_duty = 4096; // of 32767
   static constexpr int16_t step = 512;
   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());
+    if (!mcp.drive_duty(Axis::M1, duty, ec)) {
+      logger.error("drive_duty(M1, {}) failed: {}", duty, ec.message());
       break;
     }
     std::this_thread::sleep_for(250ms);
-    uint32_t count{0};
-    uint8_t enc_status{0};
+    int32_t count{0};
     int32_t speed{0};
     uint8_t direction{0};
-    if (mcp.read_encoder_m1(count, enc_status, ec) &&
-        mcp.read_encoder_speed_m1(speed, direction, ec)) {
+    if (mcp.read_encoder(Axis::M1, count, ec) && mcp.read_speed(Axis::M1, speed, direction, ec)) {
       logger.info("duty {:5d}: encoder count = {:10d}, speed = {} pulses/s ({})", duty, count,
                   speed, direction ? "backward" : "forward");
     }
   }
   for (int16_t duty = max_duty; duty >= 0; duty = static_cast<int16_t>(duty - step)) {
-    if (!mcp.drive_m1_duty(duty, ec))
+    if (!mcp.drive_duty(Axis::M1, duty, ec))
       break;
     std::this_thread::sleep_for(100ms);
   }
 
   // make sure the motor is stopped
-  if (mcp.drive_m1_duty(0, ec))
+  if (mcp.drive_duty(Axis::M1, 0, ec))
     logger.info("Motor stopped");
 
   //! [basicmicro example]
diff --git a/components/basicmicro/include/basicmicro.hpp b/components/basicmicro/include/basicmicro.hpp
index 72d14448e1..6a69b1f141 100644
--- a/components/basicmicro/include/basicmicro.hpp
+++ b/components/basicmicro/include/basicmicro.hpp
@@ -12,6 +12,7 @@
 
 #include "base_component.hpp"
 #include "detail/basicmicro_core.hpp"
+#include "motor_controller.hpp"
 
 namespace espp {
 
@@ -58,6 +59,10 @@ class Basicmicro : public BaseComponent {
   using Command = detail::BasicmicroCommand;
   /// Status bit masks returned by read_status() (manual command 90).
   using Status = detail::BasicmicroStatus;
+  /// Motor channel selector, shared with the other espp motor drivers (e.g.
+  /// espp::Mcp266) so generic code can command either transport by axis.
+  /// \see espp::MotorController
+  using Axis = MotorAxis;
 
   /// Function used to transmit a complete packet to the controller.
   /// Should return true when all bytes were written.
@@ -112,39 +117,28 @@ class Basicmicro : public BaseComponent {
   // ------------------------- duty-cycle drive ------------------------------
 
   /**
-   * @brief Drive motor 1 with a signed duty cycle (command 32).
+   * @brief Drive one motor with a signed duty cycle (commands 32 / 33).
+   * @param axis Motor channel to drive.
    * @param duty Signed duty, -32767 to +32767 (= -100% to +100%).
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_m1_duty(int16_t duty, std::error_code &ec) {
+  bool drive_duty(Axis axis, int16_t duty, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i16_be(payload, duty);
-    return write_command(Command::DriveM1SignedDuty, payload, ec);
+    return write_command(axis == Axis::M1 ? Command::DriveM1SignedDuty : Command::DriveM2SignedDuty,
+                         payload, ec);
   }
 
   /**
-   * @brief Drive motor 2 with a signed duty cycle (command 33).
-   * @param duty Signed duty, -32767 to +32767 (= -100% to +100%).
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool drive_m2_duty(int16_t duty, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    std::vector<uint8_t> payload;
-    detail::append_i16_be(payload, duty);
-    return write_command(Command::DriveM2SignedDuty, payload, ec);
-  }
-
-  /**
-   * @brief Drive both motors with signed duty cycles (command 34).
+   * @brief Drive both motors with signed duty cycles in one packet (command 34).
    * @param duty_m1 Signed duty for motor 1, -32767 to +32767.
    * @param duty_m2 Signed duty for motor 2, -32767 to +32767.
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_duty(int16_t duty_m1, int16_t duty_m2, std::error_code &ec) {
+  bool drive_both_duty(int16_t duty_m1, int16_t duty_m2, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i16_be(payload, duty_m1);
@@ -155,42 +149,29 @@ class Basicmicro : public BaseComponent {
   // ------------------------ closed-loop speed drive ------------------------
 
   /**
-   * @brief Drive motor 1 at a signed speed in quadrature pulses per second
-   *        (command 35). Requires an encoder and tuned velocity PID.
+   * @brief Drive one motor at a signed speed in quadrature pulses per second
+   *        (commands 35 / 36). Requires an encoder and tuned velocity PID.
+   * @param axis Motor channel to drive.
    * @param qpps Signed speed in quad pulses per second.
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_m1_speed(int32_t qpps, std::error_code &ec) {
+  bool drive_speed(Axis axis, int32_t qpps, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps);
-    return write_command(Command::DriveM1SignedSpeed, payload, ec);
+    return write_command(
+        axis == Axis::M1 ? Command::DriveM1SignedSpeed : Command::DriveM2SignedSpeed, payload, ec);
   }
 
   /**
-   * @brief Drive motor 2 at a signed speed in quadrature pulses per second
-   *        (command 36). Requires an encoder and tuned velocity PID.
-   * @param qpps Signed speed in quad pulses per second.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool drive_m2_speed(int32_t qpps, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    std::vector<uint8_t> payload;
-    detail::append_i32_be(payload, qpps);
-    return write_command(Command::DriveM2SignedSpeed, payload, ec);
-  }
-
-  /**
-   * @brief Drive both motors at signed speeds in quadrature pulses per second
-   *        (command 37).
+   * @brief Drive both motors at signed speeds in one packet (command 37).
    * @param qpps_m1 Signed speed for motor 1 in quad pulses per second.
    * @param qpps_m2 Signed speed for motor 2 in quad pulses per second.
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_speed(int32_t qpps_m1, int32_t qpps_m2, std::error_code &ec) {
+  bool drive_both_speed(int32_t qpps_m1, int32_t qpps_m2, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps_m1);
@@ -199,35 +180,22 @@ class Basicmicro : public BaseComponent {
   }
 
   /**
-   * @brief Drive motor 1 at a signed speed with an acceleration ramp
-   *        (command 38).
+   * @brief Drive one motor at a signed speed with an acceleration ramp
+   *        (commands 38 / 39).
+   * @param axis Motor channel to drive.
    * @param accel Acceleration in qpps per second (unsigned).
    * @param qpps Signed target speed in quad pulses per second.
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_m1_speed_accel(uint32_t accel, int32_t qpps, std::error_code &ec) {
+  bool drive_speed_accel(Axis axis, uint32_t accel, int32_t qpps, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
     detail::append_i32_be(payload, qpps);
-    return write_command(Command::DriveM1SignedSpeedAccel, payload, ec);
-  }
-
-  /**
-   * @brief Drive motor 2 at a signed speed with an acceleration ramp
-   *        (command 39).
-   * @param accel Acceleration in qpps per second (unsigned).
-   * @param qpps Signed target speed in quad pulses per second.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool drive_m2_speed_accel(uint32_t accel, int32_t qpps, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    std::vector<uint8_t> payload;
-    detail::append_u32_be(payload, accel);
-    detail::append_i32_be(payload, qpps);
-    return write_command(Command::DriveM2SignedSpeedAccel, payload, ec);
+    return write_command(axis == Axis::M1 ? Command::DriveM1SignedSpeedAccel
+                                          : Command::DriveM2SignedSpeedAccel,
+                         payload, ec);
   }
 
   /**
@@ -239,7 +207,8 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool drive_speed_accel(uint32_t accel, int32_t qpps_m1, int32_t qpps_m2, std::error_code &ec) {
+  bool drive_both_speed_accel(uint32_t accel, int32_t qpps_m1, int32_t qpps_m2,
+                              std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
@@ -251,8 +220,9 @@ class Basicmicro : public BaseComponent {
   // -------------------------- buffered motion ------------------------------
 
   /**
-   * @brief Buffered drive of motor 1 with signed speed and distance
-   *        (command 41).
+   * @brief Buffered drive of one motor with signed speed and distance
+   *        (commands 41 / 42).
+   * @param axis Motor channel to drive.
    * @param qpps Signed speed in quad pulses per second.
    * @param distance Distance in quad pulses (unsigned).
    * @param immediate If true, stop the currently-executing command, flush the
@@ -261,34 +231,16 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool buffered_drive_m1_speed_distance(int32_t qpps, uint32_t distance, bool immediate,
-                                        std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    std::vector<uint8_t> payload;
-    detail::append_i32_be(payload, qpps);
-    detail::append_u32_be(payload, distance);
-    detail::append_u8(payload, immediate ? 1 : 0);
-    return write_command(Command::BufferedM1SpeedDistance, payload, ec);
-  }
-
-  /**
-   * @brief Buffered drive of motor 2 with signed speed and distance
-   *        (command 42).
-   * @param qpps Signed speed in quad pulses per second.
-   * @param distance Distance in quad pulses (unsigned).
-   * @param immediate If true, stop the currently-executing command, flush the
-   *        buffer and run this command now; if false, queue it.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool buffered_drive_m2_speed_distance(int32_t qpps, uint32_t distance, bool immediate,
-                                        std::error_code &ec) {
+  bool buffered_drive_speed_distance(Axis axis, int32_t qpps, uint32_t distance, bool immediate,
+                                     std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps);
     detail::append_u32_be(payload, distance);
     detail::append_u8(payload, immediate ? 1 : 0);
-    return write_command(Command::BufferedM2SpeedDistance, payload, ec);
+    return write_command(axis == Axis::M1 ? Command::BufferedM1SpeedDistance
+                                          : Command::BufferedM2SpeedDistance,
+                         payload, ec);
   }
 
   /**
@@ -303,8 +255,9 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool buffered_drive_speed_distance(int32_t qpps_m1, uint32_t distance_m1, int32_t qpps_m2,
-                                     uint32_t distance_m2, bool immediate, std::error_code &ec) {
+  bool buffered_drive_both_speed_distance(int32_t qpps_m1, uint32_t distance_m1, int32_t qpps_m2,
+                                          uint32_t distance_m2, bool immediate,
+                                          std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps_m1);
@@ -316,30 +269,9 @@ class Basicmicro : public BaseComponent {
   }
 
   /**
-   * @brief Buffered drive of motor 1 with acceleration, signed speed and
-   *        distance (command 44).
-   * @param accel Acceleration in qpps per second (unsigned).
-   * @param qpps Signed speed in quad pulses per second.
-   * @param distance Distance in quad pulses (unsigned).
-   * @param immediate If true, stop the currently-executing command, flush the
-   *        buffer and run this command now; if false, queue it.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool buffered_drive_m1_speed_accel_distance(uint32_t accel, int32_t qpps, uint32_t distance,
-                                              bool immediate, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    std::vector<uint8_t> payload;
-    detail::append_u32_be(payload, accel);
-    detail::append_i32_be(payload, qpps);
-    detail::append_u32_be(payload, distance);
-    detail::append_u8(payload, immediate ? 1 : 0);
-    return write_command(Command::BufferedM1SpeedAccelDistance, payload, ec);
-  }
-
-  /**
-   * @brief Buffered drive of motor 2 with acceleration, signed speed and
-   *        distance (command 45).
+   * @brief Buffered drive of one motor with acceleration, signed speed and
+   *        distance (commands 44 / 45).
+   * @param axis Motor channel to drive.
    * @param accel Acceleration in qpps per second (unsigned).
    * @param qpps Signed speed in quad pulses per second.
    * @param distance Distance in quad pulses (unsigned).
@@ -348,15 +280,17 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool buffered_drive_m2_speed_accel_distance(uint32_t accel, int32_t qpps, uint32_t distance,
-                                              bool immediate, std::error_code &ec) {
+  bool buffered_drive_speed_accel_distance(Axis axis, uint32_t accel, int32_t qpps,
+                                           uint32_t distance, bool immediate, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
     detail::append_i32_be(payload, qpps);
     detail::append_u32_be(payload, distance);
     detail::append_u8(payload, immediate ? 1 : 0);
-    return write_command(Command::BufferedM2SpeedAccelDistance, payload, ec);
+    return write_command(axis == Axis::M1 ? Command::BufferedM1SpeedAccelDistance
+                                          : Command::BufferedM2SpeedAccelDistance,
+                         payload, ec);
   }
 
   /**
@@ -372,9 +306,10 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool buffered_drive_speed_accel_distance(uint32_t accel, int32_t qpps_m1, uint32_t distance_m1,
-                                           int32_t qpps_m2, uint32_t distance_m2, bool immediate,
-                                           std::error_code &ec) {
+  bool buffered_drive_both_speed_accel_distance(uint32_t accel, int32_t qpps_m1,
+                                                uint32_t distance_m1, int32_t qpps_m2,
+                                                uint32_t distance_m2, bool immediate,
+                                                std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
@@ -407,33 +342,41 @@ class Basicmicro : public BaseComponent {
   // ------------------------------ encoders ---------------------------------
 
   /**
-   * @brief Read the motor 1 encoder count / position (command 16).
-   * @param count Encoder count (quadrature: full 32-bit range; absolute:
-   *        0-4095).
-   * @param status Status bits: bit0 = underflow occurred (cleared on read),
-   *        bit1 = direction (0 forward, 1 backward), bit2 = overflow occurred
-   *        (cleared on read).
+   * @brief Read one motor's encoder count / position (commands 16 / 17).
+   * @param axis Motor channel to read.
+   * @param count Signed encoder count (quadrature counters wrap through the full
+   *        32-bit range; an absolute encoder reports 0-4095).
    * @param ec Set on failure.
    * @return True on success.
+   * @note The controller reports the counter as raw 32 bits; it is returned here
+   *       as a signed int32 so a quadrature encoder run in reverse reads as a
+   *       negative count. Use the overload taking a @c status out-parameter for
+   *       the underflow / direction / overflow flags.
    */
-  bool read_encoder_m1(uint32_t &count, uint8_t &status, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return read_encoder(Command::ReadEncoderM1, count, status, ec);
+  bool read_encoder(Axis axis, int32_t &count, std::error_code &ec) {
+    uint8_t status = 0;
+    return read_encoder(axis, count, status, ec);
   }
 
   /**
-   * @brief Read the motor 2 encoder count / position (command 17).
-   * @param count Encoder count (quadrature: full 32-bit range; absolute:
-   *        0-4095).
+   * @brief Read one motor's encoder count / position and status byte
+   *        (commands 16 / 17).
+   * @param axis Motor channel to read.
+   * @param count Signed encoder count (see the two-argument overload).
    * @param status Status bits: bit0 = underflow occurred (cleared on read),
    *        bit1 = direction (0 forward, 1 backward), bit2 = overflow occurred
    *        (cleared on read).
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool read_encoder_m2(uint32_t &count, uint8_t &status, std::error_code &ec) {
+  bool read_encoder(Axis axis, int32_t &count, uint8_t &status, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return read_encoder(Command::ReadEncoderM2, count, status, ec);
+    uint32_t raw = 0;
+    if (!read_count_raw(axis == Axis::M1 ? Command::ReadEncoderM1 : Command::ReadEncoderM2, raw,
+                        status, ec))
+      return false;
+    count = static_cast<int32_t>(raw);
+    return true;
   }
 
   /**
@@ -464,27 +407,33 @@ class Basicmicro : public BaseComponent {
   }
 
   /**
-   * @brief Read the motor 1 encoder speed in pulses per second (command 18).
+   * @brief Read one motor's encoder speed in pulses per second
+   *        (commands 18 / 19).
+   * @param axis Motor channel to read.
    * @param qpps Speed in pulses per second (as reported by the controller).
-   * @param direction 0 = forward, 1 = backward.
    * @param ec Set on failure.
    * @return True on success.
+   * @note Use the overload taking a @c direction out-parameter to also read the
+   *       0 = forward / 1 = backward flag.
    */
-  bool read_encoder_speed_m1(int32_t &qpps, uint8_t &direction, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return read_speed(Command::ReadEncoderSpeedM1, qpps, direction, ec);
+  bool read_speed(Axis axis, int32_t &qpps, std::error_code &ec) {
+    uint8_t direction = 0;
+    return read_speed(axis, qpps, direction, ec);
   }
 
   /**
-   * @brief Read the motor 2 encoder speed in pulses per second (command 19).
+   * @brief Read one motor's encoder speed and direction (commands 18 / 19).
+   * @param axis Motor channel to read.
    * @param qpps Speed in pulses per second (as reported by the controller).
    * @param direction 0 = forward, 1 = backward.
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool read_encoder_speed_m2(int32_t &qpps, uint8_t &direction, std::error_code &ec) {
+  bool read_speed(Axis axis, int32_t &qpps, uint8_t &direction, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return read_speed(Command::ReadEncoderSpeedM2, qpps, direction, ec);
+    return read_speed_raw(axis == Axis::M1 ? Command::ReadEncoderSpeedM1
+                                           : Command::ReadEncoderSpeedM2,
+                          qpps, direction, ec);
   }
 
   /**
@@ -526,11 +475,12 @@ class Basicmicro : public BaseComponent {
   // ----------------------------- velocity PID ------------------------------
 
   /**
-   * @brief Set the motor 1 velocity PID constants and QPPS (command 28).
+   * @brief Set one motor's velocity PID constants and QPPS (commands 28 / 29).
    *
    * Gains are converted to the controller's 16.16 fixed-point representation
    * (value * 65536); the controller defaults correspond to P=1.0, I=0.5,
    * D=0.25, QPPS=44000.
+   * @param axis Motor channel to configure.
    * @param p Proportional gain.
    * @param i Integral gain.
    * @param d Derivative gain.
@@ -538,29 +488,17 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool set_velocity_pid_m1(float p, float i, float d, uint32_t qpps, std::error_code &ec) {
+  bool set_velocity_pid(Axis axis, float p, float i, float d, uint32_t qpps, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return set_velocity_pid(Command::SetVelocityPidM1, p, i, d, qpps, ec);
+    return set_velocity_pid_raw(axis == Axis::M1 ? Command::SetVelocityPidM1
+                                                 : Command::SetVelocityPidM2,
+                                p, i, d, qpps, ec);
   }
 
   /**
-   * @brief Set the motor 2 velocity PID constants and QPPS (command 29).
-   *        See set_velocity_pid_m1() for the fixed-point conversion.
-   * @param p Proportional gain.
-   * @param i Integral gain.
-   * @param d Derivative gain.
-   * @param qpps Encoder speed (quad pulses per second) at 100% motor power.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool set_velocity_pid_m2(float p, float i, float d, uint32_t qpps, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return set_velocity_pid(Command::SetVelocityPidM2, p, i, d, qpps, ec);
-  }
-
-  /**
-   * @brief Read the motor 1 velocity PID constants and QPPS (command 55).
+   * @brief Read one motor's velocity PID constants and QPPS (commands 55 / 56).
    *        Fixed-point values are converted back to floats (divide by 65536).
+   * @param axis Motor channel to read.
    * @param p Proportional gain.
    * @param i Integral gain.
    * @param d Derivative gain.
@@ -568,28 +506,16 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool read_velocity_pid_m1(float &p, float &i, float &d, uint32_t &qpps, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return read_velocity_pid(Command::ReadVelocityPidM1, p, i, d, qpps, ec);
-  }
-
-  /**
-   * @brief Read the motor 2 velocity PID constants and QPPS (command 56).
-   *        Fixed-point values are converted back to floats (divide by 65536).
-   * @param p Proportional gain.
-   * @param i Integral gain.
-   * @param d Derivative gain.
-   * @param qpps Encoder speed (quad pulses per second) at 100% motor power.
-   * @param ec Set on failure.
-   * @return True on success.
-   */
-  bool read_velocity_pid_m2(float &p, float &i, float &d, uint32_t &qpps, std::error_code &ec) {
+  bool read_velocity_pid(Axis axis, float &p, float &i, float &d, uint32_t &qpps,
+                         std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return read_velocity_pid(Command::ReadVelocityPidM2, p, i, d, qpps, ec);
+    return read_velocity_pid_raw(axis == Axis::M1 ? Command::ReadVelocityPidM1
+                                                  : Command::ReadVelocityPidM2,
+                                 p, i, d, qpps, ec);
   }
 
   /**
-   * @brief Set the motor 1 position PID constants (command 61).
+   * @brief Set one motor's position PID constants (commands 61 / 62).
    *
    * The position loop has seven constants: P, I, D gains (transferred scaled
    * by 1024), MaxI (integral windup limit), Deadzone (in encoder counts), and
@@ -598,6 +524,7 @@ class Basicmicro : public BaseComponent {
    * so position commands (65-67) produce no motion until these are set. Note
    * the wire order for this command is D, P, I (unlike the P, I, D read order
    * of command 63).
+   * @param axis Motor channel to configure.
    * @param p Proportional gain.
    * @param i Integral gain.
    * @param d Derivative gain.
@@ -608,28 +535,19 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool set_position_pid_m1(float p, float i, float d, uint32_t max_i, uint32_t deadzone,
-                           int32_t min_pos, int32_t max_pos, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return set_position_pid(Command::SetPositionPidM1, p, i, d, max_i, deadzone, min_pos, max_pos,
-                            ec);
-  }
-
-  /**
-   * @brief Set the motor 2 position PID constants (command 62).
-   *        See set_position_pid_m1() for the constants and scaling.
-   */
-  bool set_position_pid_m2(float p, float i, float d, uint32_t max_i, uint32_t deadzone,
-                           int32_t min_pos, int32_t max_pos, std::error_code &ec) {
+  bool set_position_pid(Axis axis, float p, float i, float d, uint32_t max_i, uint32_t deadzone,
+                        int32_t min_pos, int32_t max_pos, std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return set_position_pid(Command::SetPositionPidM2, p, i, d, max_i, deadzone, min_pos, max_pos,
-                            ec);
+    return set_position_pid_raw(axis == Axis::M1 ? Command::SetPositionPidM1
+                                                 : Command::SetPositionPidM2,
+                                p, i, d, max_i, deadzone, min_pos, max_pos, ec);
   }
 
   /**
-   * @brief Read the motor 1 position PID constants (command 63).
+   * @brief Read one motor's position PID constants (commands 63 / 64).
    *        Gains are converted back to floats (divide by 1024). The reply
    *        order is P, I, D (unlike the D, P, I write order of command 61).
+   * @param axis Motor channel to read.
    * @param p Proportional gain.
    * @param i Integral gain.
    * @param d Derivative gain.
@@ -640,22 +558,13 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool read_position_pid_m1(float &p, float &i, float &d, uint32_t &max_i, uint32_t &deadzone,
-                            int32_t &min_pos, int32_t &max_pos, std::error_code &ec) {
-    std::scoped_lock lk(mutex_);
-    return read_position_pid(Command::ReadPositionPidM1, p, i, d, max_i, deadzone, min_pos, max_pos,
-                             ec);
-  }
-
-  /**
-   * @brief Read the motor 2 position PID constants (command 64).
-   *        See read_position_pid_m1() for the fields and scaling.
-   */
-  bool read_position_pid_m2(float &p, float &i, float &d, uint32_t &max_i, uint32_t &deadzone,
-                            int32_t &min_pos, int32_t &max_pos, std::error_code &ec) {
+  bool read_position_pid(Axis axis, float &p, float &i, float &d, uint32_t &max_i,
+                         uint32_t &deadzone, int32_t &min_pos, int32_t &max_pos,
+                         std::error_code &ec) {
     std::scoped_lock lk(mutex_);
-    return read_position_pid(Command::ReadPositionPidM2, p, i, d, max_i, deadzone, min_pos, max_pos,
-                             ec);
+    return read_position_pid_raw(axis == Axis::M1 ? Command::ReadPositionPidM1
+                                                  : Command::ReadPositionPidM2,
+                                 p, i, d, max_i, deadzone, min_pos, max_pos, ec);
   }
 
   // -------------------------------- telemetry ------------------------------
@@ -864,7 +773,7 @@ class Basicmicro : public BaseComponent {
    * @param ec Set on failure.
    * @return True on success.
    */
-  bool e_stop_reset(std::error_code &ec) {
+  bool reset_estop(std::error_code &ec) {
     std::scoped_lock lk(mutex_);
     return write_command(Command::EStopReset, {}, ec);
   }
@@ -963,7 +872,7 @@ class Basicmicro : public BaseComponent {
   }
 
   /// Shared implementation for commands 16/17 (count + status byte).
-  bool read_encoder(Command cmd, uint32_t &count, uint8_t &status, std::error_code &ec) {
+  bool read_count_raw(Command cmd, uint32_t &count, uint8_t &status, std::error_code &ec) {
     uint8_t data[5] = {};
     if (!read_command(cmd, data, ec))
       return false;
@@ -973,7 +882,7 @@ class Basicmicro : public BaseComponent {
   }
 
   /// Shared implementation for commands 18/19/30/31 (speed + direction byte).
-  bool read_speed(Command cmd, int32_t &qpps, uint8_t &direction, std::error_code &ec) {
+  bool read_speed_raw(Command cmd, int32_t &qpps, uint8_t &direction, std::error_code &ec) {
     uint8_t data[5] = {};
     if (!read_command(cmd, data, ec))
       return false;
@@ -992,8 +901,8 @@ class Basicmicro : public BaseComponent {
   }
 
   /// Shared implementation for commands 28/29. Wire order is D, P, I, QPPS.
-  bool set_velocity_pid(Command cmd, float p, float i, float d, uint32_t qpps,
-                        std::error_code &ec) {
+  bool set_velocity_pid_raw(Command cmd, float p, float i, float d, uint32_t qpps,
+                            std::error_code &ec) {
     std::vector<uint8_t> payload;
     // Route through scale_pid_gain (rounds; guards negative -> uint32 wrap and
     // NaN/inf -> UB in std::llround) just like the position path — a raw
@@ -1006,8 +915,8 @@ class Basicmicro : public BaseComponent {
   }
 
   /// Shared implementation for commands 55/56. Wire order is P, I, D, QPPS.
-  bool read_velocity_pid(Command cmd, float &p, float &i, float &d, uint32_t &qpps,
-                         std::error_code &ec) {
+  bool read_velocity_pid_raw(Command cmd, float &p, float &i, float &d, uint32_t &qpps,
+                             std::error_code &ec) {
     uint8_t data[16] = {};
     if (!read_command(cmd, data, ec))
       return false;
@@ -1020,8 +929,9 @@ class Basicmicro : public BaseComponent {
 
   /// Shared implementation for commands 61/62. Wire order is
   /// D, P, I, MaxI, Deadzone, MinPos, MaxPos (P/I/D scaled by 1024).
-  bool set_position_pid(Command cmd, float p, float i, float d, uint32_t max_i, uint32_t deadzone,
-                        int32_t min_pos, int32_t max_pos, std::error_code &ec) {
+  bool set_position_pid_raw(Command cmd, float p, float i, float d, uint32_t max_i,
+                            uint32_t deadzone, int32_t min_pos, int32_t max_pos,
+                            std::error_code &ec) {
     std::vector<uint8_t> payload;
     payload.reserve(28); // fixed 7 x 4 bytes; avoid incremental reallocations
     detail::append_u32_be(payload, detail::scale_pid_gain(d, detail::kBasicmicroPositionPidScale));
@@ -1036,9 +946,9 @@ class Basicmicro : public BaseComponent {
 
   /// Shared implementation for commands 63/64. Reply order is
   /// P, I, D, MaxI, Deadzone, MinPos, MaxPos (P/I/D scaled by 1024).
-  bool read_position_pid(Command cmd, float &p, float &i, float &d, uint32_t &max_i,
-                         uint32_t &deadzone, int32_t &min_pos, int32_t &max_pos,
-                         std::error_code &ec) {
+  bool read_position_pid_raw(Command cmd, float &p, float &i, float &d, uint32_t &max_i,
+                             uint32_t &deadzone, int32_t &min_pos, int32_t &max_pos,
+                             std::error_code &ec) {
     uint8_t data[28] = {};
     if (!read_command(cmd, data, ec))
       return false;
@@ -1059,4 +969,7 @@ class Basicmicro : public BaseComponent {
   std::mutex mutex_;
 };
 
+static_assert(MotorController<Basicmicro>,
+              "Basicmicro must satisfy the shared espp::MotorController interface");
+
 } // namespace espp
diff --git a/doc/en/motor_control/basicmicro.rst b/doc/en/motor_control/basicmicro.rst
index c773d5bb14..8afb7a5563 100644
--- a/doc/en/motor_control/basicmicro.rst
+++ b/doc/en/motor_control/basicmicro.rst
@@ -61,12 +61,13 @@ Basic Usage
   });
 
   std::error_code ec;
+  using Axis = espp::Basicmicro::Axis;   // shared espp::MotorAxis (M1 / M2)
   std::string version;
   mcp.read_firmware_version(version, ec);
-  mcp.drive_m1_duty(4096, ec);   // ~12.5% duty
-  uint32_t count; uint8_t status;
-  mcp.read_encoder_m1(count, status, ec);
-  mcp.drive_m1_duty(0, ec);
+  mcp.drive_duty(Axis::M1, 4096, ec);    // ~12.5% duty
+  int32_t count;
+  mcp.read_encoder(Axis::M1, count, ec);
+  mcp.drive_duty(Axis::M1, 0, ec);
 
 .. ------------------------------- Example -------------------------------------
 

From 185d3a0f0dbaf5d88621ecf6834727b7acb92f55 Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Wed, 2 Sep 2026 23:00:39 -0500
Subject: [PATCH 3/7] refactor(motor-control): move MotorController to its own
 component; address 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>
---
 components/basicmicro/CMakeLists.txt          |  2 +-
 components/basicmicro/idf_component.yml       |  1 +
 components/mcp266/CMakeLists.txt              |  2 +-
 .../mcp266/example/main/mcp266_example.cpp    |  5 ++-
 components/mcp266/idf_component.yml           |  1 +
 components/motor_controller/CMakeLists.txt    |  4 +++
 components/motor_controller/idf_component.yml | 19 ++++++++++++
 .../include/motor_controller.hpp              |  9 ++++++
 doc/Doxyfile                                  |  1 +
 doc/en/motor_control/index.rst                |  1 +
 doc/en/motor_control/motor_controller.rst     | 31 +++++++++++++++++++
 11 files changed, 73 insertions(+), 3 deletions(-)
 create mode 100644 components/motor_controller/CMakeLists.txt
 create mode 100644 components/motor_controller/idf_component.yml
 rename components/{base_component => motor_controller}/include/motor_controller.hpp (87%)
 create mode 100644 doc/en/motor_control/motor_controller.rst

diff --git a/components/basicmicro/CMakeLists.txt b/components/basicmicro/CMakeLists.txt
index 6e5a3f5d41..27c31bc3e3 100644
--- a/components/basicmicro/CMakeLists.txt
+++ b/components/basicmicro/CMakeLists.txt
@@ -3,5 +3,5 @@
 # as `#include "detail/basicmicro_core.hpp"` by consumers and by the host test.
 idf_component_register(
   INCLUDE_DIRS "include"
-  REQUIRES base_component
+  REQUIRES base_component motor_controller
 )
diff --git a/components/basicmicro/idf_component.yml b/components/basicmicro/idf_component.yml
index 912db0858e..52f3b24963 100644
--- a/components/basicmicro/idf_component.yml
+++ b/components/basicmicro/idf_component.yml
@@ -23,3 +23,4 @@ dependencies:
   idf:
     version: '>=5.0'
   espp/base_component: '>=1.0'
+  espp/motor_controller: '>=1.0'
diff --git a/components/mcp266/CMakeLists.txt b/components/mcp266/CMakeLists.txt
index 84a9ba6d0a..0f9be45db0 100644
--- a/components/mcp266/CMakeLists.txt
+++ b/components/mcp266/CMakeLists.txt
@@ -3,5 +3,5 @@
 # `#include "detail/mcp266_core.hpp"` resolve for consumers.
 idf_component_register(
   INCLUDE_DIRS "include"
-  REQUIRES base_component canopen
+  REQUIRES base_component canopen motor_controller
 )
diff --git a/components/mcp266/example/main/mcp266_example.cpp b/components/mcp266/example/main/mcp266_example.cpp
index 52c29a8420..62431d6cf7 100644
--- a/components/mcp266/example/main/mcp266_example.cpp
+++ b/components/mcp266/example/main/mcp266_example.cpp
@@ -102,7 +102,10 @@ extern "C" void app_main(void) {
     logger.error("Failed to configure M1 position loop: {}", ec.message());
     return;
   }
-  mcp.set_software_position_limits(Axis::M1, -20'000, 20'000, ec);
+  if (!mcp.set_software_position_limits(Axis::M1, -20'000, 20'000, ec)) {
+    logger.error("Failed to set M1 software position limits: {}", ec.message());
+    return;
+  }
 
   // Run a small profile-position sequence and report arrival.
   static constexpr int32_t targets[] = {10'000, -10'000, 0};
diff --git a/components/mcp266/idf_component.yml b/components/mcp266/idf_component.yml
index cf97168635..272a6c0338 100644
--- a/components/mcp266/idf_component.yml
+++ b/components/mcp266/idf_component.yml
@@ -24,3 +24,4 @@ dependencies:
     version: '>=5.0'
   espp/base_component: '>=1.0'
   espp/canopen: '>=1.0'
+  espp/motor_controller: '>=1.0'
diff --git a/components/motor_controller/CMakeLists.txt b/components/motor_controller/CMakeLists.txt
new file mode 100644
index 0000000000..8151b0f1c4
--- /dev/null
+++ b/components/motor_controller/CMakeLists.txt
@@ -0,0 +1,4 @@
+idf_component_register(
+  INCLUDE_DIRS "include"
+  REQUIRES format
+)
diff --git a/components/motor_controller/idf_component.yml b/components/motor_controller/idf_component.yml
new file mode 100644
index 0000000000..1a744b39ad
--- /dev/null
+++ b/components/motor_controller/idf_component.yml
@@ -0,0 +1,19 @@
+## IDF Component Manager Manifest File
+license: "MIT"
+description: "Shared motor-controller interface for espp: the MotorAxis channel selector and the MotorController concept satisfied by the Basicmicro (serial) and Mcp266 (CANopen) drivers"
+url: "https://github.com/esp-cpp/espp/tree/main/components/motor_controller"
+repository: "https://github.com/esp-cpp/espp.git"
+maintainers:
+  - William Emfinger <waemfinger@gmail.com>
+documentation: "https://esp-cpp.github.io/espp/motor_control/motor_controller.html"
+tags:
+  - cpp
+  - Component
+  - Motor
+  - Control
+  - Interface
+  - Concept
+dependencies:
+  idf:
+    version: '>=5.0'
+  espp/format: '>=1.0'
diff --git a/components/base_component/include/motor_controller.hpp b/components/motor_controller/include/motor_controller.hpp
similarity index 87%
rename from components/base_component/include/motor_controller.hpp
rename to components/motor_controller/include/motor_controller.hpp
index 2c34cf43a4..6da89558f1 100644
--- a/components/base_component/include/motor_controller.hpp
+++ b/components/motor_controller/include/motor_controller.hpp
@@ -4,6 +4,8 @@
 #include <cstdint>
 #include <system_error>
 
+#include "format.hpp"
+
 namespace espp {
 
 /// \brief Motor channel selector shared by the espp motor-controller drivers.
@@ -48,3 +50,10 @@ concept MotorController = requires(T t, MotorAxis axis, int16_t duty, int32_t qp
 };
 
 } // namespace espp
+
+// @brief fmt formatter for espp::MotorAxis (prints "M1" / "M2").
+template <> struct fmt::formatter<espp::MotorAxis> : fmt::formatter<std::string_view> {
+  template <typename FormatContext> auto format(espp::MotorAxis axis, FormatContext &ctx) const {
+    return fmt::formatter<std::string_view>::format(axis == espp::MotorAxis::M1 ? "M1" : "M2", ctx);
+  }
+};
diff --git a/doc/Doxyfile b/doc/Doxyfile
index 81de57f18c..41cfb110ee 100755
--- a/doc/Doxyfile
+++ b/doc/Doxyfile
@@ -227,6 +227,7 @@ INPUT = \
   $(PROJECT_PATH)/components/aw9523/include/aw9523.hpp \
   $(PROJECT_PATH)/components/base_component/include/base_component.hpp \
   $(PROJECT_PATH)/components/base_peripheral/include/base_peripheral.hpp \
+  $(PROJECT_PATH)/components/motor_controller/include/motor_controller.hpp \
   $(PROJECT_PATH)/components/basicmicro/include/basicmicro.hpp \
   $(PROJECT_PATH)/components/basicmicro/include/detail/basicmicro_core.hpp \
   $(PROJECT_PATH)/components/mcp266/include/mcp266.hpp \
diff --git a/doc/en/motor_control/index.rst b/doc/en/motor_control/index.rst
index 209068d8bc..1937c23016 100644
--- a/doc/en/motor_control/index.rst
+++ b/doc/en/motor_control/index.rst
@@ -10,6 +10,7 @@ Motor-control algorithms and controller interfaces. See also the
 
    pid
    adrc
+   motor_controller
    basicmicro
    mcp266
    odrive_ascii
diff --git a/doc/en/motor_control/motor_controller.rst b/doc/en/motor_control/motor_controller.rst
new file mode 100644
index 0000000000..2f2e3edf37
--- /dev/null
+++ b/doc/en/motor_control/motor_controller.rst
@@ -0,0 +1,31 @@
+Motor Controller Interface
+**************************
+
+The `motor_controller` component holds the shared interface used by the
+dual-channel motor-controller drivers that model the same MCP236 / MCP266
+hardware over different transports:
+
+- :cpp:enum:`espp::MotorAxis`, the ``M1`` / ``M2`` channel selector, and
+- the :cpp:concept:`espp::MotorController` concept — the common command / read
+  surface (``drive_duty`` / ``drive_speed`` by axis, ``read_encoder`` /
+  ``read_speed``, ``reset_estop``, ``read_main_battery_voltage``,
+  ``read_temperature``).
+
+Keeping the contract in a small, dependency-light component lets the two drivers
+share it without depending on each other: :doc:`basicmicro` (packet serial) and
+:doc:`mcp266` (CANopen) each ``static_assert`` that they satisfy
+``MotorController``, so generic code can command either transport by axis.
+
+.. code-block:: cpp
+
+   // works for either espp::Basicmicro or espp::Mcp266
+   bool ramp(espp::MotorController auto &mc, std::error_code &ec) {
+     return mc.drive_duty(espp::MotorAxis::M1, 4096, ec);
+   }
+
+.. ---------------------------- API Reference ----------------------------------
+
+API Reference
+-------------
+
+.. include-build-file:: inc/motor_controller.inc

From 859461e58190601a4d001b2a5b25bc41859e25cc Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Thu, 3 Sep 2026 11:27:19 -0500
Subject: [PATCH 4/7] fix(basicmicro): signed read_speed; register + doc
 ordering (review)

Address the #764 review round:
- read_speed(Axis, qpps, ec): commands 18/19 report an unsigned magnitude plus
  a separate direction byte, so the no-direction overload returned reverse
  motion as positive -- violating the MotorController signed-speed contract.
  Fold the direction into the sign (negate when backward). The direction-byte
  overload still exposes the raw magnitude + flag.
- upload_components.yml: list components/motor_controller ahead of its first-time
  dependents basicmicro / mcp266 so the registry can resolve it (per the
  documented dependency-order rule).
- Doxyfile: move the motor_controller.hpp INPUT entry to its alphabetical spot
  (after monitor), per the file-order requirement.

Builds clean: basicmicro example on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 .github/workflows/upload_components.yml      |  1 +
 components/basicmicro/include/basicmicro.hpp | 14 +++++++++++---
 doc/Doxyfile                                 |  2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/.github/workflows/upload_components.yml b/.github/workflows/upload_components.yml
index 50ddb165f2..68aabe86ca 100755
--- a/.github/workflows/upload_components.yml
+++ b/.github/workflows/upload_components.yml
@@ -55,6 +55,7 @@ jobs:
             components/aw9523
             components/base_component
             components/base_peripheral
+            components/motor_controller
             components/basicmicro
             components/mcp266
             components/bdc_driver
diff --git a/components/basicmicro/include/basicmicro.hpp b/components/basicmicro/include/basicmicro.hpp
index 6a69b1f141..73df24196d 100644
--- a/components/basicmicro/include/basicmicro.hpp
+++ b/components/basicmicro/include/basicmicro.hpp
@@ -413,12 +413,20 @@ class Basicmicro : public BaseComponent {
    * @param qpps Speed in pulses per second (as reported by the controller).
    * @param ec Set on failure.
    * @return True on success.
-   * @note Use the overload taking a @c direction out-parameter to also read the
-   *       0 = forward / 1 = backward flag.
+   * @note Commands 18/19 report an unsigned speed magnitude plus a separate
+   *       direction byte; this overload folds that direction into the sign so
+   *       reverse motion reads as a negative speed (the signed-speed contract of
+   *       espp::MotorController). Use the overload taking a @c direction
+   *       out-parameter to read the raw magnitude and the 0 = forward /
+   *       1 = backward flag separately.
    */
   bool read_speed(Axis axis, int32_t &qpps, std::error_code &ec) {
     uint8_t direction = 0;
-    return read_speed(axis, qpps, direction, ec);
+    if (!read_speed(axis, qpps, direction, ec))
+      return false;
+    if (direction != 0)
+      qpps = -qpps;
+    return true;
   }
 
   /**
diff --git a/doc/Doxyfile b/doc/Doxyfile
index 41cfb110ee..5ac85abd7d 100755
--- a/doc/Doxyfile
+++ b/doc/Doxyfile
@@ -227,7 +227,6 @@ INPUT = \
   $(PROJECT_PATH)/components/aw9523/include/aw9523.hpp \
   $(PROJECT_PATH)/components/base_component/include/base_component.hpp \
   $(PROJECT_PATH)/components/base_peripheral/include/base_peripheral.hpp \
-  $(PROJECT_PATH)/components/motor_controller/include/motor_controller.hpp \
   $(PROJECT_PATH)/components/basicmicro/include/basicmicro.hpp \
   $(PROJECT_PATH)/components/basicmicro/include/detail/basicmicro_core.hpp \
   $(PROJECT_PATH)/components/mcp266/include/mcp266.hpp \
@@ -374,6 +373,7 @@ INPUT = \
   $(PROJECT_PATH)/components/max1704x/include/max1704x.hpp \
   $(PROJECT_PATH)/components/monitor/include/heap_monitor.hpp \
   $(PROJECT_PATH)/components/monitor/include/task_monitor.hpp \
+  $(PROJECT_PATH)/components/motor_controller/include/motor_controller.hpp \
   $(PROJECT_PATH)/components/motorgo-axis/include/motorgo-axis.hpp \
   $(PROJECT_PATH)/components/motorgo-mini/include/motorgo-mini.hpp \
   $(PROJECT_PATH)/components/motorgo-plink/include/motorgo-plink.hpp \

From 47c7051c0938a99064a2833cd4fa9c89875dfb0f Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Thu, 3 Sep 2026 14:30:24 -0500
Subject: [PATCH 5/7] fix(mcp266): update webapp example for the Axis-form API

#761 (webapp) merged to main while this branch was in review; now that this
branch rebases onto main, update the webapp for the mcp266 API changes here:
- configure_position_loop: ec is now the last argument (fallback_p precedes it).
- set_position_limits -> set_software_position_limits.
- add the new motor_controller component (a transitive mcp266 dependency) to the
  manager-off component closure (EXTRA_COMPONENT_DIRS + COMPONENTS) so the
  firmware still builds with IDF_COMPONENT_MANAGER=0.

Builds clean: webapp firmware (esp32s3, manager-off) + basicmicro/mcp266
examples on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 components/mcp266/webapp_example/CMakeLists.txt               | 3 ++-
 .../mcp266/webapp_example/main/mcp266_webapp_example.cpp      | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/components/mcp266/webapp_example/CMakeLists.txt b/components/mcp266/webapp_example/CMakeLists.txt
index 77456485f4..ad33d0dcfb 100644
--- a/components/mcp266/webapp_example/CMakeLists.txt
+++ b/components/mcp266/webapp_example/CMakeLists.txt
@@ -26,6 +26,7 @@ set(EXTRA_COMPONENT_DIRS
   "../../../components/format"
   "../../../components/logger"
   "../../../components/mcp266"
+  "../../../components/motor_controller"
   "../../../components/stream_frame"
   "../../../components/task"
   "../../../components/twai"
@@ -45,7 +46,7 @@ endif()
 
 set(
   COMPONENTS
-  "main esptool_py base_component canopen dispatcher format logger mcp266 stream_frame task twai usb_device esp_tinyusb"
+  "main esptool_py base_component canopen dispatcher format logger mcp266 motor_controller stream_frame task twai usb_device esp_tinyusb"
   CACHE STRING
   "List of components to include"
   )
diff --git a/components/mcp266/webapp_example/main/mcp266_webapp_example.cpp b/components/mcp266/webapp_example/main/mcp266_webapp_example.cpp
index 67e6aa5483..7ed53b001e 100644
--- a/components/mcp266/webapp_example/main/mcp266_webapp_example.cpp
+++ b/components/mcp266/webapp_example/main/mcp266_webapp_example.cpp
@@ -275,14 +275,14 @@ extern "C" void app_main(void) {
     case proto::kConfigurePositionLoop:
       if (!need_axis(13))
         break;
-      mcp.configure_position_loop(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), ec, rd_i32(pl, 9))
+      mcp.configure_position_loop(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), rd_i32(pl, 9), ec)
           ? send_ok(type)
           : reply_error(type, ec, "configure position loop failed");
       break;
     case proto::kSetPositionLimits:
       if (!need_axis(9))
         break;
-      mcp.set_position_limits(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), ec)
+      mcp.set_software_position_limits(axis_of(pl[0]), rd_i32(pl, 1), rd_i32(pl, 5), ec)
           ? send_ok(type)
           : reply_error(type, ec, "set position limits failed");
       break;

From c5ddd73bedee176d156173b362eaf5321f9d4bfb Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Thu, 3 Sep 2026 19:43:44 -0500
Subject: [PATCH 6/7] fix(motor-control): validate the axis selector; add
 motor_controller README

Address the latest #764 review:
- Axis validation (basicmicro + mcp266): the Axis-form API dispatches with
  `axis == Axis::M1 ? ... : ...`, so an out-of-range MotorAxis (e.g. one decoded
  from an untrusted byte) silently targeted M2 -- a footgun the old per-axis
  methods could not hit. Each public axis method now calls a check_axis() guard
  that rejects anything but M1/M2 with invalid_argument before any I/O; the
  guard also covers Mcp266::axis_state (every caller validates first).
- Add components/motor_controller/README.md documenting the component.

On the reviewer's example/CI-matrix request: motor_controller is a header-only
interface/types component like bldc_types, which ships with no example or build.yml
matrix entry -- a concept has nothing runnable to demonstrate beyond the drivers
that implement it, which are already exercised by the basicmicro/mcp266 examples in
CI. Added the README (good practice) but intentionally no example, matching the
bldc_types precedent.

Verified: basicmicro + mcp266 examples build clean on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 components/basicmicro/include/basicmicro.hpp | 33 ++++++++++++++++++++
 components/mcp266/include/mcp266.hpp         | 31 ++++++++++++++++++
 components/motor_controller/README.md        | 32 +++++++++++++++++++
 3 files changed, 96 insertions(+)
 create mode 100644 components/motor_controller/README.md

diff --git a/components/basicmicro/include/basicmicro.hpp b/components/basicmicro/include/basicmicro.hpp
index 73df24196d..6422bbb9be 100644
--- a/components/basicmicro/include/basicmicro.hpp
+++ b/components/basicmicro/include/basicmicro.hpp
@@ -124,6 +124,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool drive_duty(Axis axis, int16_t duty, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i16_be(payload, duty);
@@ -157,6 +159,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool drive_speed(Axis axis, int32_t qpps, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps);
@@ -189,6 +193,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool drive_speed_accel(Axis axis, uint32_t accel, int32_t qpps, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
@@ -233,6 +239,8 @@ class Basicmicro : public BaseComponent {
    */
   bool buffered_drive_speed_distance(Axis axis, int32_t qpps, uint32_t distance, bool immediate,
                                      std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_i32_be(payload, qpps);
@@ -282,6 +290,8 @@ class Basicmicro : public BaseComponent {
    */
   bool buffered_drive_speed_accel_distance(Axis axis, uint32_t accel, int32_t qpps,
                                            uint32_t distance, bool immediate, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     std::vector<uint8_t> payload;
     detail::append_u32_be(payload, accel);
@@ -370,6 +380,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool read_encoder(Axis axis, int32_t &count, uint8_t &status, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     uint32_t raw = 0;
     if (!read_count_raw(axis == Axis::M1 ? Command::ReadEncoderM1 : Command::ReadEncoderM2, raw,
@@ -438,6 +450,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool read_speed(Axis axis, int32_t &qpps, uint8_t &direction, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     return read_speed_raw(axis == Axis::M1 ? Command::ReadEncoderSpeedM1
                                            : Command::ReadEncoderSpeedM2,
@@ -497,6 +511,8 @@ class Basicmicro : public BaseComponent {
    * @return True on success.
    */
   bool set_velocity_pid(Axis axis, float p, float i, float d, uint32_t qpps, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     return set_velocity_pid_raw(axis == Axis::M1 ? Command::SetVelocityPidM1
                                                  : Command::SetVelocityPidM2,
@@ -516,6 +532,8 @@ class Basicmicro : public BaseComponent {
    */
   bool read_velocity_pid(Axis axis, float &p, float &i, float &d, uint32_t &qpps,
                          std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     return read_velocity_pid_raw(axis == Axis::M1 ? Command::ReadVelocityPidM1
                                                   : Command::ReadVelocityPidM2,
@@ -545,6 +563,8 @@ class Basicmicro : public BaseComponent {
    */
   bool set_position_pid(Axis axis, float p, float i, float d, uint32_t max_i, uint32_t deadzone,
                         int32_t min_pos, int32_t max_pos, std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     return set_position_pid_raw(axis == Axis::M1 ? Command::SetPositionPidM1
                                                  : Command::SetPositionPidM2,
@@ -569,6 +589,8 @@ class Basicmicro : public BaseComponent {
   bool read_position_pid(Axis axis, float &p, float &i, float &d, uint32_t &max_i,
                          uint32_t &deadzone, int32_t &min_pos, int32_t &max_pos,
                          std::error_code &ec) {
+    if (!check_axis(axis, ec))
+      return false;
     std::scoped_lock lk(mutex_);
     return read_position_pid_raw(axis == Axis::M1 ? Command::ReadPositionPidM1
                                                   : Command::ReadPositionPidM2,
@@ -787,6 +809,17 @@ class Basicmicro : public BaseComponent {
   }
 
 protected:
+  /// Reject an out-of-range axis selector before any I/O. MotorAxis is a
+  /// two-value enum, but a value decoded from an untrusted byte could be neither
+  /// M1 nor M2, and the `axis == Axis::M1 ? ... : ...` dispatch would otherwise
+  /// silently target M2. Sets ec = invalid_argument and returns false.
+  static bool check_axis(Axis axis, std::error_code &ec) {
+    if (axis == Axis::M1 || axis == Axis::M2)
+      return true;
+    ec = std::make_error_code(std::errc::invalid_argument);
+    return false;
+  }
+
   /// Validate that both transport functions were configured. Calling an empty
   /// std::function throws std::bad_function_call, which would violate this
   /// component's no-exceptions contract -- so every transaction entry point
diff --git a/components/mcp266/include/mcp266.hpp b/components/mcp266/include/mcp266.hpp
index dcde434130..c777b93dc6 100644
--- a/components/mcp266/include/mcp266.hpp
+++ b/components/mcp266/include/mcp266.hpp
@@ -153,6 +153,8 @@ class Mcp266 : public BaseComponent {
   bool configure_position_loop(Axis axis, int32_t min_pos, int32_t max_pos, int32_t fallback_p,
                                std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     AxisState &a = axis_state(axis);
     std::array<int32_t, 7> readback{};
     for (uint8_t sub = 1; sub <= 7; ++sub) {
@@ -212,6 +214,8 @@ class Mcp266 : public BaseComponent {
   bool set_software_position_limits(Axis axis, int32_t min_pos, int32_t max_pos,
                                     std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     if (min_pos > max_pos) {
       ec = std::make_error_code(std::errc::invalid_argument);
       return false;
@@ -234,6 +238,8 @@ class Mcp266 : public BaseComponent {
                         uint32_t profile_acceleration, uint32_t profile_deceleration,
                         std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     AxisState &a = axis_state(axis);
     if (!enable(a, Ds402Drive::OperatingMode::ProfilePosition, ec)) {
       return false;
@@ -256,6 +262,8 @@ class Mcp266 : public BaseComponent {
   /// \brief Closed-loop speed via the mirrored packet-serial command (35/36).
   bool drive_speed(Axis axis, int32_t qpps, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     AxisState &a = axis_state(axis);
     if (qpps != 0 && !enable(a, Ds402Drive::OperatingMode::ProfileVelocity, ec)) {
       return false;
@@ -266,6 +274,8 @@ class Mcp266 : public BaseComponent {
   /// \brief Open-loop duty via the mirrored packet-serial command (32/33).
   bool drive_duty(Axis axis, int16_t duty, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     AxisState &a = axis_state(axis);
     if (duty != 0 && !enable(a, Ds402Drive::OperatingMode::ProfileVelocity, ec)) {
       return false;
@@ -285,6 +295,8 @@ class Mcp266 : public BaseComponent {
   /// \return True on success.
   bool read_encoder(Axis axis, int32_t &count, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     count = axis_state(axis).drive.get_position_actual(ec);
     return !ec;
   }
@@ -295,6 +307,8 @@ class Mcp266 : public BaseComponent {
   /// \return True on success.
   bool read_speed(Axis axis, int32_t &qpps, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     qpps = axis_state(axis).drive.get_velocity_actual(ec);
     return !ec;
   }
@@ -305,6 +319,8 @@ class Mcp266 : public BaseComponent {
   /// \return True on success.
   bool read_statusword(Axis axis, uint16_t &statusword, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     statusword = axis_state(axis).drive.get_statusword(ec);
     return !ec;
   }
@@ -315,6 +331,8 @@ class Mcp266 : public BaseComponent {
   /// \return True on success.
   bool get_state(Axis axis, Ds402Drive::State &state, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     state = axis_state(axis).drive.get_state(ec);
     return !ec;
   }
@@ -326,6 +344,8 @@ class Mcp266 : public BaseComponent {
   /// \return True on success.
   bool is_target_reached(Axis axis, bool &reached, std::error_code &ec) {
     ec.clear();
+    if (!check_axis(axis, ec))
+      return false;
     reached = axis_state(axis).drive.is_target_reached(ec);
     return !ec;
   }
@@ -402,6 +422,17 @@ class Mcp266 : public BaseComponent {
         , name(n) {}
   };
 
+  /// Reject an out-of-range axis selector. MotorAxis is a two-value enum, but a
+  /// value decoded from an untrusted byte could be neither M1 nor M2, and
+  /// axis_state()'s dispatch would otherwise silently select M2. Public axis
+  /// methods call this before any SDO I/O. Sets ec = invalid_argument on failure.
+  static bool check_axis(Axis axis, std::error_code &ec) {
+    if (axis == Axis::M1 || axis == Axis::M2)
+      return true;
+    ec = std::make_error_code(std::errc::invalid_argument);
+    return false;
+  }
+
   AxisState &axis_state(Axis axis) { return axis == Axis::M1 ? m1_ : m2_; }
 
   /// Write the axis mode of operation directly (the MCP does not echo the
diff --git a/components/motor_controller/README.md b/components/motor_controller/README.md
new file mode 100644
index 0000000000..8d636b5784
--- /dev/null
+++ b/components/motor_controller/README.md
@@ -0,0 +1,32 @@
+# Motor Controller Component
+
+[![Badge](https://components.espressif.com/components/espp/motor_controller/badge.svg)](https://components.espressif.com/components/espp/motor_controller)
+
+Header-only shared interface for the espp dual-channel motor-controller drivers
+that model the same MCP236 / MCP266 (Basicmicro / RoboClaw-family) hardware over
+different transports. It defines:
+
+- `espp::MotorAxis` — the `M1` / `M2` channel selector, and
+- `espp::MotorController` — a compile-time concept describing the common command
+  and read surface (`drive_duty` / `drive_speed` by axis, `read_encoder` /
+  `read_speed`, `reset_estop`, `read_main_battery_voltage`, `read_temperature`).
+
+Keeping the contract in a small, dependency-light component lets the two sibling
+drivers share it without depending on each other: `espp::Basicmicro` (packet
+serial) and `espp::Mcp266` (CANopen) each `static_assert` that they satisfy
+`MotorController`, so generic code can command either transport by axis.
+
+This is an interface / types component (like `bldc_types`): it has no runnable
+example of its own — it is exercised by the `basicmicro` and `mcp266` examples,
+which build in CI and instantiate the drivers that implement the concept.
+
+## Example
+
+```cpp
+#include "motor_controller.hpp"
+
+// Works for either espp::Basicmicro or espp::Mcp266 (both satisfy the concept).
+bool ramp(espp::MotorController auto &mc, std::error_code &ec) {
+  return mc.drive_duty(espp::MotorAxis::M1, 4096, ec);
+}
+```

From 331dbf3f82d651d77f88d35208c68cc4d465e559 Mon Sep 17 00:00:00 2001
From: William Emfinger <waemfinger@gmail.com>
Date: Thu, 3 Sep 2026 21:54:54 -0500
Subject: [PATCH 7/7] fix(motor-control): close the last two axis-safety gaps
 (review follow-up)

- MotorAxis fmt formatter: format an out-of-range value as "UNKNOWN" (via an
  explicit switch) instead of "M2" -- the ternary made diagnostics mislabel
  malformed input as a valid channel.
- Mcp266::drive(Axis): the public advanced-access accessor still routed an
  invalid selector through axis_state() to the M2 drive. It now returns a
  Ds402Drive* and yields nullptr for anything but M1/M2, so a decoded-byte axis
  cannot silently command the wrong drive. (No callers in-tree; pre-1.0.)

Verified: MotorAxis formatter host test prints M1/M2/UNKNOWN and compiles clean
under -Wall -Wextra; mcp266 example builds on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 components/mcp266/include/mcp266.hpp             | 10 ++++++++--
 .../include/motor_controller.hpp                 | 16 ++++++++++++++--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/components/mcp266/include/mcp266.hpp b/components/mcp266/include/mcp266.hpp
index c777b93dc6..c246e89374 100644
--- a/components/mcp266/include/mcp266.hpp
+++ b/components/mcp266/include/mcp266.hpp
@@ -389,8 +389,14 @@ class Mcp266 : public BaseComponent {
 
   /// \brief Access an axis's underlying Ds402Drive for advanced CiA 402 use.
   /// \param axis The motor channel.
-  /// \return Reference to the axis drive helper.
-  Ds402Drive &drive(Axis axis) { return axis_state(axis).drive; }
+  /// \return Pointer to the axis drive helper, or nullptr if \p axis is not a
+  ///         valid channel (M1 / M2) -- so an axis decoded from an untrusted
+  ///         byte cannot silently return the M2 drive.
+  Ds402Drive *drive(Axis axis) {
+    if (axis != Axis::M1 && axis != Axis::M2)
+      return nullptr;
+    return &axis_state(axis).drive;
+  }
 
 private:
   /// Coarse fallback position P gain, used only when the drive's stored gain
diff --git a/components/motor_controller/include/motor_controller.hpp b/components/motor_controller/include/motor_controller.hpp
index 6da89558f1..162afc8f56 100644
--- a/components/motor_controller/include/motor_controller.hpp
+++ b/components/motor_controller/include/motor_controller.hpp
@@ -2,6 +2,7 @@
 
 #include <concepts>
 #include <cstdint>
+#include <string_view>
 #include <system_error>
 
 #include "format.hpp"
@@ -51,9 +52,20 @@ concept MotorController = requires(T t, MotorAxis axis, int16_t duty, int32_t qp
 
 } // namespace espp
 
-// @brief fmt formatter for espp::MotorAxis (prints "M1" / "M2").
+// @brief fmt formatter for espp::MotorAxis (prints "M1" / "M2", or "UNKNOWN" for
+// an out-of-range value so diagnostics never mislabel malformed input as a valid
+// channel).
 template <> struct fmt::formatter<espp::MotorAxis> : fmt::formatter<std::string_view> {
   template <typename FormatContext> auto format(espp::MotorAxis axis, FormatContext &ctx) const {
-    return fmt::formatter<std::string_view>::format(axis == espp::MotorAxis::M1 ? "M1" : "M2", ctx);
+    std::string_view name = "UNKNOWN";
+    switch (axis) {
+    case espp::MotorAxis::M1:
+      name = "M1";
+      break;
+    case espp::MotorAxis::M2:
+      name = "M2";
+      break;
+    }
+    return fmt::formatter<std::string_view>::format(name, ctx);
   }
 };