Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix robotinterface plugin crashes by closing it as soon as one of its devices is destroyed #186

Merged
merged 3 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion libraries/device-registry/DeviceRegistry.cc
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <DeviceRegistry.hh>

#include <cstddef>
#include <gz/sim/Entity.hh>
#include <gz/sim/EntityComponentManager.hh>
#include <gz/sim/Util.hh>

#include <cstddef>
#include <iostream>
#include <mutex>
#include <ostream>
Expand Down Expand Up @@ -242,6 +243,7 @@ bool DeviceRegistry::removeDevice(const gz::sim::EntityComponentManager& ecm,
return false;
}

m_deviceRemovedEvent(deviceDatabaseKey);
m_devicesMap.at(gzInstanceId).erase(deviceDatabaseKey);
}
}
Expand Down
16 changes: 15 additions & 1 deletion libraries/device-registry/DeviceRegistry.hh
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#pragma once

#include <gz/common/Event.hh>
#include <gz/common/events/Types.hh>
#include <gz/sim/Entity.hh>

#include <mutex>
#include <string>
#include <unordered_map>
Expand Down Expand Up @@ -37,6 +40,15 @@ public:
bool
removeDevice(const gz::sim::EntityComponentManager& ecm, const std::string& deviceDatabaseKey);

/**
* Add a callback when a device is removed, i.e. removeDevice is called.
*/
template<typename T>
gz::common::ConnectionPtr connectDeviceRemoved(T _subscriber)
{
return m_deviceRemovedEvent.Connect(_subscriber);
}

std::vector<std::string> getDevicesKeys(const gz::sim::EntityComponentManager& ecm) const;

private:
Expand All @@ -54,7 +66,9 @@ private:
static DeviceRegistry* s_handle;
static std::mutex& mutex();
std::unordered_map<std::string, std::unordered_map<std::string, yarp::dev::PolyDriver*>>
m_devicesMap; // map of known yarp devices
m_devicesMap; // map of known yarp devices Updated upstream
// Event for when a device is removed
gz::common::EventT<void (std::string)> m_deviceRemovedEvent;
};

} // namespace gzyarp
22 changes: 22 additions & 0 deletions plugins/robotinterface/RobotInterface.cc
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#include <ConfigurationHelpers.hh>
#include <DeviceRegistry.hh>

#include <functional>
#include <memory>
#include <sdf/Element.hh>
#include <string>
#include <vector>

#include <gz/common/Event.hh>
#include <gz/common/events/Types.hh>
#include <gz/plugin/Register.hh>
#include <gz/sim/Entity.hh>
#include <gz/sim/EntityComponentManager.hh>
Expand Down Expand Up @@ -62,10 +65,23 @@ class RobotInterface : public System, public ISystemConfigure
yError() << "gz-sim-yarp-robotinterface-system: impossible to run phase "
"ActionPhaseShutdown in robotinterface";
}
m_connection.reset();
m_robotInterfaceCorrectlyStarted = false;
}
}

void OnDeviceRemoved(std::string removeDeviceRegistryDatabaseKey)
{
// Check if deviceRegistryDatabaseKey is among the one passed to this instance of gz_yarp_robotinterface
// If yes, close the robotinterface to avoid crashes due to access to a device that is being deleted
for (auto&& usedDeviceScopedName: m_deviceScopedNames) {
if (removeDeviceRegistryDatabaseKey == usedDeviceScopedName) {
CloseRobotInterface();
}
}
return;
}

virtual void Configure(const Entity& _entity,
const std::shared_ptr<const sdf::Element>& _sdf,
EntityComponentManager& _ecm,
Expand Down Expand Up @@ -105,11 +121,17 @@ class RobotInterface : public System, public ISystemConfigure
return;
}
m_robotInterfaceCorrectlyStarted = true;
// If the robotinterface started correctly, add a callback to ensure that it is closed as
// soon that an external device passed to it is deleted
m_connection =
DeviceRegistry::getHandler()->connectDeviceRemoved(
std::bind(&RobotInterface::OnDeviceRemoved, this, std::placeholders::_1));
}

private:
yarp::robotinterface::XMLReaderResult m_xmlRobotInterfaceResult;
std::vector<std::string> m_deviceScopedNames;
gz::common::ConnectionPtr m_connection;
bool m_robotInterfaceCorrectlyStarted;

bool loadYarpRobotInterfaceConfigurationFile(const std::shared_ptr<const sdf::Element>& _sdf,
Expand Down
Loading