Skip to content

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

Merged
finger563 merged 1 commit into
mainfrom
refactor/mcp-shared-command-set
Sep 4, 2026
Merged

refactor(motor-control): single source of truth for the MCP command numbers#766
finger563 merged 1 commit into
mainfrom
refactor/mcp-shared-command-set

Conversation

@finger563

Copy link
Copy Markdown
Contributor

Single source of truth for the MCP236/266 command numbers

Stacked on #764 (needs the motor_controller component). Rebases onto main once #764 merges.

Follow-up to the design discussion on whether basicmicro (packet serial) and mcp266
(CANopen) could be unified: the right answer is the MotorController concept for the
API, not a merged component — but the one thing genuinely worth de-duplicating is the
command-number table, and this PR does that.

The drift risk

The MCP266's CANopen firmware mirrors the Basicmicro packet-serial command set into its
manufacturer object dictionary at 0x2000 + command. So "read main battery voltage" is
command 24 on serial and object 0x2018 on CAN — the same 24. But mcp266_core.hpp
hardcoded those numbers as magic values (command_object(24), (82), (200),
(61/62/63/64), (32/33/35/36)) that had to be hand-kept in lockstep with
basicmicro's BasicmicroCommand enum. Nothing stopped them drifting.

The change

  • Move the BasicmicroCommand table into the shared motor_controller component
    (basicmicro_commands.hpp). Both drivers already depend on that component, so this adds
    no dependency between the two sibling drivers and no new component.
  • basicmicro_core.hpp includes it (keeps its packet codecs / CRC / BasicmicroStatus
    those are transport-specific and not shared).
  • mcp266_core.hpp derives every manufacturer object from the named enum via a new
    command_object(BasicmicroCommand) overload, e.g.
    command_object(BasicmicroCommand::ReadMainBatteryVoltage) instead of command_object(24).
    The numbers now live in exactly one place.

No behavior change

Pure relocation + indirection — no object address or command byte changes. Both
host-buildable cores test-verify the same concrete values (the mcp266 test still asserts
0x2018 etc.).

Verified

  • basicmicro + mcp266 host tests pass (rebuilt with the added
    -I../../motor_controller/include for the shared header; their build instructions are
    updated accordingly — these are dev tests, not CI-gated).
  • Both examples build clean on IDF v6.0.1.

Note: this is the DRY win, not a merge. The transports stay separate components (different
wire protocols, byte order, and control models — CiA 402 state machine vs direct commands);
they share the interface (MotorController) and now the command-number table, which is
all they genuinely have in common.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 3, 2026 22:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is a low-risk refactor that centralizes command constants without altering computed indices/values, and the dependency/include updates appear consistent.

Pull request overview

This PR refactors the MCP236/266 motor-control stack to make the Basicmicro/MCP command-number mapping a single source of truth, shared across the packet-serial (basicmicro) and CANopen (mcp266) transports via the motor_controller component.

Changes:

  • Introduces a shared espp::detail::BasicmicroCommand enum in motor_controller and updates both transport cores to include it.
  • Updates mcp266 to derive manufacturer object indices from named commands (command_object(BasicmicroCommand::...)) instead of hardcoded magic numbers.
  • Updates docs and host-test build instructions to reflect the shared header location.
File summaries
File Description
doc/en/motor_control/motor_controller.rst Documents the shared BasicmicroCommand table and why mcp266 derives object indices from it.
doc/Doxyfile Adds the new shared header to Doxygen INPUT so it appears in generated docs.
components/motor_controller/include/basicmicro_commands.hpp New shared command-number enum (BasicmicroCommand) serving as the single source of truth.
components/mcp266/test/mcp266_host_test.cpp Updates host-build instructions to include the motor_controller include path.
components/mcp266/include/detail/mcp266_core.hpp Switches manufacturer-object constants to use BasicmicroCommand and adds an overload command_object(BasicmicroCommand).
components/basicmicro/test/basicmicro_host_test.cpp Updates host-build instructions to include the motor_controller include path.
components/basicmicro/include/detail/basicmicro_core.hpp Removes the local BasicmicroCommand definition in favor of the shared header.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Base automatically changed from refactor/motor-control-consistency to main September 4, 2026 03:48
…umbers

The mcp266 CANopen driver mirrors the Basicmicro packet-serial command set into
its manufacturer object dictionary at 0x2000 + command number, but it hardcoded
those command numbers (24, 82, 200, 61-64, 32/33/35/36) as magic values that had
to be kept in lockstep with basicmicro's BasicmicroCommand enum by hand.

Move the BasicmicroCommand table into the shared motor_controller component
(basicmicro_commands.hpp) -- which both drivers already depend on, so neither
sibling depends on the other -- and derive the manufacturer object indices from
the named enum values via a command_object(BasicmicroCommand) overload. The
numbers now live in exactly one place and cannot drift.

No value changes: both host cores test-verify the same concrete addresses
(e.g. ReadMainBatteryVoltage -> 0x2018). basicmicro_core keeps its packet codecs
/ CRC / BasicmicroStatus (transport-specific, not shared).

Verified: basicmicro + mcp266 host tests pass; both examples build on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@finger563
finger563 force-pushed the refactor/mcp-shared-command-set branch from aa92cba to a15bf74 Compare September 4, 2026 04:11
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The component README must document the newly shared command table.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +19 to +20
The component also holds :cpp:enum:`espp::detail::BasicmicroCommand`
(``basicmicro_commands.hpp``) — the packet-serial command-number table. Both
@finger563
finger563 merged commit 590be3d into main Sep 4, 2026
157 checks passed
@finger563
finger563 deleted the refactor/mcp-shared-command-set branch September 4, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants