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

Replace ign common plugin loader with ign-plugin #38

Closed
wants to merge 25 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 19, 2020

This PR is related with this issue #18

I have some issue. When I run the tests for ign-sensors4 everything look good. Then problem is inside ign-gazebo code, when the library is loaded for some plugins is not able to find the pluginNames.

For example:

  • Camera

    • In the test: LoadLib() returns that the loaded plugin is N8ignition7sensors2v416SensorTypePluginINS1_12CameraSensorEEE -> ignition::sensors::v4::SensorTypePluginignition::sensors::v4::CameraSensor
    • Inside ign-gazebo code: the LoadLib() returns a empty structure. (No loaded plugins)
  • Depth Camera or GPU lidar are working properly.

Maybe @mxgrey or @scpeters can add some light here.

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 19, 2020
@ahcorde ahcorde requested a review from iche033 August 19, 2020 12:18
@ahcorde ahcorde self-assigned this Aug 19, 2020
@mxgrey
Copy link

mxgrey commented Aug 20, 2020

Off the top of my head, I can't think of why no plugins are showing up for ign-gazebo, but I would guess maybe something funny is going on with the linker. Does the problem occur on all platforms (Linux, Mac, Windows)?

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2020

I try to explain what I think it's happening here:

  • ign-gazebo is linking against camera from ign-sensor in the Sensor.cc. This is the CMakeLists.txt. When the program starts then the symbols from libignition-gazebo4-sensors-system.so are loaded which also includes libignition-sensors4-camera.so because as I explained before the CMake is linking with the sensor library.

  • This same file (Sensor.cc) is loading the camera plugin (libignition-sensors4-camera.so) using dlsym, In this line of code if there is a <sensor name="camera" type="camera"> in the sdf file. Here is where the problem arises

The Macro IGN_SENSORS_REGISTER_SENSOR(CameraSensor) is creating a struct in an anonymous namespace. Inside this namespace a static variable is created which is in charge of loading the symbols. The first time that this struct is generated is when the class is called (linked by the CMake) but the second time that we need to read the symbols (in the "createSensor" method) then this static variable is no being initialized again.

I have create a simple demo to reproduce this here.

The right example to hace a look is ./main_dl_link_cmake. This is doing exactely the same thing.

The hello library contains a struct and a static variable inside an anonymous namespace. In the main this library is linked with cmake and then is being loaded using dlsym(). The text is only printed once and it should be printed twice

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 21, 2020

I think I found a solution. If the symbols are already loaded, why we need to reload it again? There is no need to do it.

I added in the Manager class a method called AddSensor() 25be8d7 to add a sensor to the list of loaded sensors.

The I will use this class in ign-gazebo. Here it's the PR using this new method. gazebosim/gz-sim#304

@ahcorde ahcorde mentioned this pull request Aug 24, 2020
@scpeters scpeters changed the title [WIP] Replace ign common plugin loader with ign-plugin Replace ign common plugin loader with ign-plugin Aug 24, 2020
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
src/CameraSensor.cc Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
src/AltimeterSensor.cc Outdated Show resolved Hide resolved
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 14, 2020

waiting this PR #46 that fix the Github actions

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

It looks like plugins are not being loaded correctly on CI

@@ -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

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

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 ?

src/SensorFactory.cc Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 16, 2020

I noticed that the CI is not able to run the tests. But I'm not able to reproduce it in my machine. I will try on a docker container.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I noticed that the CI is not able to run the tests. But I'm not able to reproduce it in my machine.

I'm seeing the same failures locally, i.e.

$ ./build/ignition-sensors4/bin/INTEGRATION_imu_plugin
[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from ImuSensorTest
[ RUN      ] ImuSensorTest.CreateImu
[Err] [SensorFactory.cc:81] Unable to load sensor plugin file for [/home/chapulina/dome_ws/install/lib/libignition-sensors4-imu.so]
[Err] [SensorFactory.cc:166] Unable to instantiate sensor plugin for [ignition-sensors4-imu]
[Err] [SensorFactory.hh:133] Failed to create sensor of type[imu]
/home/chapulina/dome_ws/src/ign-sensors/test/integration/imu_plugin.cc:85: Failure
Value of: sensor != nullptr
  Actual: false
Expected: true
Segmentation fault (core dumped)

It looks like the shared library is generated and installed, but it doesn't have the correct symbols inside it.

src/SensorFactory.cc Show resolved Hide resolved
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

A couple more comments.

And great work cleaning up the Windows warnings! Those could even be pushed to a separate PR so they can get merged faster.

@@ -23,12 +23,20 @@

#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?

@@ -191,19 +195,14 @@ TEST(ImuSensor_TEST, CreateImuSensor)
sdf::ElementPtr imuSDF = ImuSensorToSDF(name, update_rate, topic,
accelNoise, gyroNoise, always_on, visualize);

// Create an ImuSensor
auto sensor = mgr.CreateSensor<ignition::sensors::ImuSensor>(imuSDF);
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't remove the CreateSensor tests, that function is still supported, right?

Instead, we should add separate tests for the Load functionality that you're using below.

@chapulina
Copy link
Contributor

Just talked to @ahcorde , we'll save this for Ign-E so we have more time to try and test the new approach. Removing it from the Dome beta.

@nkoenig nkoenig changed the base branch from main to ign-sensors5 December 23, 2020 23:35
@chapulina chapulina changed the base branch from ign-sensors5 to main February 1, 2021 17:14
@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Feb 3, 2021
@chapulina
Copy link
Contributor

This PR is outdated, it would be nice to sync it with the current main branch

chapulina added a commit that referenced this pull request Feb 5, 2021
@chapulina chapulina added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Mar 17, 2021
@chapulina
Copy link
Contributor

Removing beta label, we won't have time to wrap this up before code freeze. Let's retarget at Ignition-F.

@chapulina chapulina added 🏯 fortress Ignition Fortress and removed 🏢 edifice Ignition Edifice labels Apr 1, 2021
@chapulina
Copy link
Contributor

The plugin interface was completely removed on #90.

@chapulina chapulina closed this Aug 12, 2021
@chapulina chapulina deleted the ahcorde/replace/pluginload branch August 12, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants