refactor(motor-control): clarity pass - public helpers + named constants (no breaking changes) - #763
Merged
Merged
Conversation
… API change) From the basicmicro/canopen/mcp266 design review. All fixes preserve the public API and the "true => ec cleared" contract. - basicmicro set_velocity_pid: route the P/I/D gains through scale_pid_gain() (rounds; guards negative -> uint32 wrap and NaN/inf -> UB in std::llround) like the position path already does — a raw static_cast of the float*scale product bypassed those guards on the more commonly tuned loop. - basicmicro read_status: the 32-bit fast path returned true without clearing the caller's ec (it used a local ec32), so a caller reusing one std::error_code saw success reported as a stale error. Clear ec before returning. - canopen sdo_upload: a conformant server may leave the SDO size-indicated bit clear on an expedited upload (all 4 data bytes valid, core reports len == 4). The exact-width check then failed read_u8/read_u16 with a spurious protocol_error. When the size is NOT indicated, let the caller's requested width govern and take the low N bytes; keep the strict check when a size IS indicated (a genuine truncation/oversize is still rejected). - canopen last_abort_code(): reset the cached abort code at the start of every transaction so it cannot report a stale code from a much earlier failure. - canopen node_id: validate to 1-127 in the constructor (0 = broadcast/ unconfigured would break 0x580/0x600 addressing); clamp to 1 with a loud error. Verified: basicmicro + canopen host tests pass (cores unchanged); basicmicro and canopen examples build clean on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…amed constants From the design review (§01B / §02 / §03). Additive and internal only — no existing signature changes (the breaking name/arity alignment is the separate consistency pass). - Surface the helpers the example/users previously had to reach into detail:: for: Ds402Drive::to_string(State) and state_from_statusword(u16); CanopenClient:: abort_code_to_string(u32). The canopen example now uses Ds402Drive::to_string instead of espp::detail::ds402::state_to_string. - Mcp266: add get_state(Axis, State&, ec) and is_target_reached(Axis, bool&, ec) (forwarding to the axis Ds402Drive), so a motor-control user gets arrival/state without decoding the raw statusword themselves. - Mcp266: replace hardcoded CiA 402 indices with the canonical constants + apply_axis_offset() (which validates the offset): 0x6060 -> OBJ_MODES_OF_OPERATION, 0x607D -> new OBJ_SOFTWARE_POSITION_LIMIT in canopen_core. Express kDefaultPositionP in decimal (15491, ~15.1 x1024) and name the 25 ms mode-settle delay (kModeSettle). Verified: canopen + mcp266 host tests pass; canopen + mcp266 examples build clean on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR improves clarity and usability by promoting previously-detail:: helper functionality to public APIs and replacing magic numbers with named constants, while keeping existing public signatures intact.
Changes:
- Added public helpers for CiA-402 state decoding/stringification and CiA-301 SDO abort code stringification.
- Added MCP266 convenience accessors for decoded drive state and “target reached”.
- Replaced raw object-index literals and a hardcoded delay with named constants.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| components/mcp266/include/mcp266.hpp | Uses named OD constants + axis offset helper; adds state/arrival convenience methods; names the mode-settle delay and clarifies default P gain. |
| components/canopen/include/ds402.hpp | Exposes public to_string(State) and state_from_statusword(u16) helpers. |
| components/canopen/include/detail/canopen_core.hpp | Introduces a named constant for software position limits object index (0x607D). |
| components/canopen/include/canopen_client.hpp | Exposes public abort_code_to_string(u32) helper. |
| components/canopen/example/main/canopen_example.cpp | Updates example to use the new public DS402 to_string helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Doc/readability follow-ups from the #763 review (no code behavior change): - mcp266.hpp: split the combined `\param`/`\return` doc lines onto one tag per `///` line throughout — Doxygen only parses the first tag on a line, so the compact form dropped every parameter after the first from the generated docs. - canopen_core.hpp: spell out the OBJ_SOFTWARE_POSITION_LIMIT comment as "i32; subindex 1 = min, 2 = max" instead of the cryptic "i32:1/:2". - ds402.hpp: document that Ds402Drive::to_string() returns a static-lifetime string literal (non-owning, never freed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per review, add fmt::formatter specializations so application code can print the
CANopen / DS402 enums directly -- e.g. logger.info("Drive state: {}", state) --
instead of calling a to-string helper inline at every call site, matching the
formatter pattern used elsewhere in espp (e.g. wifi_format_helpers.hpp).
New canopen_format_helpers.hpp covers Ds402Drive::State (reuses state_to_string),
Ds402Drive::OperatingMode, CanopenClient::NmtState, and CanopenClient::NmtCommand.
It is included from canopen_client.hpp, so ds402.hpp / mcp266 get it transitively.
Ds402Drive::to_string() is retained for non-fmt callers. The canopen example now
prints the drive state and heartbeat NMT state via {}.
Builds clean: canopen example on IDF v6.0.1.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
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.
Second in the review follow-up stack (bugs → clarity → consistency), on top of #762. Additive and internal only — no existing signature changes (the breaking name/arity/encoder-sign alignment is the separate consistency PR).
From the review §01B (
detail::leaks), §02, §03:detail::for:Ds402Drive::to_string(State)andDs402Drive::state_from_statusword(u16)CanopenClient::abort_code_to_string(u32)Ds402Drive::to_string(...)instead ofespp::detail::ds402::state_to_string(...).get_state(Axis, State&, ec)andis_target_reached(Axis, bool&, ec)forwarding to the axisDs402Drive.0x6060 → OBJ_MODES_OF_OPERATION,0x607D → OBJ_SOFTWARE_POSITION_LIMIT(new incanopen_core), both viaapply_axis_offset()(which validates the offset).kDefaultPositionPwritten in decimal (15491 ≈ 15.1 ×1024) and the 25 ms mode-settle delay namedkModeSettle.Verified
Deliberately deferred to the consistency PR (all breaking):
ec-position inconfigure_position_loop,set_position_limits→set_software_position_limitsrename,reset_estop↔e_stop_reset, thedrive_duty/drive_speedarity collision, and encoder-sign alignment.🤖 Generated with Claude Code