Add a diagnostics hook and exception isolation to EcSlaveBase - #244
Open
mcbed wants to merge 12 commits into
Open
Add a diagnostics hook and exception isolation to EcSlaveBase#244mcbed wants to merge 12 commits into
EcSlaveBase#244mcbed wants to merge 12 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.
This was referenced Aug 19, 2026
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.
mcbed
force-pushed
the
feat/slave-fault-isolation
branch
from
August 24, 2026 15:35
703da33 to
99b959b
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.
collectDiagnostics(): lets any slave report itself (generic online/AL-state by default; subclasses extend it).setHealth()feeds in the master's per-cycle link/AL-state observation.processDataSafe()/updateStateSafe(): anyEcMasterBaseimplementation should call these instead ofprocess_data()/updateState()directly — they catch, log, and record a fault so one misbehaving slave can't abort the read/write cycle for the rest of the bus.To be merged after #241