Add auto_enable flag (default false) — changes default behavior, read carefully - #247
Open
mcbed wants to merge 18 commits into
Open
Add auto_enable flag (default false) — changes default behavior, read carefully#247mcbed wants to merge 18 commits into
auto_enable flag (default false) — changes default behavior, read carefully#247mcbed wants to merge 18 commits into
Conversation
…e identity check to the master abstraction The master abstraction (EcMasterBase/EtherlabMaster) dropped several capabilities EcMaster had before the split: a blocking CoE SDO upload (read), public getters for master/domain/slave state, an is_valid() guard, and add_slave()'s vendor/product identity verification against what's actually on the bus. All are restored here at the EcMasterBase interface (as backend-agnostic types with no ecrt.h/IgH dependency) and implemented in EtherlabMaster: - EcMasterBase gains is_valid(), upload_slave_sdo(), get_master_state()/get_domain_state()/get_slave_states(), backed by new portable EcMasterStateInfo/EcDomainStateInfo/EcSlaveStateInfo structs (no ecrt_master_state_t/ec_domain_state_t leakage, so a future non-IgH master plugin isn't tied to these types). - EtherlabMaster::upload_slave_sdo() refuses (logs an error, returns -1) if called while activated_ — this is a blocking mailbox round-trip and must only run during the configure phase (after configure_slaves(), before start()), never while the cyclic process-data loop is running. - EtherlabMaster::add_slave() again refuses to configure a slave whose vendor/product doesn't match what's actually on the bus at that ring position (alias-0 slaves only, addressed by absolute position), instead of silently leaving it unconfigured. - EthercatBusManager's masterState()/domainState()/slaveStates()/ readSlaveSdo() are wired to the new methods; their declared return types (ec_master_state_t/ec_domain_state_t, with no <ecrt.h> in reach from this header) would not have compiled otherwise. Also fixes a pre-existing bug (bus_config_.master_id doesn't exist; the field is master_iface) and drops a dead, already-commented-out SDO config block superseded by EtherlabMaster::configure_slaves().
…hex status logging - Adopts the process_data(int index, ...) rename cleanly (removes the two commented-out old-signature leftovers from the rebase). - Restores targetPositionPassthrough(): extracts the hardcoded "override 0x607a unless CSP" check into an overridable virtual, so a vendor subclass can extend which modes pass the target position through (e.g. an impedance mode that also uses 0x607a as an equilibrium setpoint) without duplicating process_data(). - Logs status_word in hex (matches how it's read/reported everywhere else) instead of decimal.
…driver EthercatBusConfig::master_plugin defaults to "ethercat_master/EtherlabMaster", pluginlib-loaded at runtime, but nothing declared that this package needs to be installed/sourced for the default to resolve.
Port of 771e984 (never carried into EtherlabMaster when it superseded EcMaster). Without ecrt_release_master() the kernel device stays locked after the process exits, preventing re-initialization on restart without unloading the kernel module — critical in multi-master setups.
Generalizes the pattern behind a vendor's drive-commissioning check (e.g. verifying a torque sensor is configured before enabling impedance) into something any slave_config can use: a one-shot SDO upload (read) at configure time, refusing to configure the slave if the value isn't in an allowed set. Mirrors the existing sdo: write block structurally, but never writes anything — for hardware preconditions that must already be true on the physical drive (a commissioned sensor type, a firmware revision, ...), as opposed to sdo: which pushes a value onto the drive. - SdoCheckEntry (ec_sdo_manager.hpp): parses index/sub_index/type plus either a single `value` or a `values` list, and an optional `description` surfaced in the failure message. decode()/matches() reuse the existing little-endian read_* macros so behaviour matches the wire format ecrt_master_sdo_upload returns. - EcSlaveBase gains sdo_check_config_/get_sdo_check_config(), parsed from a slave_config's `sdo_check:` block in GenericEcSlave (and thus inherited by EcCiA402Drive) alongside the existing `sdo:` parsing. - EtherlabMaster::configure_slaves() runs each slave's checks (after its sdo: writes, so a check can validate their result) via ecrt_master_sdo_upload, and returns false — refusing to configure — on a read failure, a short read, or a value outside the allowed set.
…ister) A module that fails validation (vendor/product identity or sdo_check:) used to always abort the whole bus, at any scale — one bad drive out of 40 blocked everything. Splits bring-up into two phases: - Phase 1 (validate, every module, no side effects): identity check + sdo_check: reads. Both are addressed by ring position at the master level and need no ecrt_master_slave_config() first, so this has nothing to undo on failure. - Phase 2 (register, passing modules only): ecrt_master_slave_config() + PDO mapping + domain registration + sdo: writes — exactly what add_slave()/configure_slaves() already did, just restricted to the modules that passed phase 1. New per-module "required" param (absent or "true" — today's only behavior, unchanged — or "true": failure refuses to configure the whole bus, exactly as before. "false": the module is excluded instead — never added, absent from the cyclic domain, not readable or writable — while the rest of the bus still comes up. No new state or interface needed for this: EthercatClient::slaveHealth() (in the downstream consumer) already default-initializes to offline and only overwrites entries it finds a matching alias/position for in get_slave_states(), so an excluded module already reads as slave_online: false with zero changes on that side. EcMasterBase gains check_slave() (new pure virtual — validate only, no registration) alongside the existing add_slave(). EtherlabMaster factors the identity check and the sdo_check: reads (moved out of configure_slaves(), which now only does sdo: writes) into two shared private helpers used by both check_slave() and add_slave(), so add_slave()'s own behavior/contract for any direct caller is unchanged. Transparent for existing configs: with "required" absent everywhere (the default), phase 1 fail-fast-aborts on the first failing module exactly as add_slave() used to, byte for byte.
deactivateBus() only stopped our own cyclic loop; it never told the master or slaves anything, so slaves stayed stuck at OP indefinitely with no mailbox/SDO access. Per spec, loss of cyclic data should let each slave's Sync Manager Watchdog autonomously drop OP -> Safe-OP instead. Add EcMasterBase::deactivate() (ecrt_master_deactivate(), implemented in EtherlabMaster), call it from deactivateBus(), and gate master (re)request on master_ existing rather than on configured_ so a later activation re-requests the network but not the master. Verified on hardware: slave drops OP -> PREOP immediately on deactivate, reactivation brings it back to OP, SIGINT shutdown still releases the master cleanly.
deactivateBus() clearing configured_ meant a reactivation re-ran the full network setup, including the one-shot config SDO downloads (the slave_config YAML sdo: block) — those must be sent exactly once, not repeated every time the bus comes back from inactive. Split configNetwork() into registerSlaves() (identity/sdo_check: + PDO domain registration — must be re-runnable, since deactivate() frees that registration) and downloadSdoConfig() (the config SDO writes — must run exactly once, ever). Track their validity separately: network_registered_ is what deactivateBus() clears and activateBusLocked() rebuilds via registerSlaves() alone; configured_ now means "SDO config has been downloaded" and is never cleared by deactivateBus(). Verified on hardware: "Downloading config SDO(s)" logs exactly once across configure + two full deactivate/activate cycles, with the drive still reaching OP cleanly each time.
activateBusLocked() called spin_slaves_until_operational(), a hand-timed clock_nanosleep loop (no timeout) run outside the real RT cycle, purely to wait for a byproduct of the ordinary cyclic exchange: AL-state progression and DC sync already happen unconditionally on every read_process_data()/write_process_data() call, starting with the very first cycle once the bus is active. Each module's own state-machine walk already gates on its real per-slave AL state (is_operational_), so nothing needed this wait to be safe. Drop the call; on_activate() now returns as soon as master_->start() and transfer-net registration succeed. Slaves walk to OPERATIONAL over the following ordinary RT cycles instead of a bolted-on timing loop. spin_slaves_until_operational() and its sole helper update() had no other caller, so both are removed rather than left dead. Verified on hardware: activateBusLocked() now returns in ~1.6ms instead of blocking; the bus still reaches OP (EthercatClient::start(), unaffected, absorbs the wait via its own pre-existing settle loop), and a deactivate/reactivate cycle still works correctly.
be93554 removed spin_slaves_until_operational() (an untimed clock_nanosleep loop, with its own private read/write reimplementation) but left nothing in its place: activateBus() reported success as soon as master_->start() succeeded, with no guarantee any slave had actually reached OPERATIONAL before a caller's first read(). The old wait isn't a fit to restore even with a timeout added: it polled EcCiA402Drive::initialized() (CiA402 Operation Enabled), which can never become true during activation on a stack that doesn't auto-enable - it would just hang until timeout every time. waitForSlavesOperational() instead polls EcSlaveStateInfo::operational (bus AL-state, via slaveStates()) through the ordinary read()/write() cycle, bounded by a new readiness_timeout_s bus parameter (default 25s), rolling back via deactivateBus() on timeout or a read()/write() failure. Confirmed on real hardware (2dof bench): both slaves reached OPERATIONAL in ~9.7s, activateBus() blocked correctly until then, and controller spawning proceeded normally afterward.
A slave has no visibility into its own link/AL state; add setHealth() so whoever owns the master can push that in each cycle, and collectDiagnostics() so a slave can report itself. The base implementation covers generic EtherCAT-layer health (online, al_state); subclasses with more specific diagnostics override it, call the base first, then append. Non-pure virtual with a working default, so existing slave plugins are unaffected until they choose to extend it.
A throw from one slave's process_data()/updateState() would otherwise propagate out of the caller's per-cycle loop over the whole bus, aborting it for every other slave. Add processDataSafe()/updateStateSafe() wrappers on EcSlaveBase that any EcMasterBase implementation should call instead of the raw virtuals: they catch, log once, and record the fault so collectDiagnostics() can report it on that slave only.
collectDiagnostics() override: calls GenericEcSlave's first (generic online/al_state), then appends the CiA402 device state, mode of operation, and — only while faulted — the latched error code (CoE 0x603F, newly tracked here like status_word already was; stays 0 if a slave_config doesn't map it). Level/message are refined once online: FAULT/FAULT_REACTION_ACTIVE is ERROR, OPERATION_ENABLED is OK, anything else WARN.
Edge-triggered, no PDO object of their own (same shape as the existing reset_fault) — a rising edge latches a walk toward Operation Enabled or down to Switch-on-Disabled, driven one CiA402 transition per cycle regardless of auto_state_transitions_, until the target state is reached or a new request overrides it. disable_drive commands Disable Voltage (0x00) unconditionally, valid from any powered CiA402 state per the standard — a vendor subclass wanting a controlled quick-stop-first sequence can override this. No ROS services — command-interface-only, matching the existing reset_fault convention.
A walk with no timeout could run forever if the drive never reaches its target state. Give it 5s, matching the timeout convention already used elsewhere in this stack; on expiry, stop driving the controlword and latch which target was missed. Not a runtime error — surfaced only as a WARN-level diagnostic (collectDiagnostics), cleared by the next enable_drive/disable_drive request whether or not that one succeeds.
mcbed
force-pushed
the
feat/auto-enable
branch
from
August 24, 2026 15:35
b509f32 to
2d90468
Compare
Two command interfaces for one binary intent (walk up vs. walk down) meant a caller always had to manage both together - e.g. clear disable_drive before setting enable_drive, or the two could disagree. One interface now carries both directions: rising edge (0/NaN -> nonzero) walks to Operation Enabled, falling edge (nonzero -> 0/NaN) walks to Switch-on-Disabled, same kWalkTimeout-bounded walk and diagnostics as before either way. disable_drive_command_interface_index_/last_disable_drive_command_ are gone; nothing else in this repo referenced them (confirmed by search), and a downstream vendor's own hand-rolled enable/disable mechanism is entirely separate from this class, so this is not a cross-repo break.
mcbed
force-pushed
the
feat/auto-enable
branch
from
August 25, 2026 09:10
2d90468 to
2d36a39
Compare
walking_to_disabled_ unconditionally commanded Disable Voltage (0x00) from whatever state the drive was in - a direct, uncontrolled drop rather than the standard CiA402 shutdown sequence. Now: any powered state commands Quick Stop (0x02) first; only once the drive reports Quick Stop Active does it command Disable Voltage. Matches the CiA402-standard controlled stop and the same two-step pattern already used elsewhere for this exact transition. updateState()'s completion check (state_ == STATE_SWITCH_ON_DISABLED) is unchanged and still gates the same kWalkTimeout deadline across both steps, so a stuck Quick Stop still times out rather than hanging. Confirmed on real hardware: status word sequence 0x1237 (Operation Enabled) -> 0x1217 (Quick Stop Active) -> 0x0250 (Switch on Disabled) on both joints, falling edge on the same enable_drive interface.
Decouples "does the plugin manage CiA402 transitions generically" (auto_state_transitions) from "does that walk go all the way to Operation Enabled on its own". auto_state_transitions still defaults true and is unchanged for anyone already setting it explicitly either way. New default: the auto-walk now stops at Switched On (powered, not producing torque/motion) rather than continuing to Operation Enabled, until an explicit enable_drive request takes the last step. auto_enable: true restores the old always-auto-enable behaviour, for a caller not using the enable_drive/disable_drive mechanism at all. Docs updated to match (config_cia402_drive.rst).
mcbed
force-pushed
the
feat/auto-enable
branch
from
August 25, 2026 10:03
2d36a39 to
6d45dae
Compare
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
1 participant
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.
Before this,
auto_state_transitions: true(still the default) walked a drive all the way toOPERATION_ENABLEDautomatically. Now the auto-walk stops atSWITCHED_ON(powered, not yet producing torque/motion) unlessauto_enable: trueis set, or an explicitenable_driverequest takes the last step.Any config that relies on the old implicit auto-enable-to-
OPERATION_ENABLEDbehavior needsauto_enable: trueadded to keep it.To be merged after PR #246