From 18b818481614c602c3026f9cf17963a79b01c90e Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Wed, 2 Sep 2026 21:44:10 -0500 Subject: [PATCH 1/4] 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 729fc92620c13a812c87a81a38e6d3779a93c020 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Wed, 2 Sep 2026 21:50:14 -0500 Subject: [PATCH 2/4] refactor(motor-control): clarity pass - surface public helpers, use named constants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../canopen/example/main/canopen_example.cpp | 2 +- components/canopen/include/canopen_client.hpp | 6 ++++ .../canopen/include/detail/canopen_core.hpp | 16 +++++---- components/canopen/include/ds402.hpp | 10 ++++++ components/mcp266/include/mcp266.hpp | 34 ++++++++++++++++--- 5 files changed, 55 insertions(+), 13 deletions(-) diff --git a/components/canopen/example/main/canopen_example.cpp b/components/canopen/example/main/canopen_example.cpp index 175307d5fa..60e596995d 100644 --- a/components/canopen/example/main/canopen_example.cpp +++ b/components/canopen/example/main/canopen_example.cpp @@ -129,7 +129,7 @@ extern "C" void app_main(void) { } else { // DS402: profile velocity mode, enable, gentle ramp, stop, disable. if (auto state = drive.get_state(ec); !ec) { - logger.info("Drive state: {}", espp::detail::ds402::state_to_string(state)); + logger.info("Drive state: {}", espp::Ds402Drive::to_string(state)); if (state == espp::Ds402Drive::State::Fault) { logger.info("Drive is in Fault; attempting fault reset"); if (!drive.fault_reset(ec)) { diff --git a/components/canopen/include/canopen_client.hpp b/components/canopen/include/canopen_client.hpp index 5bea672881..b6caf57922 100644 --- a/components/canopen/include/canopen_client.hpp +++ b/components/canopen/include/canopen_client.hpp @@ -444,6 +444,12 @@ class CanopenClient : public BaseComponent { return last_abort_code_; } + /// \brief Human-readable description of a CiA 301 SDO abort code (e.g. for + /// logging the reason behind an SDO failure / last_abort_code()). + static const char *abort_code_to_string(uint32_t abort_code) { + return detail::canopen::sdo_abort_to_string(abort_code); + } + /// @} protected: diff --git a/components/canopen/include/detail/canopen_core.hpp b/components/canopen/include/detail/canopen_core.hpp index a747cecde5..fb5836062d 100644 --- a/components/canopen/include/detail/canopen_core.hpp +++ b/components/canopen/include/detail/canopen_core.hpp @@ -462,13 +462,15 @@ inline constexpr uint16_t OBJ_CONTROLWORD = 0x6040; ///< Controlword (u16 inline constexpr uint16_t OBJ_STATUSWORD = 0x6041; ///< Statusword (u16). inline constexpr uint16_t OBJ_MODES_OF_OPERATION = 0x6060; ///< Modes of operation (i8). inline constexpr uint16_t OBJ_MODES_OF_OPERATION_DISPLAY = 0x6061; ///< Modes display (i8). -inline constexpr uint16_t OBJ_POSITION_ACTUAL = 0x6064; ///< Position actual value (i32). -inline constexpr uint16_t OBJ_VELOCITY_ACTUAL = 0x606C; ///< Velocity actual value (i32). -inline constexpr uint16_t OBJ_TARGET_POSITION = 0x607A; ///< Target position (i32). -inline constexpr uint16_t OBJ_PROFILE_VELOCITY = 0x6081; ///< Profile velocity (u32). -inline constexpr uint16_t OBJ_PROFILE_ACCELERATION = 0x6083; ///< Profile acceleration (u32). -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). +inline constexpr uint16_t OBJ_POSITION_ACTUAL = 0x6064; ///< Position actual value (i32). +inline constexpr uint16_t OBJ_VELOCITY_ACTUAL = 0x606C; ///< Velocity actual value (i32). +inline constexpr uint16_t OBJ_TARGET_POSITION = 0x607A; ///< Target position (i32). +inline constexpr uint16_t OBJ_PROFILE_VELOCITY = 0x6081; ///< Profile velocity (u32). +inline constexpr uint16_t OBJ_PROFILE_ACCELERATION = 0x6083; ///< Profile acceleration (u32). +inline constexpr uint16_t OBJ_PROFILE_DECELERATION = 0x6084; ///< Profile deceleration (u32). +inline constexpr uint16_t OBJ_TARGET_VELOCITY = 0x60FF; ///< Target velocity (i32). /// @} /// @name Object-index bounds used for per-axis offsetting diff --git a/components/canopen/include/ds402.hpp b/components/canopen/include/ds402.hpp index e0123e6924..191282a64a 100644 --- a/components/canopen/include/ds402.hpp +++ b/components/canopen/include/ds402.hpp @@ -34,6 +34,16 @@ class Ds402Drive : public BaseComponent { using State = detail::ds402::State; ///< CiA 402 drive state. using OperatingMode = detail::ds402::OperatingMode; ///< CiA 402 mode of operation. + /// \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); } + + /// \brief Decode a raw CiA 402 statusword (object 0x6041) into a drive state. + /// \details Useful for decoding a statusword you already have (e.g. from a + /// cached read or a TPDO) without another SDO round-trip. + static State state_from_statusword(uint16_t statusword) { + return detail::ds402::decode_state(statusword); + } + /// \brief Configuration for the Ds402Drive. struct Config { std::chrono::milliseconds state_timeout{ diff --git a/components/mcp266/include/mcp266.hpp b/components/mcp266/include/mcp266.hpp index 0d5e09aff8..bec59c9bd1 100644 --- a/components/mcp266/include/mcp266.hpp +++ b/components/mcp266/include/mcp266.hpp @@ -202,7 +202,8 @@ class Mcp266 : public BaseComponent { ec = std::make_error_code(std::errc::invalid_argument); return false; } - const uint16_t obj = static_cast(0x607D + axis_state(axis).objects.object_offset); + const uint16_t obj = detail::ds402::apply_axis_offset( + detail::ds402::OBJ_SOFTWARE_POSITION_LIMIT, axis_state(axis).objects.object_offset); return client_.write_i32(obj, 1, min_pos, ec) && client_.write_i32(obj, 2, max_pos, ec); } @@ -284,6 +285,22 @@ class Mcp266 : public BaseComponent { statusword = axis_state(axis).drive.get_statusword(ec); return !ec; } + /// \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. + bool get_state(Axis axis, Ds402Drive::State &state, std::error_code &ec) { + ec.clear(); + state = axis_state(axis).drive.get_state(ec); + return !ec; + } + /// \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. + bool is_target_reached(Axis axis, bool &reached, std::error_code &ec) { + ec.clear(); + reached = axis_state(axis).drive.is_target_reached(ec); + return !ec; + } /// @} @@ -326,8 +343,14 @@ class Mcp266 : public BaseComponent { /// Coarse fallback position P gain, used only when the drive's stored gain /// reads back as zero (see configure_position_loop()). It is a non-tuned /// starting point that produces motion out of the box, not a good gain for - /// any particular motor; callers should tune and pass their own. - static constexpr int32_t kDefaultPositionP = 0x3C83; + /// any particular motor; callers should tune and pass their own. 15491 is + /// ~15.1 in the MCP's position-PID fixed-point representation (x1024). + static constexpr int32_t kDefaultPositionP = 15491; // = 0x3C83 + + /// The MCP266 does not echo the requested mode in 0x6061, so after writing the + /// mode of operation enable() waits this fixed settle time before reading the + /// state rather than polling the (unchanging) mode display. + static constexpr auto kModeSettle = std::chrono::milliseconds(25); /// Per-axis state: the manufacturer object addresses and a Ds402Drive whose /// object offset selects M1 (0) or M2 (0x800). @@ -353,12 +376,13 @@ class Mcp266 : public BaseComponent { /// display -- would time out), clear any fault, and walk to Operation /// Enabled. bool enable(AxisState &a, Ds402Drive::OperatingMode mode, std::error_code &ec) { - const uint16_t mode_obj = static_cast(0x6060 + a.objects.object_offset); + const uint16_t mode_obj = detail::ds402::apply_axis_offset( + detail::ds402::OBJ_MODES_OF_OPERATION, a.objects.object_offset); if (!client_.write_i8(mode_obj, 0, static_cast(mode), ec)) { logger_.error("{}: failed to set mode: {}", a.name, ec.message()); return false; } - std::this_thread::sleep_for(std::chrono::milliseconds(25)); + std::this_thread::sleep_for(kModeSettle); const auto state = a.drive.get_state(ec); if (ec) { return false; From a598188e8c4ac1d6e7746bd66e72e8eed6bcb2cb Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Wed, 2 Sep 2026 22:57:24 -0500 Subject: [PATCH 3/4] refactor(motor-control): address clarity-PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../canopen/include/detail/canopen_core.hpp | 10 ++-- components/canopen/include/ds402.hpp | 2 + components/mcp266/include/mcp266.hpp | 54 +++++++++++++------ 3 files changed, 45 insertions(+), 21 deletions(-) diff --git a/components/canopen/include/detail/canopen_core.hpp b/components/canopen/include/detail/canopen_core.hpp index fb5836062d..bf1cf33235 100644 --- a/components/canopen/include/detail/canopen_core.hpp +++ b/components/canopen/include/detail/canopen_core.hpp @@ -463,11 +463,11 @@ inline constexpr uint16_t OBJ_STATUSWORD = 0x6041; ///< Statusword (u16) inline constexpr uint16_t OBJ_MODES_OF_OPERATION = 0x6060; ///< Modes of operation (i8). inline constexpr uint16_t OBJ_MODES_OF_OPERATION_DISPLAY = 0x6061; ///< Modes display (i8). inline constexpr uint16_t OBJ_SOFTWARE_POSITION_LIMIT = - 0x607D; ///< Software position limit (i32:1/:2). -inline constexpr uint16_t OBJ_POSITION_ACTUAL = 0x6064; ///< Position actual value (i32). -inline constexpr uint16_t OBJ_VELOCITY_ACTUAL = 0x606C; ///< Velocity actual value (i32). -inline constexpr uint16_t OBJ_TARGET_POSITION = 0x607A; ///< Target position (i32). -inline constexpr uint16_t OBJ_PROFILE_VELOCITY = 0x6081; ///< Profile velocity (u32). + 0x607D; ///< Software position limit (i32; subindex 1 = min, 2 = max). +inline constexpr uint16_t OBJ_POSITION_ACTUAL = 0x6064; ///< Position actual value (i32). +inline constexpr uint16_t OBJ_VELOCITY_ACTUAL = 0x606C; ///< Velocity actual value (i32). +inline constexpr uint16_t OBJ_TARGET_POSITION = 0x607A; ///< Target position (i32). +inline constexpr uint16_t OBJ_PROFILE_VELOCITY = 0x6081; ///< Profile velocity (u32). inline constexpr uint16_t OBJ_PROFILE_ACCELERATION = 0x6083; ///< Profile acceleration (u32). inline constexpr uint16_t OBJ_PROFILE_DECELERATION = 0x6084; ///< Profile deceleration (u32). inline constexpr uint16_t OBJ_TARGET_VELOCITY = 0x60FF; ///< Target velocity (i32). diff --git a/components/canopen/include/ds402.hpp b/components/canopen/include/ds402.hpp index 191282a64a..4d9cc18e15 100644 --- a/components/canopen/include/ds402.hpp +++ b/components/canopen/include/ds402.hpp @@ -35,6 +35,8 @@ class Ds402Drive : public BaseComponent { using OperatingMode = detail::ds402::OperatingMode; ///< CiA 402 mode of operation. /// \brief Human-readable name for a CiA 402 drive state (e.g. for logging). + /// \return A pointer to a string literal with static lifetime — do not free it; + /// it stays valid for the program's duration. static const char *to_string(State state) { return detail::ds402::state_to_string(state); } /// \brief Decode a raw CiA 402 statusword (object 0x6041) into a drive state. diff --git a/components/mcp266/include/mcp266.hpp b/components/mcp266/include/mcp266.hpp index bec59c9bd1..240405d337 100644 --- a/components/mcp266/include/mcp266.hpp +++ b/components/mcp266/include/mcp266.hpp @@ -194,8 +194,11 @@ class Mcp266 : public BaseComponent { } /// \brief Set the CiA 402 software position limits (0x607D:1/:2) for an axis. - /// \param axis The motor channel. \param min_pos Lower limit. \param max_pos - /// Upper limit. \param ec Set on failure. \return True on success. + /// \param axis The motor channel. + /// \param min_pos Lower limit. + /// \param max_pos Upper limit. + /// \param ec Set on failure. + /// \return True on success. bool set_position_limits(Axis axis, int32_t min_pos, int32_t max_pos, std::error_code &ec) { ec.clear(); if (min_pos > max_pos) { @@ -264,30 +267,41 @@ class Mcp266 : public BaseComponent { /// @name Feedback /// @{ - /// \brief Read the actual position (0x6064 / 0x6864). \param axis Channel. - /// \param count Out: encoder counts. \param ec Set on failure. \return True on success. + /// \brief Read the actual position (0x6064 / 0x6864). + /// \param axis Channel. + /// \param count Out: encoder counts. + /// \param ec Set on failure. + /// \return True on success. bool read_encoder(Axis axis, int32_t &count, std::error_code &ec) { ec.clear(); count = axis_state(axis).drive.get_position_actual(ec); return !ec; } - /// \brief Read the actual velocity (0x606C / 0x686C). \param axis Channel. - /// \param qpps Out: counts/s. \param ec Set on failure. \return True on success. + /// \brief Read the actual velocity (0x606C / 0x686C). + /// \param axis Channel. + /// \param qpps Out: counts/s. + /// \param ec Set on failure. + /// \return True on success. bool read_speed(Axis axis, int32_t &qpps, std::error_code &ec) { ec.clear(); qpps = axis_state(axis).drive.get_velocity_actual(ec); return !ec; } - /// \brief Read the CiA 402 statusword (0x6041 / 0x6841). \param axis Channel. - /// \param statusword Out. \param ec Set on failure. \return True on success. + /// \brief Read the CiA 402 statusword (0x6041 / 0x6841). + /// \param axis Channel. + /// \param statusword Out. + /// \param ec Set on failure. + /// \return True on success. bool read_statusword(Axis axis, uint16_t &statusword, std::error_code &ec) { ec.clear(); statusword = axis_state(axis).drive.get_statusword(ec); return !ec; } /// \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. + /// \param axis Channel. + /// \param state Out: the power-drive-system state. + /// \param ec Set on failure. + /// \return True on success. bool get_state(Axis axis, Ds402Drive::State &state, std::error_code &ec) { ec.clear(); state = axis_state(axis).drive.get_state(ec); @@ -295,7 +309,10 @@ class Mcp266 : public BaseComponent { } /// \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. + /// \param axis Channel. + /// \param reached Out. + /// \param ec Set on failure. + /// \return True on success. bool is_target_reached(Axis axis, bool &reached, std::error_code &ec) { ec.clear(); reached = axis_state(axis).drive.is_target_reached(ec); @@ -307,15 +324,19 @@ class Mcp266 : public BaseComponent { /// @name Device telemetry /// @{ - /// \brief Read the main battery voltage (mirrored command 24). \param volts - /// Out: volts. \param ec Set on failure. \return True on success. + /// \brief Read the main battery voltage (mirrored command 24). + /// \param volts Out: volts. + /// \param ec Set on failure. + /// \return True on success. bool read_main_battery_voltage(float &volts, std::error_code &ec) { ec.clear(); volts = static_cast(client_.read_u16(detail::mcp266::kMainBatteryObject, 0, ec)) / 10.0f; return !ec; } - /// \brief Read the board temperature (mirrored command 82). \param temp_c - /// Out: degrees C. \param ec Set on failure. \return True on success. + /// \brief Read the board temperature (mirrored command 82). + /// \param temp_c Out: degrees C. + /// \param ec Set on failure. + /// \return True on success. bool read_temperature(float &temp_c, std::error_code &ec) { ec.clear(); temp_c = @@ -336,7 +357,8 @@ class Mcp266 : public BaseComponent { /// @} /// \brief Access an axis's underlying Ds402Drive for advanced CiA 402 use. - /// \param axis The motor channel. \return Reference to the axis drive helper. + /// \param axis The motor channel. + /// \return Reference to the axis drive helper. Ds402Drive &drive(Axis axis) { return axis_state(axis).drive; } private: From 687dc9d9d48eb62771e2a7e5f766023a228e2203 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Thu, 3 Sep 2026 11:27:47 -0500 Subject: [PATCH 4/4] feat(canopen): libfmt formatters for the CiA 402 / NMT enums 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) --- .../canopen/example/main/canopen_example.cpp | 4 +- components/canopen/include/canopen_client.hpp | 1 + .../include/canopen_format_helpers.hpp | 101 ++++++++++++++++++ doc/Doxyfile | 1 + 4 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 components/canopen/include/canopen_format_helpers.hpp diff --git a/components/canopen/example/main/canopen_example.cpp b/components/canopen/example/main/canopen_example.cpp index 60e596995d..6be1b46927 100644 --- a/components/canopen/example/main/canopen_example.cpp +++ b/components/canopen/example/main/canopen_example.cpp @@ -79,7 +79,7 @@ extern "C" void app_main(void) { .on_heartbeat = // captureless: logger has static storage duration (see above) [](uint8_t hb_node, espp::CanopenClient::NmtState state) { - logger.info("Heartbeat from node {}: NMT state {}", hb_node, static_cast(state)); + logger.info("Heartbeat from node {}: NMT state {}", hb_node, state); }, .log_level = espp::Logger::Verbosity::INFO, }); @@ -129,7 +129,7 @@ extern "C" void app_main(void) { } else { // DS402: profile velocity mode, enable, gentle ramp, stop, disable. if (auto state = drive.get_state(ec); !ec) { - logger.info("Drive state: {}", espp::Ds402Drive::to_string(state)); + logger.info("Drive state: {}", state); if (state == espp::Ds402Drive::State::Fault) { logger.info("Drive is in Fault; attempting fault reset"); if (!drive.fault_reset(ec)) { diff --git a/components/canopen/include/canopen_client.hpp b/components/canopen/include/canopen_client.hpp index b6caf57922..6d89657334 100644 --- a/components/canopen/include/canopen_client.hpp +++ b/components/canopen/include/canopen_client.hpp @@ -12,6 +12,7 @@ #include #include "base_component.hpp" +#include "canopen_format_helpers.hpp" #include "detail/canopen_core.hpp" namespace espp { diff --git a/components/canopen/include/canopen_format_helpers.hpp b/components/canopen/include/canopen_format_helpers.hpp new file mode 100644 index 0000000000..216b1152f7 --- /dev/null +++ b/components/canopen/include/canopen_format_helpers.hpp @@ -0,0 +1,101 @@ +#pragma once + +#include + +#include "format.hpp" + +#include "detail/canopen_core.hpp" + +// libfmt formatters for the CANopen / CiA 402 enums so application code can print +// them directly -- e.g. `logger.info("Drive state: {}", state)` -- instead of +// calling a to-string helper inline at every call site. + +/// \brief fmt formatter for a CiA 402 drive state (espp::Ds402Drive::State). +template <> struct fmt::formatter : fmt::formatter { + template + auto format(espp::detail::ds402::State state, FormatContext &ctx) const { + return fmt::formatter::format(espp::detail::ds402::state_to_string(state), + ctx); + } +}; + +/// \brief fmt formatter for a CiA 402 mode of operation +/// (espp::Ds402Drive::OperatingMode). +template <> +struct fmt::formatter : fmt::formatter { + template + auto format(espp::detail::ds402::OperatingMode mode, FormatContext &ctx) const { + using espp::detail::ds402::OperatingMode; + std::string_view s = "Unknown"; + switch (mode) { + case OperatingMode::ProfilePosition: + s = "Profile position"; + break; + case OperatingMode::ProfileVelocity: + s = "Profile velocity"; + break; + case OperatingMode::ProfileTorque: + s = "Profile torque"; + break; + case OperatingMode::Homing: + s = "Homing"; + break; + } + return fmt::formatter::format(s, ctx); + } +}; + +/// \brief fmt formatter for an NMT node state (espp::CanopenClient::NmtState). +template <> +struct fmt::formatter : fmt::formatter { + template + auto format(espp::detail::canopen::NmtState state, FormatContext &ctx) const { + using espp::detail::canopen::NmtState; + std::string_view s = "Unknown"; + switch (state) { + case NmtState::BootUp: + s = "Boot-up"; + break; + case NmtState::Stopped: + s = "Stopped"; + break; + case NmtState::Operational: + s = "Operational"; + break; + case NmtState::PreOperational: + s = "Pre-operational"; + break; + case NmtState::Unknown: + break; + } + return fmt::formatter::format(s, ctx); + } +}; + +/// \brief fmt formatter for an NMT master command (espp::CanopenClient::NmtCommand). +template <> +struct fmt::formatter : fmt::formatter { + template + auto format(espp::detail::canopen::NmtCommand cmd, FormatContext &ctx) const { + using espp::detail::canopen::NmtCommand; + std::string_view s = "Unknown"; + switch (cmd) { + case NmtCommand::Start: + s = "Start"; + break; + case NmtCommand::Stop: + s = "Stop"; + break; + case NmtCommand::PreOperational: + s = "Pre-operational"; + break; + case NmtCommand::ResetNode: + s = "Reset node"; + break; + case NmtCommand::ResetCommunication: + s = "Reset communication"; + break; + } + return fmt::formatter::format(s, ctx); + } +}; diff --git a/doc/Doxyfile b/doc/Doxyfile index ba074b4d14..81de57f18c 100755 --- a/doc/Doxyfile +++ b/doc/Doxyfile @@ -257,6 +257,7 @@ INPUT = \ $(PROJECT_PATH)/components/button/include/button.hpp \ $(PROJECT_PATH)/components/byte90/include/byte90.hpp \ $(PROJECT_PATH)/components/canopen/include/canopen_client.hpp \ + $(PROJECT_PATH)/components/canopen/include/canopen_format_helpers.hpp \ $(PROJECT_PATH)/components/canopen/include/detail/canopen_core.hpp \ $(PROJECT_PATH)/components/canopen/include/ds402.hpp \ $(PROJECT_PATH)/components/chsc6x/include/chsc6x.hpp \