Extract core logic to ethercat bus master#223
Merged
mcbed merged 7 commits intoJul 9, 2026
Merged
Conversation
Call ecrt_release_master() in ~EcMaster() to properly release the kernel EtherCAT master device. Without this, the master remains locked after the process exits, preventing re-initialization on restart without unloading the kernel module. This is especially critical in multi-master setups where multiple EcMaster instances coexist.
|
I've ran the tests:
Good work! |
JenniferBuehler
marked this pull request as ready for review
June 11, 2026 12:05
tgaspar
force-pushed
the
feat/extract-core-logic-to-ethercat-bus-master
branch
from
June 12, 2026 06:39
56d7f46 to
a550602
Compare
mcbed
approved these changes
Jul 9, 2026
mcbed
left a comment
Member
There was a problem hiding this comment.
Good for me, hardware and safety tests are passing
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
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.
This PR proposes to extract the core logic from
ethercat_driver.cppinto two modules which are in itself largely ROS2-independent and can be re-used in different hardware interface implementations (or even without ros2_control):EthercatBusManagerwhich handles configuration, activation, reading, writing to/from the EtherCAT bus. That was previously all done in theEthercatDriver(ros2_control hardware interface).<ec_module>blocks has been extracted to a separate parser function inethercat_ros2_control_xml_parser.hpp/cppWhat remains ROS2 related (which is fine, we mainly want to extract from ros2_control hardware interface, so we can re-use in a different hardware interface!):
ECSlaveplugins. Reasoning: while plugins are part of the ROS2 ecosystem, it is a generic and very useful concept (it would actually be better if it lived outside the ROS2 ecosystem). We want the user to be able to load their own slaves through plugins. So we'll keep using this insideEthercatBusManagereven if it's ROS2.Important
Merge after #221 !
Important
This PR is focussed mainly on moving code, without changing logic significantly (though a few minor changes were made, see sections below).

However the diff still looks horrendous because code moving to new files can't be visualized well via github.
To make it easier to inspect the actual moving parts, I attached a script which uses
meld(you'll need tosudo apt install meld) to show the diff.This will show you the diff between the original
ethercat_driver.cppto the new place where the code was moved.The white parts are the same - so moved code. Blue and green are where changes happened - when it looks like lots of changes in one diff, it's largely because that part has moved to the other file (e.g. xml parser). This is also not perfect, but at least a bit better.
Script: just run from the repo root folder. meld_diff.sh
Logic changes beyond just code moving
Beyond moving code and necessities (naming, splitting to different functions) this PR adds the following minimal changes to the logic.
Everything else in the diff is API shape required by the extraction (touched on in next section below), not a behavior change.
Bug fix: propagate errors
activateBus()propagates errors fromsetupMaster()andconfigNetwork(). The pre-refactoron_activate()calledboth but ignored their return values; if
master_idorcontrol_frequencyparsing failed it logged FATAL and thensegfaulted in
master_->activate()on a null pointer. The new code returns failure asCallbackReturn::ERROR.Guard: rejection of double bus activation
configureModules()rejects re-entry when the bus is active. If called whileactivated_is true, it logs FATAL and returns false rather than clearing the internal state (if we want to add this feature later, we'll have to make sure the state is really properly cleaned, including going throughstop). When using ros2_control this should never be hit becauseon_initis only called once.Minor bug fix: empty transfer/fsoe config
Empty
transfer_config / fsoe_configvalues now fail earlier withCallbackReturn::ERROR. Before: the key being present triggered transfer loading, then an empty path would fail later inYAML::LoadFile. Providing bothtransfer_configandfsoe_confignow also fails via the early config conversion path (at the top ofon_init). Previously it failed later, insideloadTransferConfigYamlFile().Other (not real behavioral) changes
activation defaults to false
activated_is now initialized to false inEthercatBusManager(EthercatDriver::activated_was a raw with no initializer)New result type
read() / write()now return a typedEthercatCycleResult(kCompleted / kSkippedInactive / kSkippedBusy / kError). The driver wrapper translates everything exceptkErrortoreturn_type::OK, preserving the original semantics. Nothing returnskErrortoday, so the actual behavior is unchanged. The enum exists for future error paths.API decoupling from ros2_control
Bus configuration is a typed
EthercatBusConfigstruct (master_id, control_frequency, transfer_config, fsoe_config) rather than astd::unordered_map<std::string,std::string>of ros2_control hardware parameters.EthercatDriver::on_inittranslatesinfo_.hardware_parametersinto the struct before handing it to the manager. This allows for different structure of input (e.g. from a differently structured URDF/yaml).ConfiguredEcModulefield renames:state_interface → input_values,command_interface → output_values. Removesros2_control vocabulary from the manager's public data type to avoid confusion.
Logging
Logger channel renamed from "EthercatDriver" to "EthercatBusManager" for every call inside the bus manager.
Unused code removal
configTransferNetwork()no-op method deleted. It had an empty body in the pre-refactor code; transfer-net registration is now done inline in activateBus().Test removal
TestHelperEthercatSafetyDriversubclass deleted (testHelper_ethercat_safety_driver.hpp) as it wasn't needed any more: those methods are now pulbis onEthercatBusManager. The three tests now instantiateethercat_driver::EthercatBusManagerdriver; and calldriver.getEcTransferModuleParam(...) / driver.getEcTransferNets(...)directly. No actual runtime behavior change.Test plan
Run the unit tests!
Important
I used IgH
ethercatbranchstable-1.6for testingcolcon build --symlink-install --packages-up-to ethercat_driver colcon test --packages-select ethercat_driver --event-handlers console_direct+Expected: 9/9 tests pass
Then, test on the real HW 🤖 - just check that no regression happened (this is a code moving PR so it should be OK, but please do verify)