refactor(motor-control): clarity pass - public helpers + named constants (no breaking changes) - #763
Open
finger563 wants to merge 2 commits into
Open
refactor(motor-control): clarity pass - public helpers + named constants (no breaking changes)#763finger563 wants to merge 2 commits into
finger563 wants to merge 2 commits into
Conversation
…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.
Comment on lines
+288
to
+290
| /// \brief Read the decoded CiA 402 drive state of an axis (from its statusword). | ||
| /// \param axis Channel. \param state Out: the power-drive-system state. | ||
| /// \param ec Set on failure. \return True on success. |
Comment on lines
+296
to
+298
| /// \brief Whether an axis reports "target reached" (statusword bit 10) — the | ||
| /// authoritative arrival signal for profile moves. | ||
| /// \param axis Channel. \param reached Out. \param ec Set on failure. \return True on success. |
| inline constexpr uint16_t OBJ_PROFILE_DECELERATION = 0x6084; ///< Profile deceleration (u32). | ||
| inline constexpr uint16_t OBJ_TARGET_VELOCITY = 0x60FF; ///< Target velocity (i32). | ||
| inline constexpr uint16_t OBJ_SOFTWARE_POSITION_LIMIT = | ||
| 0x607D; ///< Software position limit (i32:1/:2). |
Comment on lines
+37
to
+38
| /// \brief Human-readable name for a CiA 402 drive state (e.g. for logging). | ||
| static const char *to_string(State state) { return detail::ds402::state_to_string(state); } |
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>
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