Repository navigation
Conversation
…record Adds to espp::Mcp266, through the same manufacturer command mirror (0x2000 + packet-serial command) the component already uses: - set_encoder(axis, count): command 22/23, so a quadrature encoder can be homed against a limit switch or restored to a remembered count after power-up. - reset_encoders(): command 20, matching espp::Basicmicro::reset_encoders. - read_position_pid(...) / set_position_pid(...): the seven-field record (commands 63/64 and 61/62) with the same float-gain signature as espp::Basicmicro, handling the setter's D, P, I field order. - read_position_limits(axis, min, max): the MinPos/MaxPos clamp alone, two SDOs, as a cheap check that the controller still holds its configuration (it reverts to the factory [0, 0] clamp on a power cycle). configure_position_loop() now uses the shared read/write helpers. The core gains the encoder-set objects, the reset-encoders object and the x1024 gain scale, with host tests; README, docs and the example updated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Position gains are narrowed to signed 32-bit values, incorrectly clamping writes and producing negative readbacks for valid unsigned records.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds MCP266 encoder controls and position-PID read/write support.
Changes:
- Adds encoder set/reset APIs.
- Adds PID readback, limit reading, gain scaling, and setter support.
- Updates tests, documentation, and example logging.
| File | Description |
|---|---|
components/mcp266/include/mcp266.hpp |
Adds public encoder and PID APIs. |
components/mcp266/include/detail/mcp266_core.hpp |
Adds object mappings and gain scaling. |
components/mcp266/test/mcp266_host_test.cpp |
Tests mappings and gain conversion. |
components/mcp266/example/main/mcp266_example.cpp |
Logs configured PID values. |
components/mcp266/example/README.md |
Documents example readback. |
components/mcp266/README.md |
Documents the new APIs. |
doc/en/motor_control/mcp266.rst |
Updates component documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅Static analysis result - no issues found! ✅ |
finger563
approved these changes
Oct 5, 2026
The record's P, I, D, MaxI and Deadzone fields are unsigned on the device; only MinPos / MaxPos are signed. scale_position_gain() now clamps to the full [0, UINT32_MAX] range like Basicmicro's scale_pid_gain(), and read_position_pid() decodes the gain fields from their bit pattern instead of dividing the signed slot, so a gain with bit 31 set no longer reads back negative. The i32 SDO helpers are unchanged: they move a 4-byte pattern either way. Addresses the Copilot review on esp-cpp#842. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
espp::Mcp266wraps the MCP266's manufacturer command mirror for the position PID setter but leaves two neighbours unwrapped: the encoder setter (commands 22/23) and the position PID read-back (63/64). A caller that wants to home a quadrature encoder, or detect that a controller power-cycled back to its factory[0, 0]clamp, currently has to reach pastMcp266to theCanopenClientwith hand-written object indices. This adds:set_encoder(axis, count, ec)— command 22/23. Per-axis;set_encoder(axis, 0, ec)resets one encoder.reset_encoders(ec)— command 20, zeros both, matchingespp::Basicmicro::reset_encoders().read_position_pid(...)/set_position_pid(...)— the seven-field record, same float-gain signature asespp::Basicmicro, with the setter's D, P, I field order handled. Gains are scaled x1024 through a newscale_position_gain()in the host-testable core that rounds to nearest and clamps non-negative, mirroring Basicmicro'sscale_pid_gain().read_position_limits(axis, min, max, ec)— the MinPos/MaxPos clamp alone (two SDOs), as a cheap "did the controller reset" check.configure_position_loop()is refactored onto the same private read/write helpers; no behaviour change.AxisObjectsgainsencoder_set, and the core gainskResetEncodersObjectandkPositionGainScale. README, RST docs and the example are updated (the example now logs the PID record after configuring the loop).Motivation and Context
Needed by a wheelchair-base controller (RAMMP MIB) whose carriage joints use incremental encoders on MCP266s and must be homed against limit switches every boot, and which needs to notice when a controller has power-cycled while the host kept running. Discussed with @finger563, who said a PR adding these would be welcome.
How has this been tested?
components/mcp266/test/mcp266_host_test.cpp), including new checks for the 0x2014/0x2016/0x2017 objects,encoder_seton both axes, the gain scale, andscale_position_gain()rounding/clamping.pre-commit run(clang-format) passes on all changed files.mcp266.hppcompiles cleanly for esp32p4 on ESP-IDF v6.1 in a downstream project viaoverride_path.set_encoderandreset_encodersfollow the same mirror rule as the objects already verified on an MCP266, but I do not currently have access to the hardware. The read-back path (commands 63/64) was exercised on an MCP266 through raw SDOs in the downstream project before this wrapper existed. I will bench-testset_encoderas soon as I have a controller again and report here; happy to hold the merge until then if preferred.Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action. (Not needed: the mcp266 examples are already in the matrix.)🤖 Generated with Claude Code