fix(motor-control): five correctness bugs in basicmicro / canopen (no API change)

[finger563]

First of the follow-ups from the motor-control design review — §01C, the standalone bug list. No public API changes; each preserves 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), exactly like the position path. A raw static_cast<uint32_t>(gain * scale) bypassed those guards on the more commonly tuned loop.
• basicmicro read_status — the 32-bit fast path returned true without clearing the callers ec (it used a local ec32), so a caller reusing one std::error_code saw success reported with 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; the 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 callers requested width govern (take the low N bytes); keep the strict check when a size is indicated (genuine truncation/oversize still rejected).
• canopen last_abort_code() — reset the cached abort code at the start of every transaction so it cant 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 and heartbeat matching); clamp to 1 with a loud error.

Verified

• basicmicro + canopen host tests pass (the constexpr wire cores are unchanged).
• basicmicro and canopen examples build clean on IDF v6.0.1.

Runtime-level regression tests for the sdo_upload / abort paths need a mock-SDO-server host test — thats the separate test-seam pass (review §06 step 5), not this PR.

First in a stack: bugs → clarity → consistency. The clarity and consistency passes follow as stacked PRs.

🤖 Generated with Claude Code

[github-actions]

✅Static analysis result - no issues found! ✅

[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

Fixes multiple correctness bugs in the Basicmicro and CANopen client implementations without changing the public API, preserving the “true ⇒ ec cleared” contract and improving protocol conformance.

Changes:

• Validate/clamp CANopen node_id and reset cached abort code at the start of each transaction.
• Fix CANopen expedited sdo_upload to handle servers that don’t indicate size while still returning 4 bytes.
• Fix Basicmicro status fast-path to clear caller ec on success, and route velocity PID gains through scale_pid_gain().

Reviewed changes

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

File	Description
components/canopen/include/canopen_client.hpp	Adds node-id validation, improves expedited upload width handling, and resets abort code per transaction.
components/basicmicro/include/basicmicro.hpp	Clears ec on successful 32-bit status read and scales velocity PID gains safely/consistently.

---

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

[Copilot]

🟡 Changes recommended

Empty SDO output buffers can return an ambiguous zero-byte success, and the upload documentation contradicts the new behavior.

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

Review details

Suppressed comments (1)

components/canopen/include/canopen_client.hpp:310

• An empty out now takes the size-unspecified branch, computes n == 0, and returns the API's error sentinel without setting ec. Previously this was rejected because the four-byte response exceeded the buffer. Keep zero-capacity destinations on the error path so callers never receive an ambiguous zero-byte “success.”

    const size_t n =
        (response.size_indicated || response.len <= out.size()) ? response.len : out.size();
    if (n > out.size()) {

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