refactor(motor-control): single source of truth for the MCP command numbers

components/basicmicro/include/detail/basicmicro_core.hpp

@@ -36,6 +36,11 @@
 #include <span>
 #include <vector>
 
+// The BasicmicroCommand table lives in the shared motor_controller component so
+// the mcp266 CANopen driver (which mirrors these commands at object 0x2000 + n)
+// can derive its object indices from the same source of truth.
+#include "basicmicro_commands.hpp"
+
 namespace espp {
 namespace detail {
 
@@ -62,81 +67,9 @@ static constexpr float kBasicmicroPidScale = 65536.0f;
 /// loop uses the separate kBasicmicroPidScale above.
 static constexpr float kBasicmicroPositionPidScale = 1024.0f;
 
-/// @brief Packet-serial command bytes.
-///
-/// Every value below was verified against the MCP Series User Manual (sections
-/// 2.2.12, 2.2.13, 2.3.1, 2.4.8 and 2.4.9). Commands whose payload layout is
-/// not clearly documented in the manual are intentionally omitted.
-enum class BasicmicroCommand : uint8_t {
-  // -- Compatibility commands (section 2.2.12), payload: one byte 0-127 --
-  DriveForwardM1 = 0,   ///< 0 = stop, 127 = full forward
-  DriveBackwardsM1 = 1, ///< 0 = stop, 127 = full reverse
-  DriveForwardM2 = 4,   ///< 0 = stop, 127 = full forward
-  DriveBackwardsM2 = 5, ///< 0 = stop, 127 = full reverse
-  DriveM1_7Bit = 6,     ///< 0 = full reverse, 64 = stop, 127 = full forward
-  DriveM2_7Bit = 7,     ///< 0 = full reverse, 64 = stop, 127 = full forward
-  // -- Encoder commands (section 2.4.8) --
-  ReadEncoderM1 = 16,           ///< reply: count (4 bytes), status (1 byte)
-  ReadEncoderM2 = 17,           ///< reply: count (4 bytes), status (1 byte)
-  ReadEncoderSpeedM1 = 18,      ///< reply: pulses/s (4 bytes), direction (1 byte)
-  ReadEncoderSpeedM2 = 19,      ///< reply: pulses/s (4 bytes), direction (1 byte)
-  ResetEncoders = 20,           ///< payload: none (write command, CRC appended)
-  ReadFirmwareVersion = 21,     ///< reply: string terminated by LF + NUL (<=48 bytes)
-  SetEncoderM1 = 22,            ///< payload: value (4 bytes)
-  SetEncoderM2 = 23,            ///< payload: value (4 bytes)
-  ReadMainBatteryVoltage = 24,  ///< reply: tenths of a volt (2 bytes)
-  ReadLogicBatteryVoltage = 25, ///< reply: tenths of a volt (2 bytes)
-  SetVelocityPidM1 = 28,        ///< payload: D, P, I (16.16 fixed), QPPS (4 bytes each)
-  SetVelocityPidM2 = 29,        ///< payload: D, P, I (16.16 fixed), QPPS (4 bytes each)
-  ReadRawSpeedM1 = 30,          ///< reply: counts/s (4 bytes), direction (1 byte)
-  ReadRawSpeedM2 = 31,          ///< reply: counts/s (4 bytes), direction (1 byte)
-  // -- Advanced motor control (section 2.4.9) --
-  DriveM1SignedDuty = 32,              ///< payload: duty (2 bytes, +/-32767)
-  DriveM2SignedDuty = 33,              ///< payload: duty (2 bytes, +/-32767)
-  DriveM1M2SignedDuty = 34,            ///< payload: dutyM1 (2 bytes), dutyM2 (2 bytes)
-  DriveM1SignedSpeed = 35,             ///< payload: speed (4 bytes, qpps)
-  DriveM2SignedSpeed = 36,             ///< payload: speed (4 bytes, qpps)
-  DriveM1M2SignedSpeed = 37,           ///< payload: speedM1 (4 bytes), speedM2 (4 bytes)
-  DriveM1SignedSpeedAccel = 38,        ///< payload: accel (4 bytes), speed (4 bytes)
-  DriveM2SignedSpeedAccel = 39,        ///< payload: accel (4 bytes), speed (4 bytes)
-  DriveM1M2SignedSpeedAccel = 40,      ///< payload: accel, speedM1, speedM2 (4 bytes each)
-  BufferedM1SpeedDistance = 41,        ///< payload: speed, distance (4 bytes each), buffer (1 byte)
-  BufferedM2SpeedDistance = 42,        ///< payload: speed, distance (4 bytes each), buffer (1 byte)
-  BufferedM1M2SpeedDistance = 43,      ///< payload: speedM1, distM1, speedM2, distM2, buffer
-  BufferedM1SpeedAccelDistance = 44,   ///< payload: accel, speed, distance, buffer
-  BufferedM2SpeedAccelDistance = 45,   ///< payload: accel, speed, distance, buffer
-  BufferedM1M2SpeedAccelDistance = 46, ///< payload: accel, speedM1, distM1, speedM2, distM2, buffer
-  ReadBufferLengths = 47,              ///< reply: bufferM1 (1 byte), bufferM2 (1 byte)
-  ReadMotorPWMs = 48,                  ///< reply: pwmM1 (2 bytes), pwmM2 (2 bytes), +/-32767
-  ReadMotorCurrents = 49,       ///< reply: currentM1 (2 bytes), currentM2 (2 bytes), 10 mA units
-  ReadVelocityPidM1 = 55,       ///< reply: P, I, D (16.16 fixed), QPPS (4 bytes each)
-  ReadVelocityPidM2 = 56,       ///< reply: P, I, D (16.16 fixed), QPPS (4 bytes each)
-  SetMainBatteryVoltages = 57,  ///< payload: min (2 bytes), max (2 bytes), tenths of a volt
-  SetLogicBatteryVoltages = 58, ///< payload: min (2 bytes), max (2 bytes), tenths of a volt
-  ReadMainBatteryVoltageSettings = 59,  ///< reply: min (2 bytes), max (2 bytes)
-  ReadLogicBatteryVoltageSettings = 60, ///< reply: min (2 bytes), max (2 bytes)
-  SetPositionPidM1 =
-      61, ///< payload: D, P, I (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
-  SetPositionPidM2 =
-      62, ///< payload: D, P, I (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
-  ReadPositionPidM1 = 63, ///< reply: P, I, D (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
-  ReadPositionPidM2 = 64, ///< reply: P, I, D (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
-  SetM1DefaultDutyAccel = 68, ///< payload: accel (4 bytes)
-  SetM2DefaultDutyAccel = 69, ///< payload: accel (4 bytes)
-  ReadEncoderCounters = 78,   ///< reply: encM1 (4 bytes), encM2 (4 bytes)
-  ReadISpeedCounters = 79,    ///< reply: ispeedM1 (4 bytes), ispeedM2 (4 bytes)
-  RestoreDefaults = 80,       ///< payload: none (write command, CRC appended)
-  ReadDefaultDutyAccels = 81, ///< reply: accelM1 (4 bytes), accelM2 (4 bytes)
-  ReadTemperature = 82,       ///< reply: tenths of a degree (2 bytes)
-  ReadTemperature2 = 83,      ///< reply: tenths of a degree (2 bytes), supported units only
-  // -- Status / configuration (section 2.3.1) --
-  ReadStatus = 90,            ///< reply: status bit mask (see BasicmicroStatus)
-  ReadEncoderModes = 91,      ///< reply: encM1 mode (1 byte), encM2 mode (1 byte)
-  SetEncoderModeM1 = 92,      ///< payload: pin/mode (1 byte)
-  SetEncoderModeM2 = 93,      ///< payload: pin/mode (1 byte)
-  WriteSettingsToEeprom = 94, ///< no payload and no CRC on the request (per manual), ACK reply
-  EStopReset = 200,           ///< payload: none (write command, CRC appended)
-};
+// BasicmicroCommand (the packet-serial command-number table) is defined in
+// basicmicro_commands.hpp, included above -- it is shared with the mcp266
+// CANopen driver so the two cannot drift.
 
 /// @brief Unit status bit masks returned by BasicmicroCommand::ReadStatus
 ///        (manual command 90). Current MCP firmware returns a 32-bit status

components/basicmicro/test/basicmicro_host_test.cpp

@@ -1,6 +1,8 @@
 // Host-buildable unit tests for the Basicmicro (MCP / RoboClaw-family) packet
 // serial wire core. Build & run with:
-//   c++ -std=c++20 -I../include basicmicro_host_test.cpp -o test && ./test
+//   c++ -std=c++20 -I../include -I../../motor_controller/include \
+//       basicmicro_host_test.cpp -o test && ./test
+// (the -I../../motor_controller/include is for the shared BasicmicroCommand table)
 //
 // These tests exercise the helpers in detail/basicmicro_core.hpp directly so
 // they need no ESP-IDF headers. Golden CRC values were computed by executing

components/mcp266/include/detail/mcp266_core.hpp

@@ -3,6 +3,11 @@
 #include <array>
 #include <cstdint>
 
+// The MCP266 manufacturer objects mirror the packet-serial command set, so the
+// object indices are derived from the shared BasicmicroCommand table (kept in
+// the motor_controller component) rather than hardcoded numbers.
+#include "basicmicro_commands.hpp"
+
 /// \file mcp266_core.hpp
 /// \brief Host-buildable, ESP-independent core for the Basicmicro MCP266
 ///        CANopen mapping: the manufacturer command-object mirror, per-axis
@@ -24,6 +29,16 @@ inline constexpr uint16_t command_object(uint8_t command) {
   return static_cast<uint16_t>(0x2000 + command);
 }
 
+/// \brief Manufacturer object index for a named packet-serial command.
+/// \details Overload that takes the shared BasicmicroCommand enum so call sites
+///          read as `command_object(BasicmicroCommand::ReadMainBatteryVoltage)`
+///          instead of a magic number that could drift from the serial driver.
+/// \param command The Basicmicro packet-serial command.
+/// \return The corresponding manufacturer object index (0x2000 + command).
+inline constexpr uint16_t command_object(BasicmicroCommand command) {
+  return command_object(static_cast<uint8_t>(command));
+}
+
 /// \brief Object offset added to a CiA 402 device-profile index (0x60xx) to
 ///        select a motor axis. M1 is at the standard indices; M2 mirrors them
 ///        at +0x800 (e.g. controlword 0x6040 -> 0x6840).
@@ -35,9 +50,12 @@ inline constexpr uint16_t kAxisOffsetM2 = 0x800;
 /// \brief Device-level (non-axis) telemetry / maintenance objects, mirrored
 ///        from their packet-serial commands.
 /// @{
-inline constexpr uint16_t kMainBatteryObject = command_object(24); ///< tenths of a volt (u16)
-inline constexpr uint16_t kTemperatureObject = command_object(82); ///< tenths of a degree C (u16)
-inline constexpr uint16_t kEStopResetObject = command_object(200); ///< write-only
+inline constexpr uint16_t kMainBatteryObject =
+    command_object(BasicmicroCommand::ReadMainBatteryVoltage); ///< tenths of a volt (u16)
+inline constexpr uint16_t kTemperatureObject =
+    command_object(BasicmicroCommand::ReadTemperature); ///< tenths of a degree C (u16)
+inline constexpr uint16_t kEStopResetObject =
+    command_object(BasicmicroCommand::EStopReset); ///< write-only
 /// @}
 
 /// \brief The manufacturer command objects and CiA 402 offset for one axis.
@@ -51,13 +69,17 @@ struct AxisObjects {
 
 /// \brief Objects for motor 1 (the standard axis).
 inline constexpr AxisObjects axis_m1() {
-  return {kAxisOffsetM1, command_object(61), command_object(63), command_object(32),
-          command_object(35)};
+  return {kAxisOffsetM1, command_object(BasicmicroCommand::SetPositionPidM1),
+          command_object(BasicmicroCommand::ReadPositionPidM1),
+          command_object(BasicmicroCommand::DriveM1SignedDuty),
+          command_object(BasicmicroCommand::DriveM1SignedSpeed)};
 }
 /// \brief Objects for motor 2 (mirrored at +0x800 / command n+1).
 inline constexpr AxisObjects axis_m2() {
-  return {kAxisOffsetM2, command_object(62), command_object(64), command_object(33),
-          command_object(36)};
+  return {kAxisOffsetM2, command_object(BasicmicroCommand::SetPositionPidM2),
+          command_object(BasicmicroCommand::ReadPositionPidM2),
+          command_object(BasicmicroCommand::DriveM2SignedDuty),
+          command_object(BasicmicroCommand::DriveM2SignedSpeed)};
 }
 
 /// \brief Remap a position-PID record from the readback order to the setter

components/mcp266/test/mcp266_host_test.cpp

@@ -1,5 +1,7 @@
 // Host-buildable unit tests for the MCP266 CANopen mapping core. Build & run:
-//   c++ -std=c++20 -I../include mcp266_host_test.cpp -o test && ./test
+//   c++ -std=c++20 -I../include -I../../motor_controller/include \
+//       mcp266_host_test.cpp -o test && ./test
+// (the -I../../motor_controller/include is for the shared BasicmicroCommand table)
 //
 // These tests exercise detail/mcp266_core.hpp directly (no ESP-IDF headers).
 // The object addresses were verified against a live MCP266's SDO object

components/motor_controller/include/basicmicro_commands.hpp

@@ -0,0 +1,102 @@
+#pragma once
+
+#include <cstdint>
+
+// Basicmicro (MCP236 / MCP266 / RoboClaw-family) command-number table -- the
+// single source of truth for the packet-serial command bytes.
+//
+// This lives in the shared `motor_controller` component (which both drivers of
+// this hardware family already depend on) because the command NUMBERS are used
+// by BOTH transports and must stay in lockstep:
+//   * espp::Basicmicro (packet serial) sends `[addr][command][data][CRC16]`.
+//   * espp::Mcp266 (CANopen) talks to the same controller, whose firmware
+//     mirrors the packet-serial command set into the manufacturer region of the
+//     object dictionary at index 0x2000 + command number (see
+//     mcp266_core.hpp::command_object). So e.g. "read main battery voltage" is
+//     command 24 on serial and object 0x2018 on CAN -- the SAME 24.
+// Keeping the numbers here lets mcp266 derive its object indices from these
+// named values instead of hardcoding magic numbers that could silently drift
+// from the serial driver. It is a dependency-free header (just an enum), so both
+// host-buildable cores can include it without pulling in anything else.
+
+namespace espp {
+namespace detail {
+
+/// @brief Packet-serial command bytes.
+///
+/// Every value below was verified against the MCP Series User Manual (sections
+/// 2.2.12, 2.2.13, 2.3.1, 2.4.8 and 2.4.9). Commands whose payload layout is
+/// not clearly documented in the manual are intentionally omitted.
+enum class BasicmicroCommand : uint8_t {
+  // -- Compatibility commands (section 2.2.12), payload: one byte 0-127 --
+  DriveForwardM1 = 0,   ///< 0 = stop, 127 = full forward
+  DriveBackwardsM1 = 1, ///< 0 = stop, 127 = full reverse
+  DriveForwardM2 = 4,   ///< 0 = stop, 127 = full forward
+  DriveBackwardsM2 = 5, ///< 0 = stop, 127 = full reverse
+  DriveM1_7Bit = 6,     ///< 0 = full reverse, 64 = stop, 127 = full forward
+  DriveM2_7Bit = 7,     ///< 0 = full reverse, 64 = stop, 127 = full forward
+  // -- Encoder commands (section 2.4.8) --
+  ReadEncoderM1 = 16,           ///< reply: count (4 bytes), status (1 byte)
+  ReadEncoderM2 = 17,           ///< reply: count (4 bytes), status (1 byte)
+  ReadEncoderSpeedM1 = 18,      ///< reply: pulses/s (4 bytes), direction (1 byte)
+  ReadEncoderSpeedM2 = 19,      ///< reply: pulses/s (4 bytes), direction (1 byte)
+  ResetEncoders = 20,           ///< payload: none (write command, CRC appended)
+  ReadFirmwareVersion = 21,     ///< reply: string terminated by LF + NUL (<=48 bytes)
+  SetEncoderM1 = 22,            ///< payload: value (4 bytes)
+  SetEncoderM2 = 23,            ///< payload: value (4 bytes)
+  ReadMainBatteryVoltage = 24,  ///< reply: tenths of a volt (2 bytes)
+  ReadLogicBatteryVoltage = 25, ///< reply: tenths of a volt (2 bytes)
+  SetVelocityPidM1 = 28,        ///< payload: D, P, I (16.16 fixed), QPPS (4 bytes each)
+  SetVelocityPidM2 = 29,        ///< payload: D, P, I (16.16 fixed), QPPS (4 bytes each)
+  ReadRawSpeedM1 = 30,          ///< reply: counts/s (4 bytes), direction (1 byte)
+  ReadRawSpeedM2 = 31,          ///< reply: counts/s (4 bytes), direction (1 byte)
+  // -- Advanced motor control (section 2.4.9) --
+  DriveM1SignedDuty = 32,              ///< payload: duty (2 bytes, +/-32767)
+  DriveM2SignedDuty = 33,              ///< payload: duty (2 bytes, +/-32767)
+  DriveM1M2SignedDuty = 34,            ///< payload: dutyM1 (2 bytes), dutyM2 (2 bytes)
+  DriveM1SignedSpeed = 35,             ///< payload: speed (4 bytes, qpps)
+  DriveM2SignedSpeed = 36,             ///< payload: speed (4 bytes, qpps)
+  DriveM1M2SignedSpeed = 37,           ///< payload: speedM1 (4 bytes), speedM2 (4 bytes)
+  DriveM1SignedSpeedAccel = 38,        ///< payload: accel (4 bytes), speed (4 bytes)
+  DriveM2SignedSpeedAccel = 39,        ///< payload: accel (4 bytes), speed (4 bytes)
+  DriveM1M2SignedSpeedAccel = 40,      ///< payload: accel, speedM1, speedM2 (4 bytes each)
+  BufferedM1SpeedDistance = 41,        ///< payload: speed, distance (4 bytes each), buffer (1 byte)
+  BufferedM2SpeedDistance = 42,        ///< payload: speed, distance (4 bytes each), buffer (1 byte)
+  BufferedM1M2SpeedDistance = 43,      ///< payload: speedM1, distM1, speedM2, distM2, buffer
+  BufferedM1SpeedAccelDistance = 44,   ///< payload: accel, speed, distance, buffer
+  BufferedM2SpeedAccelDistance = 45,   ///< payload: accel, speed, distance, buffer
+  BufferedM1M2SpeedAccelDistance = 46, ///< payload: accel, speedM1, distM1, speedM2, distM2, buffer
+  ReadBufferLengths = 47,              ///< reply: bufferM1 (1 byte), bufferM2 (1 byte)
+  ReadMotorPWMs = 48,                  ///< reply: pwmM1 (2 bytes), pwmM2 (2 bytes), +/-32767
+  ReadMotorCurrents = 49,       ///< reply: currentM1 (2 bytes), currentM2 (2 bytes), 10 mA units
+  ReadVelocityPidM1 = 55,       ///< reply: P, I, D (16.16 fixed), QPPS (4 bytes each)
+  ReadVelocityPidM2 = 56,       ///< reply: P, I, D (16.16 fixed), QPPS (4 bytes each)
+  SetMainBatteryVoltages = 57,  ///< payload: min (2 bytes), max (2 bytes), tenths of a volt
+  SetLogicBatteryVoltages = 58, ///< payload: min (2 bytes), max (2 bytes), tenths of a volt
+  ReadMainBatteryVoltageSettings = 59,  ///< reply: min (2 bytes), max (2 bytes)
+  ReadLogicBatteryVoltageSettings = 60, ///< reply: min (2 bytes), max (2 bytes)
+  SetPositionPidM1 =
+      61, ///< payload: D, P, I (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
+  SetPositionPidM2 =
+      62, ///< payload: D, P, I (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
+  ReadPositionPidM1 = 63, ///< reply: P, I, D (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
+  ReadPositionPidM2 = 64, ///< reply: P, I, D (scaled 1024), MaxI, Deadzone, MinPos, MaxPos (4 each)
+  SetM1DefaultDutyAccel = 68, ///< payload: accel (4 bytes)
+  SetM2DefaultDutyAccel = 69, ///< payload: accel (4 bytes)
+  ReadEncoderCounters = 78,   ///< reply: encM1 (4 bytes), encM2 (4 bytes)
+  ReadISpeedCounters = 79,    ///< reply: ispeedM1 (4 bytes), ispeedM2 (4 bytes)
+  RestoreDefaults = 80,       ///< payload: none (write command, CRC appended)
+  ReadDefaultDutyAccels = 81, ///< reply: accelM1 (4 bytes), accelM2 (4 bytes)
+  ReadTemperature = 82,       ///< reply: tenths of a degree (2 bytes)
+  ReadTemperature2 = 83,      ///< reply: tenths of a degree (2 bytes), supported units only
+  // -- Status / configuration (section 2.3.1) --
+  ReadStatus = 90,            ///< reply: status bit mask (see BasicmicroStatus)
+  ReadEncoderModes = 91,      ///< reply: encM1 mode (1 byte), encM2 mode (1 byte)
+  SetEncoderModeM1 = 92,      ///< payload: pin/mode (1 byte)
+  SetEncoderModeM2 = 93,      ///< payload: pin/mode (1 byte)
+  WriteSettingsToEeprom = 94, ///< no payload and no CRC on the request (per manual), ACK reply
+  EStopReset = 200,           ///< payload: none (write command, CRC appended)
+};
+
+} // namespace detail
+} // namespace espp

doc/Doxyfile

@@ -373,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/basicmicro_commands.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 \

doc/en/motor_control/motor_controller.rst

@@ -16,6 +16,14 @@ 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.
 
+The component also holds :cpp:enum:`espp::detail::BasicmicroCommand`
+(``basicmicro_commands.hpp``) — the packet-serial command-number table. Both
+drivers use it as the single source of truth: the serial driver sends the command
+byte directly, while the MCP266 CANopen firmware mirrors the same command set into
+its manufacturer object dictionary at ``0x2000 + command``, so ``mcp266`` derives
+those object indices from this table rather than from magic numbers that could
+drift out of sync.
+
 .. code-block:: cpp
 
    // works for either espp::Basicmicro or espp::Mcp266