feat(basicmicro): position PID get/set for both channels (commands 61-64)

[finger563]

Summary

The component implemented the velocity PID (28/29, 55/56) but not the position PID, so closed-loop position moves could not be configured. This adds the full position-loop support for both channels:

• set_position_pid_m1 / set_position_pid_m2 (commands 61/62)
• read_position_pid_m1 / read_position_pid_m2 (commands 63/64)

Each covers the seven-constant position loop: P/I/D gains, MaxI (integral windup limit), Deadzone (encoder counts), and the MinPos/MaxPos clamp.

Details

• Scaling: position P/I/D are transferred scaled by 1024 (the position-loop convention in the Basicmicro reference library), distinct from the velocity loop's 16.16. Added kBasicmicroPositionPidScale alongside the existing kBasicmicroPidScale.
• Wire asymmetry: command 61/62 send D, P, I while 63/64 reply P, I, D (per the manual) — handled inside the shared set_position_pid / read_position_pid helpers and documented.
• The factory default for every position constant is zero, which clamps all position targets to [0, 0]; these setters are what a master (packet-serial or a CANopen bridge to the same registers) uses to make position mode usable.

Testing

• Host golden-frame test extended (test/basicmicro_host_test.cpp): the command enum values and a command-61 payload (D/P/I ×1024 + raw MaxI/Deadzone/MinPos/MaxPos, 28 data bytes) with CRC — ALL PASSED.
• README feature list updated.
• Exercised on real MCP266 hardware (the same registers are mirrored into the device's CANopen object dictionary; configuring the position loop this way is what made profile-position moves work).

🤖 Generated with Claude Code

[Copilot]

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

Adds full Basicmicro position-loop PID configuration support (commands 61–64) for both motor channels, including correct 1024x gain scaling and payload ordering.

Changes:

• Introduces position PID scale constant and command enum values for set/read on M1 and M2.
• Adds set_position_pid_* and read_position_pid_* APIs plus shared helpers handling wire-order asymmetry.
• Extends host golden-frame tests and updates README feature list.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
components/basicmicro/test/basicmicro_host_test.cpp	Adds command ID checks and golden packet/CRC coverage for command 61 position PID payload.
components/basicmicro/include/detail/basicmicro_core.hpp	Adds kBasicmicroPositionPidScale and defines commands 61–64 in BasicmicroCommand.
components/basicmicro/include/basicmicro.hpp	Adds public position PID get/set APIs for both channels and shared 61–64 encoding/decoding helpers.
components/basicmicro/README.md	Documents the new position PID capability in the feature list.

---

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

[github-actions]

✅Static analysis result - no issues found! ✅

[finger563]

Addressed the review comments:

• Truncation bias / negative wrap in gain scaling: added detail::scale_pid_gain(gain, scale) which rounds to nearest (via llroundf) instead of truncating, and clamps to the non-negative uint32_t range (PID gains are non-negative on these controllers; a raw cast of a negative product would wrap, and NaN now maps to 0). The position-PID setter uses it. Host test covers rounding, negative→0, and the velocity 16.16 scale.
• Read-path coverage: added a golden test that feeds a 28-byte command-63/64 reply and asserts p/i/d/max_i/deadzone/min_pos/max_pos decode from the correct offsets in the P, I, D reply order (distinct from the D, P, I write order), plus assertions that the M2 commands are 62/64.

[Copilot]

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[finger563]

Second round addressed:

• scale_pid_gain() overflow / non-finite UB: the scaling now happens in double, and a saturation check (!(scaled < 2^32), which is also false for +inf/NaN) runs before std::llround, so out-of-range and non-finite inputs saturate to UINT32_MAX (or 0) instead of reaching the rounding function. Switched from llroundf to std::llround. Host test now covers +inf, NaN, >> 2^32, exactly 2^32, and just-below-2^32.
• Payload reservation: set_position_pid() now reserve(28)s the payload up front.
• Test/production alignment: the golden-frame test builds the gains through scale_pid_gain() (matching production) rather than a raw cast.

[finger563]

Self-reviewed the full diff — no further issues. scale_pid_gain's boundary handling is correct (a value just under 2^32 that llrounds up to exactly 2^32 is caught by the > UINT32_MAX check and saturates), the read/write field orders match the manual, and the payload is pre-reserved. Host golden-frame test covers the new paths.