From 81c2e4e0f50e16547194d6da1c44f1bbd6c9f0f1 Mon Sep 17 00:00:00 2001 From: Robert Wilbrandt Date: Thu, 14 May 2020 16:08:26 +0200 Subject: [PATCH 1/4] Remove inconsistent InterfaceManager manager registering behavior All InterfaceManager now handle registered InterfaceManagers transparently. This allows chains of multiple InterfaceManagers registered to each other to work corectly, mostly relevant for registering a manager from a combined_robot_hw RobotHW. Resolves #452 --- .../internal/interface_manager.h | 20 ++++ .../test/interface_manager_test.cpp | 93 +++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/hardware_interface/include/hardware_interface/internal/interface_manager.h b/hardware_interface/include/hardware_interface/internal/interface_manager.h index 9ab86c2f1..56b0f8a4a 100644 --- a/hardware_interface/include/hardware_interface/internal/interface_manager.h +++ b/hardware_interface/include/hardware_interface/internal/interface_manager.h @@ -219,6 +219,19 @@ class InterfaceManager { out.push_back(interface.first); } + + for (const auto& interface_manager : interface_managers_) + { + // Make sure interfaces appear only once, as they may have been combined + for (const auto& interface_name : interface_manager->getNames()) + { + if (std::find(out.begin(), out.end(), interface_name) == out.end()) + { + out.push_back(interface_name); + } + } + } + return out; } @@ -237,6 +250,13 @@ class InterfaceManager { out = it->second; } + + for (const auto& interface_manager : interface_managers_) + { + std::vector resources = interface_manager->getInterfaceResources(iface_type); + out.insert(out.end(), resources.begin(), resources.end()); + } + return out; } diff --git a/hardware_interface/test/interface_manager_test.cpp b/hardware_interface/test/interface_manager_test.cpp index a2213ead2..a2503e2b1 100644 --- a/hardware_interface/test/interface_manager_test.cpp +++ b/hardware_interface/test/interface_manager_test.cpp @@ -48,6 +48,22 @@ struct BazInterface double baz; }; +class TestCombinationHandle +{ +public: + TestCombinationHandle(const std::string& name) : name_(name) {} + + std::string getName() const + { + return name_; + } + +private: + std::string name_; +}; + +class TestCombinationInterface : public ResourceManager {}; + TEST(InterfaceManagerTest, InterfaceRegistration) { // Register interfaces @@ -84,6 +100,83 @@ TEST(InterfaceManagerTest, InterfaceRewriting) EXPECT_EQ(2, foo_iface_ptr->foo); } +TEST(InterfaceManagerTest, InterfaceCombination) +{ + // Create two InterfaceManagers, each containing a simple interface and a ResourceManager interface, + // and an additional simple interface for the second one + InterfaceManager root_mgr; + InterfaceManager leaf_mgr; + + FooInterface foo_root_iface(1); + root_mgr.registerInterface(&foo_root_iface); + FooInterface foo_leaf_iface(2); + leaf_mgr.registerInterface(&foo_leaf_iface); + + BazInterface baz_leaf_iface; + baz_leaf_iface.baz = 4.2; + leaf_mgr.registerInterface(&baz_leaf_iface); + + TestCombinationInterface combi_root_iface; + TestCombinationHandle combi_root_handle("combi_root_handle"); + combi_root_iface.registerHandle(combi_root_handle); + root_mgr.registerInterface(&combi_root_iface); + + TestCombinationInterface combi_leaf_iface; + TestCombinationHandle combi_leaf_handle("combi_leaf_handle"); + combi_leaf_iface.registerHandle(combi_leaf_handle); + leaf_mgr.registerInterface(&combi_leaf_iface); + + // Now register leaf_mgr to root_mgr + root_mgr.registerInterfaceManager(&leaf_mgr); + + // Querying FooInterface should not work anymore, as this would require combining the interfaces + FooInterface* foo_iface_ptr = root_mgr.get(); + EXPECT_EQ(nullptr, foo_iface_ptr); + + // BazInterface should still work, as there is only one interface of this type + BazInterface* baz_iface_ptr = root_mgr.get(); + EXPECT_NE(nullptr, baz_iface_ptr); + if (baz_iface_ptr != nullptr) // Don't crash on error + { + EXPECT_EQ(4.2, baz_iface_ptr->baz); + } + + // Check presence of handles in combined interface + TestCombinationInterface* combi_iface_ptr = root_mgr.get(); + EXPECT_NE(nullptr, combi_iface_ptr); + if (combi_iface_ptr != nullptr) // Don't crash on error + { + std::vector combi_handle_names = combi_iface_ptr->getNames(); + EXPECT_EQ(2, combi_handle_names.size()); + EXPECT_NE(combi_handle_names.end(), std::find(combi_handle_names.begin(), combi_handle_names.end(), "combi_root_handle")) + << "Did not find handle 'combi_root_handle' in combined interface"; + EXPECT_NE(combi_handle_names.end(), std::find(combi_handle_names.begin(), combi_handle_names.end(), "combi_leaf_handle")) + << "Did not find handle 'combi_leaf_handle' in combined interface"; + } + + // Check presence of interfaces in combined interface + std::vector iface_names = root_mgr.getNames(); + EXPECT_EQ(3, iface_names.size()); + EXPECT_NE(iface_names.end(), std::find(iface_names.begin(), iface_names.end(), "FooInterface")) + << "Did not find interface 'FooInterface' in combined interface"; + EXPECT_NE(iface_names.end(), std::find(iface_names.begin(), iface_names.end(), "BazInterface")) + << "Did not find interface 'BazInterface' in combined interface"; + EXPECT_NE(iface_names.end(), std::find(iface_names.begin(), iface_names.end(), "TestCombinationInterface")) + << "Did not find interface 'TestCombinationInterface' in combined interface"; + + // Check presence of resources in combined interface + std::vector iface_res = root_mgr.getInterfaceResources("FooInterface"); + EXPECT_EQ(0, iface_res.size()); + iface_res = root_mgr.getInterfaceResources("BazInterface"); + EXPECT_EQ(0, iface_res.size()); + iface_res = root_mgr.getInterfaceResources("TestCombinationInterface"); + EXPECT_EQ(2, iface_res.size()); + EXPECT_NE(iface_res.end(), std::find(iface_res.begin(), iface_res.end(), "combi_root_handle")) + << "Did not find interface resource 'combi_root_handle' for interface 'TestCombinationInterface' in combined interface"; + EXPECT_NE(iface_res.end(), std::find(iface_res.begin(), iface_res.end(), "combi_leaf_handle")) + << "Did not find interface resource 'combi_leaf_handle' for interface 'TestCombinationInterface' in combined interface"; +} + int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); From 8b0403d31f100146f9c28039b6fed053de551c39 Mon Sep 17 00:00:00 2001 From: Robert Wilbrandt Date: Thu, 14 May 2020 16:24:39 +0200 Subject: [PATCH 2/4] Removed duplicate error message Previously, trying to combine two non-ResourceManager interfaces yielded two identical error messages. --- .../include/hardware_interface/internal/interface_manager.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hardware_interface/include/hardware_interface/internal/interface_manager.h b/hardware_interface/include/hardware_interface/internal/interface_manager.h index 56b0f8a4a..ff8553fad 100644 --- a/hardware_interface/include/hardware_interface/internal/interface_manager.h +++ b/hardware_interface/include/hardware_interface/internal/interface_manager.h @@ -200,10 +200,8 @@ class InterfaceManager interfaces_combo_[type_name] = iface_combo; num_ifaces_registered_[type_name] = iface_list.size(); } else { - // it is not a ResourceManager - ROS_ERROR("You cannot register multiple interfaces of the same type which are " - "not of type ResourceManager. There is no established protocol " - "for combining them."); + // Multiple interfaces of the same type which are not ResourceManager cannot be combined, so return + // nullptr iface_combo = nullptr; } } From 5488cf14e62bc179084a214d40e1e15864d060c9 Mon Sep 17 00:00:00 2001 From: Robert Wilbrandt Date: Thu, 14 May 2020 17:02:17 +0200 Subject: [PATCH 3/4] Updated InterfaceManager documentation --- .../internal/interface_manager.h | 57 ++++++++++++++++--- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/hardware_interface/include/hardware_interface/internal/interface_manager.h b/hardware_interface/include/hardware_interface/internal/interface_manager.h index ff8553fad..0a61fef85 100644 --- a/hardware_interface/include/hardware_interface/internal/interface_manager.h +++ b/hardware_interface/include/hardware_interface/internal/interface_manager.h @@ -110,6 +110,15 @@ struct CheckIsResourceManager { } // namespace internal +/** + * \brief Manager for hardware interface registrations. + * + * This class enables the registration of interfaces based on their class type, + * handling all the required demangling and storage. The registration ensures + * the presence of at most one interface instance per type. Accessors for + * interface listing are provided. Additionally, combinations of interfaces as + * required in \c CombinedRobotHW are handled transparently. + */ class InterfaceManager { public: @@ -119,6 +128,9 @@ class InterfaceManager * This associates the name of the type of interface to be registered with * the given pointer. * + * \note The registration of an interface will replace previously registered + * instances of its type + * * \tparam T The interface type * \param iface A pointer to the interface to store */ @@ -134,6 +146,14 @@ class InterfaceManager internal::CheckIsResourceManager::callGetResources(resources_[iface_name], iface); } + /** + * \brief Register another interface manager. + * + * This manager will be integrated transparently into all further access + * methods. + * + * \param iface_man A pointer to the interface manager to store + */ void registerInterfaceManager(InterfaceManager* iface_man) { interface_managers_.push_back(iface_man); @@ -142,12 +162,23 @@ class InterfaceManager /** * \brief Get an interface. * - * Since this class only stores one interface per type, this returns a - * pointer to the requested interface type. If the interface type is not - * registered, it will return \c NULL. + * If this class and its registered sub-managers only have one registered + * instance of the requested type, this will be returned. If multiple + * instances of the interface type were registered and the type is a + * \c ResourceManager, this call will try to combine them into a single + * handle. In all other cases, \c nullptr will be returned. + * + * \note As this instance will only store one instance of each manager type, + * the only way multiple interfaces are registered is the registration of + * other managers with interfaces of the same type. + * + * \note If there are multiple registered interfaces of the requested type + * (either directly or through registered interfaces) and they are not + * \c ResourceManager, this method will not be able to combine them and will + * return \c nullptr. * * \tparam T The interface type - * \return A pointer to the stored interface of type \c T or \c NULL + * \return A handle for the stored interfaces of type \c T or \c nullptr */ template T* get() @@ -208,7 +239,15 @@ class InterfaceManager return iface_combo; } - /** \return Vector of interface names registered to this instance. */ + /** + * \brief Lists the demangled names of all registered interfaces. + * + * This includes the interfaces directly registered to this instance and + * the interfaces registered to registered managers. As multiple interfaces + * of the same type will get merged on access, duplicates are omitted. + * + * \return Vector of demangled interface names of registered interfaces. + **/ std::vector getNames() const { std::vector out; @@ -234,10 +273,12 @@ class InterfaceManager } /** - * \brief Get the resource names registered to an interface, specified by type - * (as this class only stores one interface per type) + * \brief Get the resource names registered to an interface, specified by type. + * + * This will return the list of all registered resources for + * \c ResourceManager interfaces and an empty list for all others. * - * \param iface_type A string with the demangled type name of the interface + * \param iface_type The demangled type name of an interface * \return A vector of resource names registered to this interface */ std::vector getInterfaceResources(std::string iface_type) const From 04bcb42a80da3b7f6e8dc498532723ab8ba00fee Mon Sep 17 00:00:00 2001 From: Robert Wilbrandt Date: Sun, 13 Sep 2020 20:40:26 +0200 Subject: [PATCH 4/4] Clarified documentation for InterfaceManager sub-manager handling --- .../internal/interface_manager.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/hardware_interface/include/hardware_interface/internal/interface_manager.h b/hardware_interface/include/hardware_interface/internal/interface_manager.h index 0a61fef85..a60c9dad8 100644 --- a/hardware_interface/include/hardware_interface/internal/interface_manager.h +++ b/hardware_interface/include/hardware_interface/internal/interface_manager.h @@ -163,19 +163,18 @@ class InterfaceManager * \brief Get an interface. * * If this class and its registered sub-managers only have one registered - * instance of the requested type, this will be returned. If multiple - * instances of the interface type were registered and the type is a + * instance of the requested interface type, this will be returned. If + * multiple instances of the interface type were registered and the type is a * \c ResourceManager, this call will try to combine them into a single * handle. In all other cases, \c nullptr will be returned. * - * \note As this instance will only store one instance of each manager type, - * the only way multiple interfaces are registered is the registration of - * other managers with interfaces of the same type. + * \note As this class can only store one instance of each interface type, + * the only way multiple instances of the same interface type can be + * registered is through the registration of sub-managers. * * \note If there are multiple registered interfaces of the requested type - * (either directly or through registered interfaces) and they are not - * \c ResourceManager, this method will not be able to combine them and will - * return \c nullptr. + * and they are not \c ResourceManager, this method will not be able to + * combine them and will return \c nullptr. * * \tparam T The interface type * \return A handle for the stored interfaces of type \c T or \c nullptr