From 18b818481614c602c3026f9cf17963a79b01c90e Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Wed, 2 Sep 2026 21:44:10 -0500 Subject: [PATCH 1/3] fix(motor-control): five correctness bugs in basicmicro / canopen (no API change) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- components/basicmicro/include/basicmicro.hpp | 11 ++++-- components/canopen/include/canopen_client.hpp | 38 +++++++++++++++---- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/components/basicmicro/include/basicmicro.hpp b/components/basicmicro/include/basicmicro.hpp index ebf96893e6..7216314407 100644 --- a/components/basicmicro/include/basicmicro.hpp +++ b/components/basicmicro/include/basicmicro.hpp @@ -818,6 +818,8 @@ class Basicmicro : public BaseComponent { std::error_code ec32; if (read_command(Command::ReadStatus, data, ec32)) { status = detail::read_u32_be(data, 0); + ec.clear(); // the 32-bit attempt used a local ec32; honor the "true => ec + // cleared" contract so a caller reusing ec sees success return true; } } @@ -992,9 +994,12 @@ class Basicmicro : public BaseComponent { bool set_velocity_pid(Command cmd, float p, float i, float d, uint32_t qpps, std::error_code &ec) { std::vector payload; - detail::append_u32_be(payload, static_cast(d * detail::kBasicmicroPidScale)); - detail::append_u32_be(payload, static_cast(p * detail::kBasicmicroPidScale)); - detail::append_u32_be(payload, static_cast(i * detail::kBasicmicroPidScale)); + // Route through scale_pid_gain (rounds; guards negative -> uint32 wrap and + // NaN/inf -> UB in std::llround) just like the position path — a raw + // static_cast of the float product bypassed those guards. + detail::append_u32_be(payload, detail::scale_pid_gain(d, detail::kBasicmicroPidScale)); + detail::append_u32_be(payload, detail::scale_pid_gain(p, detail::kBasicmicroPidScale)); + detail::append_u32_be(payload, detail::scale_pid_gain(i, detail::kBasicmicroPidScale)); detail::append_u32_be(payload, qpps); return write_command(cmd, payload, ec); } diff --git a/components/canopen/include/canopen_client.hpp b/components/canopen/include/canopen_client.hpp index 5c6b4d711d..5bea672881 100644 --- a/components/canopen/include/canopen_client.hpp +++ b/components/canopen/include/canopen_client.hpp @@ -78,7 +78,15 @@ class CanopenClient : public BaseComponent { , node_id_(config.node_id) , send_(config.send) , sdo_timeout_(config.sdo_timeout) - , on_heartbeat_(config.on_heartbeat) {} + , on_heartbeat_(config.on_heartbeat) { + // A CANopen node id is 1-127; 0 is the broadcast/unconfigured value and would + // make SDO addressing (0x580/0x600 + id) and heartbeat matching wrong. + if (node_id_ < 1 || node_id_ > 127) { + logger_.error("node_id {} is out of range (1-127); clamping to 1 — set a valid node id", + node_id_); + node_id_ = 1; + } + } /// \brief The configured server node id. uint8_t node_id() const { return node_id_; } @@ -286,15 +294,28 @@ class CanopenClient : public BaseComponent { index, subindex, ec)) { return 0; } - if (response.type != detail::canopen::SdoResponse::Type::ExpeditedUpload || - response.len > out.size()) { - logger_.error("SDO upload 0x{:04X}:{:02X}: not an expedited response of <= {} bytes", index, - subindex, out.size()); + if (response.type != detail::canopen::SdoResponse::Type::ExpeditedUpload) { + logger_.error("SDO upload 0x{:04X}:{:02X}: not an expedited response", index, subindex); + ec = std::make_error_code(std::errc::protocol_error); + return 0; + } + // When the server INDICATED a size, the object must fit the caller's buffer + // (a larger object is a real width mismatch -> error below). When it did NOT + // indicate a size, CiA 301 says all four expedited data bytes are valid and + // the caller's requested width governs, so take the low out.size() bytes — + // otherwise a conformant u8/u16 read against a server that leaves the size + // bit clear (where the core reports len == 4) would spuriously fail. + const size_t n = + (response.size_indicated || response.len <= out.size()) ? response.len : out.size(); + if (n > out.size()) { + logger_.error( + "SDO upload 0x{:04X}:{:02X}: object is {} bytes, larger than the {}-byte buffer", index, + subindex, response.len, out.size()); ec = std::make_error_code(std::errc::protocol_error); return 0; } - std::copy_n(response.data.begin(), response.len, out.begin()); - return response.len; + std::copy_n(response.data.begin(), n, out.begin()); + return n; } /// \brief Read a string object via SDO segmented (or expedited) upload. @@ -449,6 +470,9 @@ class CanopenClient : public BaseComponent { { std::lock_guard lock(response_mutex_); awaiting_response_ = true; + // Clear any abort code cached by a previous transaction so last_abort_code() + // never reports a stale code from an earlier, unrelated failure. + last_abort_code_ = 0; // Record what the in-flight request is for, so process_frame() can // reject stale/unrelated responses instead of completing the wrong // transaction (segment responses carry no index/subindex and are From 73bcd63310e1e10a684fd3222a1e6f6cc79f82d3 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Wed, 2 Sep 2026 22:57:47 -0500 Subject: [PATCH 2/3] fix(canopen): address bug-PR review comments Follow-ups from the #762 review: - sdo_upload (expedited): add a source-side bound check before std::copy_n so a malformed/oversized reported length can never read past the fixed-size expedited data buffer (the existing guard only bounded the destination). - node_id out-of-range log: cast the uint8_t to unsigned so the numeric id is printed reliably regardless of the formatter's char handling. - basicmicro read_status: reflow the split-across-two-lines quoted comment so the "true => ec cleared" contract reads as one contiguous phrase. No behavior change on the valid-frame path. Builds clean: canopen example on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) --- components/basicmicro/include/basicmicro.hpp | 5 +++-- components/canopen/include/canopen_client.hpp | 11 ++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/components/basicmicro/include/basicmicro.hpp b/components/basicmicro/include/basicmicro.hpp index 7216314407..72d14448e1 100644 --- a/components/basicmicro/include/basicmicro.hpp +++ b/components/basicmicro/include/basicmicro.hpp @@ -818,8 +818,9 @@ class Basicmicro : public BaseComponent { std::error_code ec32; if (read_command(Command::ReadStatus, data, ec32)) { status = detail::read_u32_be(data, 0); - ec.clear(); // the 32-bit attempt used a local ec32; honor the "true => ec - // cleared" contract so a caller reusing ec sees success + // The 32-bit attempt used a local ec32; honor the "true => ec cleared" + // contract so a caller reusing ec sees success. + ec.clear(); return true; } } diff --git a/components/canopen/include/canopen_client.hpp b/components/canopen/include/canopen_client.hpp index 5bea672881..a83c721344 100644 --- a/components/canopen/include/canopen_client.hpp +++ b/components/canopen/include/canopen_client.hpp @@ -83,7 +83,7 @@ class CanopenClient : public BaseComponent { // make SDO addressing (0x580/0x600 + id) and heartbeat matching wrong. if (node_id_ < 1 || node_id_ > 127) { logger_.error("node_id {} is out of range (1-127); clamping to 1 — set a valid node id", - node_id_); + static_cast(node_id_)); node_id_ = 1; } } @@ -314,6 +314,15 @@ class CanopenClient : public BaseComponent { ec = std::make_error_code(std::errc::protocol_error); return 0; } + // Defensive: never read past the fixed-size expedited data buffer even if a + // malformed frame or parser bug reported a length the parser should have + // capped at 4 (guards the copy_n source, not just the out destination). + if (n > response.data.size()) { + logger_.error("SDO upload 0x{:04X}:{:02X}: reported length {} exceeds the {}-byte payload", + index, subindex, response.len, response.data.size()); + ec = std::make_error_code(std::errc::protocol_error); + return 0; + } std::copy_n(response.data.begin(), n, out.begin()); return n; } From fa1b6402fdd6bfa5e8cc36bd6ed7723ea7c61fcd Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Thu, 3 Sep 2026 11:23:09 -0500 Subject: [PATCH 3/3] docs(canopen): clarify sdo_upload size-mismatch contract Per review, document that the strict "object larger than out is rejected" rule only applies when the server INDICATES a size; when it does not, expedited data is truncated to out.size() (the narrow-read path), so callers must size out to the width they expect. Co-Authored-By: Claude Opus 4.8 (1M context) --- components/canopen/include/canopen_client.hpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/components/canopen/include/canopen_client.hpp b/components/canopen/include/canopen_client.hpp index a83c721344..f85c0ac377 100644 --- a/components/canopen/include/canopen_client.hpp +++ b/components/canopen/include/canopen_client.hpp @@ -284,9 +284,16 @@ class CanopenClient : public BaseComponent { /// \param index Object dictionary index. /// \param subindex Object dictionary subindex. /// \param out Destination for the object data (little-endian). - /// \param ec Set on transmit failure, timeout, SDO abort, or if the object is - /// larger than \p out (use read_string() for segmented transfers). + /// \param ec Set on transmit failure, timeout, SDO abort, or a size mismatch + /// (use read_string() for segmented transfers). /// \return Number of bytes read (> 0), or 0 on error. + /// \note Size handling depends on whether the server indicated the object size + /// in its response. When it did, an object larger than \p out is rejected + /// as a width mismatch (ec = protocol_error). When it did NOT (CiA 301 + /// allows this for expedited transfers, where all four data bytes are + /// valid), the low \p out.size() bytes are returned and any remaining + /// high bytes are truncated -- so a caller must size \p out to the width + /// it expects for such objects. size_t sdo_upload(uint16_t index, uint8_t subindex, std::span out, std::error_code &ec) { std::lock_guard lock(sdo_mutex_); detail::canopen::SdoResponse response;