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.

Sign up for GitHub

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

Jump to bottom

Replace ign common plugin loader with ign-plugin #38

Closed
wants to merge 25 commits into from
Closed

Replace ign common plugin loader with ign-plugin #38

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a027783
Replace ign common plugin loader with ign-plugin
ahcorde Aug 19, 2020
50feecc
Updated includes
ahcorde Aug 21, 2020
25be8d7
Added AddSensor method to Manager
ahcorde Aug 21, 2020
21badd7
Merge branch 'master' into ahcorde/replace/pluginload
ahcorde Aug 24, 2020
b78594d
make linters happy
ahcorde Aug 27, 2020
c5d0cdb
make linters happy
ahcorde Aug 27, 2020
f3dfca3
Removed unnecessary code
ahcorde Sep 3, 2020
a0a0e65
Fixed headers
ahcorde Sep 3, 2020
dd97599
Merge branch 'master' into ahcorde/replace/pluginload
chapulina Sep 14, 2020
5b4b7a9
Fixed windows build error
ahcorde Sep 18, 2020
b69410c
Fixed tests
ahcorde Sep 18, 2020
6da3f22
Fixed integration tests
ahcorde Sep 18, 2020
f233366
Removed windowns warnings
ahcorde Sep 19, 2020
6e5c4e9
Removed windows warnings
ahcorde Sep 19, 2020
161765c
make linters happy
ahcorde Sep 19, 2020
acc0084
Removed C4251 and C4146 warnings on windows
ahcorde Sep 19, 2020
893b79c
Removed C4251 and C4146 warnings - Integration test - windows
ahcorde Sep 19, 2020
0a6de53
Removed warning C4068 on windows
ahcorde Sep 19, 2020
74f3c71
Removed C4005 warning
ahcorde Sep 19, 2020
947361e
make linters happy
ahcorde Sep 19, 2020
0a03653
Removed C4005 Windows warnings
ahcorde Sep 21, 2020
151415b
Remove more windows warnings
ahcorde Sep 21, 2020
69ba811
Improved WaitForMessageTestHelper - test
ahcorde Sep 21, 2020
c15ad7c
make linters happy
ahcorde Sep 21, 2020
7448fbe
Merge branch 'main' into ahcorde/replace/pluginload
chapulina Nov 11, 2020
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
1 change: 0 additions & 1 deletion include/ignition/sensors/CameraSensor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

#include <sdf/sdf.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
1 change: 0 additions & 1 deletion include/ignition/sensors/DepthCameraSensor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

#include <ignition/common/Event.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
2 changes: 1 addition & 1 deletion include/ignition/sensors/LogicalCameraSensor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

#include <sdf/sdf.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/plugin/RegisterMore.hh>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other plugins have RegisterMore on their headers, can it be removed here?

#include <ignition/common/Time.hh>

#include <ignition/math/Angle.hh>
Expand Down
8 changes: 8 additions & 0 deletions include/ignition/sensors/Manager.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ namespace ignition
/// is returned on erro.
public: ignition::sensors::SensorId CreateSensor(sdf::ElementPtr _sdf);

/// \brief Add a sensor from a sensor instance.
/// \sa Sensor()
/// \param[in] _sensor pointer to the sensor
/// \return A sensor id that refers to the created sensor. NO_SENSOR
/// is returned on error.
public: ignition::sensors::SensorId AddSensor(
std::unique_ptr<sensors::Sensor> _sensor);

/// \brief Create a sensor from SDF without a known sensor type.
///
/// This creates sensors by looking at the given sdf element.
Expand Down
7 changes: 2 additions & 5 deletions include/ignition/sensors/SensorFactory.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include <sdf/sdf.hh>

#include <ignition/common/Console.hh>
#include <ignition/common/PluginMacros.hh>
#include <ignition/plugin/Loader.hh>

#include <ignition/sensors/config.hh>
#include <ignition/sensors/Export.hh>
Expand All @@ -42,9 +42,6 @@ namespace ignition
/// \brief Base sensor plugin interface
class IGNITION_SENSORS_VISIBLE SensorPlugin
{
/// \brief Allows using shorter APIS in common::PluginLoader
public: IGN_COMMON_SPECIALIZE_INTERFACE(ignition::sensors::SensorPlugin)

/// \brief Instantiate new sensor
/// \return New sensor
public: virtual Sensor *New() = 0;
Expand Down Expand Up @@ -182,7 +179,7 @@ namespace ignition

/// \brief Sensor registration macro
#define IGN_SENSORS_REGISTER_SENSOR(classname) \
IGN_COMMON_REGISTER_SINGLE_PLUGIN(\
IGNITION_ADD_PLUGIN(\
ignition::sensors::SensorTypePlugin<classname>, \
ignition::sensors::SensorPlugin)
}
Expand Down
1 change: 0 additions & 1 deletion include/ignition/sensors/ThermalCameraSensor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

#include <ignition/common/Event.hh>

#include <ignition/common/PluginMacros.hh>
#include <ignition/common/Time.hh>

#include <ignition/msgs.hh>
Expand Down
2 changes: 2 additions & 0 deletions src/AirPressureSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <ignition/common/Profiler.hh>
#include <ignition/transport/Node.hh>

#include <ignition/plugin/Register.hh>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this include needed at every sensor? Shouldn't SensorFactory.hh be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According with the migration guide https://github.com/ignitionrobotics/ign-plugin/blob/94482af3e9b2cc1cbb7c2dbeb2c7846f8283f0f6/MIGRATION.md#registering-a-plugin

I added this header for registering plugins. All sensors are individual components that why I need to use ignition/plugin/Register.hh in the cc file. I tried to include this header (or #include <ignition/plugin/RegisterMore.hh>) in the SensorFactory.hh but I was not able to compile the code because an undefined reference:

``/usr/bin/ld: ../lib/libignition-sensors4-imu.so.4.0.0~pre1: undefined reference `IgnitionPluginHook'

Copy link
Contributor

@chapulina chapulina Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According with the migration guide

I see. I mean, we'd still be including that header, just indirectly. The way ign-sensors is exposing the plugin functionality is through SensorFactory. So technically each sensor doesn't need to know about ign-plugins, they just know they need to use the IGN_SENSORS_REGISTER_SENSOR macro from the SensorFactory.hh header.

I tried to include this header (or #include <ignition/plugin/RegisterMore.hh>) in the SensorFactory.hh but I was not able to compile the code because an undefined reference:

SensorFactory.hh uses the IGNITION_ADD_PLUGIN macro from ignition/plugin/Register.hh, so I believe it should include that header. Otherwise, that header is not usable on its own.

And it probably needs to link against ignition-plugin${IGN_PLUGIN_VER}::register to fix the undefined reference. I believe that if you link to it publicly, then each individual sensor won't need to also link against register, only against ign-sensors's core library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chapulina, I was trying your suggestion but I'm not able to compile the code. I was having a look to other packages that make use of ignition-plugin and the approach it's similar to the one that I used here. For example:

Copy link
Contributor

@chapulina chapulina Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's similar to the one that I used here

I think the key difference here is that the IGNITION_ADD_PLUGIN macro is used in SensorFactory, not in each plugin. All the examples you gave use that macro directly on the plugin files.

Does SensorFactory.hh work if you don't include Register.hh above it? If it doesn't, that sounds like an architecture flaw. A header should work when included by itself, without the need for extra includes above it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All plugin are using the MACRO but it's hidden under this other one IGN_SENSORS_REGISTER_SENSOR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chapulina I can remove IGN_SENSORS_REGISTER_SENSOR and make use of `IGNITION_ADD_PLUGIN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove IGN_SENSORS_REGISTER_SENSOR and make use of `IGNITION_ADD_PLUGIN.

That sounds like a big migration effort for users. But since our support for custom sensors is not really there yet (#9), maybe this isn't a big deal. I'd be ok with it. What do you think, @iche033 ?


#include "ignition/sensors/GaussianNoiseModel.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorTypes.hh"
Expand Down
5 changes: 3 additions & 2 deletions src/AltimeterSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@
*/

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>

#include "ignition/sensors/AltimeterSensor.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/AltimeterSensor.hh"
#include "ignition/sensors/SensorTypes.hh"

using namespace ignition;
using namespace sensors;
Expand Down
20 changes: 20 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ target_link_libraries(${PROJECT_LIBRARY_TARGET_NAME}
ignition-math${IGN_MATH_VER}::ignition-math${IGN_MATH_VER}
ignition-common${IGN_COMMON_VER}::ignition-common${IGN_COMMON_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
ignition-plugin${IGN_PLUGIN_VER}::loader
sdformat${SDF_VER}::sdformat${SDF_VER}
PRIVATE
ignition-common${IGN_COMMON_VER}::profiler
Expand Down Expand Up @@ -47,6 +48,7 @@ target_link_libraries(${camera_target}
PUBLIC
${rendering_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)
Expand All @@ -68,6 +70,7 @@ target_link_libraries(${lidar_target}
PUBLIC
${rendering_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)
Expand All @@ -87,21 +90,38 @@ ign_add_component(logical_camera SOURCES ${logical_camera_sources} GET_TARGET_NA
target_compile_definitions(${logical_camera_target} PUBLIC LogicalCameraSensor_EXPORTS)
target_link_libraries(${logical_camera_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
ignition-msgs${IGN_MSGS_VER}::ignition-msgs${IGN_MSGS_VER}
ignition-transport${IGN_TRANSPORT_VER}::ignition-transport${IGN_TRANSPORT_VER}
)

set(magnetometer_sources MagnetometerSensor.cc)
ign_add_component(magnetometer SOURCES ${magnetometer_sources} GET_TARGET_NAME magnetometer_target)
target_link_libraries(${magnetometer_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(imu_sources ImuSensor.cc)
ign_add_component(imu SOURCES ${imu_sources} GET_TARGET_NAME imu_target)
target_link_libraries(${imu_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(altimeter_sources AltimeterSensor.cc)
ign_add_component(altimeter SOURCES ${altimeter_sources} GET_TARGET_NAME altimeter_target)
target_link_libraries(${altimeter_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(air_pressure_sources AirPressureSensor.cc)
ign_add_component(air_pressure SOURCES ${air_pressure_sources} GET_TARGET_NAME air_pressure_target)
target_link_libraries(${air_pressure_target}
PRIVATE
ignition-plugin${IGN_PLUGIN_VER}::register
)

set(rgbd_camera_sources RgbdCameraSensor.cc)
ign_add_component(rgbd_camera SOURCES ${rgbd_camera_sources} GET_TARGET_NAME rgbd_camera_target)
Expand Down
4 changes: 2 additions & 2 deletions src/CameraSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
#include <ignition/common/Profiler.hh>
#include <ignition/common/StringUtils.hh>
#include <ignition/math/Angle.hh>
#include <ignition/transport/Node.hh>

#include <ignition/math/Helpers.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>

#include "ignition/sensors/CameraSensor.hh"
#include "ignition/sensors/ImageGaussianNoiseModel.hh"
Expand Down
6 changes: 3 additions & 3 deletions src/Camera_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
#include <gtest/gtest.h>
#include <sdf/sdf.hh>

#include <ignition/sensors/Export.hh>
#include <ignition/sensors/CameraSensor.hh>
#include <ignition/sensors/Manager.hh>
#include "ignition/sensors/CameraSensor.hh"
#include "ignition/sensors/Export.hh"
#include "ignition/sensors/Manager.hh"

sdf::ElementPtr cameraToBadSdf()
{
Expand Down
2 changes: 2 additions & 0 deletions src/DepthCameraSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include <ignition/math/Angle.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include <ignition/transport/Node.hh>

#include "ignition/sensors/DepthCameraSensor.hh"
Expand Down
7 changes: 5 additions & 2 deletions src/GpuLidarSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@
* limitations under the License.
*
*/

#include <ignition/msgs/pointcloud_packed.pb.h>
#include <ignition/msgs/Utility.hh>
#include <ignition/transport/Node.hh>

#include <ignition/common/Console.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/msgs/Utility.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>

#include "ignition/sensors/GpuLidarSensor.hh"
#include "ignition/sensors/SensorFactory.hh"

Expand Down
5 changes: 3 additions & 2 deletions src/ImuSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@
* limitations under the License.
*
*/

#include <ignition/msgs/imu.pb.h>

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>

#include "ignition/sensors/ImuSensor.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/ImuSensor.hh"

using namespace ignition;
using namespace sensors;
Expand Down
10 changes: 3 additions & 7 deletions src/ImuSensor_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@

#include <ignition/math/Helpers.hh>
#include <ignition/msgs.hh>
#include <ignition/sensors/Export.hh>
#include <ignition/sensors/Manager.hh>

#include <ignition/sensors/ImuSensor.hh>
#include "ignition/sensors/Export.hh"
#include "ignition/sensors/ImuSensor.hh"
#include "ignition/sensors/Manager.hh"

using namespace ignition;

Expand Down Expand Up @@ -297,7 +297,3 @@ int main(int argc, char **argv)
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}




3 changes: 2 additions & 1 deletion src/Lidar.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@
#include <ignition/common/Console.hh>
#include <ignition/common/Event.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>
#include <sdf/Lidar.hh>

#include "ignition/sensors/GaussianNoiseModel.hh"
#include "ignition/sensors/Lidar.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/GaussianNoiseModel.hh"

using namespace ignition::sensors;

Expand Down
7 changes: 3 additions & 4 deletions src/Lidar_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@
#include <ignition/math/Angle.hh>
#include <ignition/math/Helpers.hh>
#include <ignition/msgs.hh>
#include <ignition/sensors/Export.hh>
#include <ignition/sensors/Manager.hh>

#include <ignition/sensors/Lidar.hh>

#include "ignition/sensors/Export.hh"
#include "ignition/sensors/Manager.hh"
#include "ignition/sensors/Lidar.hh"

sdf::ElementPtr LidarToSDF(const std::string &name, double update_rate,
const std::string &topic, double horz_samples, double horz_resolution,
Expand Down
2 changes: 2 additions & 0 deletions src/LogicalCameraSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <ignition/math/Frustum.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/LogicalCameraSensor.hh"

Expand Down
4 changes: 3 additions & 1 deletion src/MagnetometerSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@
*/

#include <ignition/msgs/magnetometer.pb.h>

#include <ignition/common/Profiler.hh>
#include <ignition/plugin/Register.hh>
#include <ignition/transport/Node.hh>
#include <sdf/Magnetometer.hh>

#include "ignition/sensors/MagnetometerSensor.hh"
#include "ignition/sensors/Noise.hh"
#include "ignition/sensors/SensorFactory.hh"
#include "ignition/sensors/SensorTypes.hh"
#include "ignition/sensors/MagnetometerSensor.hh"

using namespace ignition;
using namespace sensors;
Expand Down
14 changes: 12 additions & 2 deletions src/Manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
#include "ignition/sensors/Manager.hh"
#include <memory>
#include <unordered_map>
#include <ignition/common/PluginLoader.hh>
#include <ignition/common/Plugin.hh>
#include <ignition/plugin/Loader.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/common/SystemPaths.hh>
#include <ignition/common/Console.hh>
Expand Down Expand Up @@ -101,6 +100,17 @@ void Manager::RunOnce(const ignition::common::Time &_time, bool _force)
}
}

/////////////////////////////////////////////////
ignition::sensors::SensorId Manager::AddSensor(
std::unique_ptr<sensors::Sensor> _sensor)
{
if (!_sensor)
return NO_SENSOR;
SensorId id = _sensor->Id();
this->dataPtr->sensors[id] = std::move(_sensor);
return id;
}

/////////////////////////////////////////////////
ignition::sensors::SensorId Manager::CreateSensor(const sdf::Sensor &_sdf)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Manager_TEST.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
*/

#include <gtest/gtest.h>
#include <ignition/sensors/Manager.hh>

#include "ignition/sensors/Manager.hh"

//////////////////////////////////////////////////
TEST(Manager, construct)
Expand Down
5 changes: 3 additions & 2 deletions src/Noise.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@

#include <functional>

#include <ignition/sensors/Noise.hh>
#include <ignition/common/Console.hh>
#include <ignition/sensors/GaussianNoiseModel.hh>

#include "ignition/sensors/GaussianNoiseModel.hh"
#include "ignition/sensors/Noise.hh"

using namespace ignition;
using namespace sensors;
Expand Down
2 changes: 2 additions & 0 deletions src/RgbdCameraSensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include <ignition/common/Profiler.hh>
#include <ignition/math/Helpers.hh>

#include <ignition/plugin/Register.hh>

#include <ignition/rendering/Camera.hh>
#include <ignition/rendering/DepthCamera.hh>
#include <ignition/transport/Node.hh>
Expand Down
3 changes: 2 additions & 1 deletion src/Sensor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
#include "ignition/sensors/Sensor.hh"
#include <map>
#include <vector>
#include <ignition/sensors/Manager.hh>
#include <ignition/common/Console.hh>
#include <ignition/common/Profiler.hh>
#include <ignition/transport/TopicUtils.hh>

#include "ignition/sensors/Manager.hh"

using namespace ignition::sensors;


Expand Down
Loading