Declare the hardware_interface dependency explicitly - #39
Merged
Conversation
Both controllers include <hardware_interface/loaned_command_interface.hpp> and
name hardware_interface::LoanedCommandInterface directly, but neither declared
hardware_interface in package.xml or found it in CMake. They relied on
controller_interface to pull it in transitively:
picknik_*_controller -> controller_interface -> hardware_interface
That works today and is not a build break, but it means a dependency we use
directly is invisible to rosdep and to anyone reading the manifest, and it would
break silently if controller_interface ever stopped re-exporting it.
Adds, for both packages:
- <depend>hardware_interface</depend> in package.xml
- find_package(hardware_interface REQUIRED)
- hardware_interface in THIS_PACKAGE_INCLUDE_DEPENDS, so it is re-exported to
consumers alongside the other direct deps
- ${hardware_interface_TARGETS} in target_link_libraries, matching the
${..._TARGETS} style already used for controller_interface. That variable
resolves to hardware_interface::hardware_interface only -- mock_components
lives in a separate export set and is not pulled in.
Reported by @Plumezz in #36 against the humble branch; the same gap is present
on main and on humble, and both packages are affected, not just
picknik_reset_fault_controller.
Verified by building both packages from a clean workspace in a
ros:lyrical-ros-base container: rosdep resolves the new dependency and colcon
build succeeds.
Fixes #36
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #36.
Why
Both controllers use
hardware_interfacedirectly:picknik_reset_fault_controllersrc/…:26#include "hardware_interface/loaned_command_interface.hpp"using hardware_interface::LoanedCommandInterface;picknik_twist_controllersrc/…:31same headerNeither declared
hardware_interfaceinpackage.xml, and neitherfind_packaged it. They relied on it arriving transitively:As @Plumezz noted in #36, this is not a build break today. It's a metadata problem: a dependency we use directly is invisible to rosdep and to anyone reading the manifest, and it would break silently if
controller_interfaceever stopped re-exporting it.Two additions to the original report: the gap is on
mainas well ashumble, and it affects both packages, not justpicknik_reset_fault_controller.How
For each package:
<depend>hardware_interface</depend>inpackage.xmlfind_package(hardware_interface REQUIRED)hardware_interfaceadded toTHIS_PACKAGE_INCLUDE_DEPENDS, so it's re-exported to consumers alongside the other direct deps${hardware_interface_TARGETS}intarget_link_libraries, matching the${..._TARGETS}style already used forcontroller_interfaceOn the last point — I checked that
hardware_interface_TARGETSresolves tohardware_interface::hardware_interfaceonly.mock_componentsis in a separate export set (export_mock_components) and is not pulled in, so this doesn't widen the link beyond what's intended.8 lines added, nothing removed.
Verification
Built both packages from a clean workspace in a
ros:lyrical-ros-basecontainer:rosdep installresolves the newly-declared dependency, andcolcon buildsucceeds for both.Separate finding: 12 ignored
set_value()return valuesNot fixed here, since it needs a behaviour decision rather than a metadata one — but the build surfaces it, so worth recording.
LoanedCommandInterface::set_value()is[[nodiscard]]on lyrical and returnsfalseif it cannot acquire the lock withinmax_tries(default 10). Both controllers discard it at every call site — 12-Wunused-resultwarnings, 6 per package:picknik_reset_fault_controller.cpp: 87, 88, 113, 114, 122, 123picknik_twist_controller.cpp: 148–153So both controllers can currently drop a command write silently. Note the read path was already modernised —
get_optional()withvalue_or(...)— so this is the write half of the same API migration left unfinished. Worth its own issue; happy to file it and take it if you want.Sequencing
Branched off
main, so until #38 merges this PR only exercises the currently-active workflows. Once #38 is in I'll rebase so it gets the full matrix, including the blockinglyrical-mainjob.The same fix is wanted on the
humblebranch, which has the identical gap in both packages. Say the word and I'll open that as a companion PR.