search

osrf / rmf_demos

Demos to showcase the capabilities of RMF
Apache License 2.0
69 stars 38 forks source link

Create Ignition plugins for TeleportIngestor and TeleportDispenser and refactor with existing Gazebo plugins #124

Closed mrushyendra closed 4 years ago

mrushyendra commented 4 years ago

Creates new TeleportIngestorPlugin and TeleportDispenserPlugin for Ignition. Tested with the Office Demo.

Refactors both the Ignition and Gazebo plugins in order to move common state variables, callbacks and ROS message publishers/subscribers into common TeleportIngestorCommon and TeleportDispenserCommon classes. For the existing Gazebo plugins, this involves setting new flags when Ingestor/Dispenser requests are made, and moving the actual ingesting/dispensing to the on_update() calls instead.

luca-della-vedova commented 4 years ago

Can you try to merge / rebase to the latest master? Would like to check if github actions run for the linter

mrushyendra commented 4 years ago

Sure, will do in the next few days. This might take quite a while because of recent changes to master.

luca-della-vedova commented 4 years ago

I merged the latest master to reduce the size of the diff, can you rebase on top of the latest feature/add_ignition_support?

We should probably just have opened a PR for that branch into master anyway, will do after this gets in.

mrushyendra commented 4 years ago

I further refactored the code and addressed the review comments. The code is structured such that the main ingesting/dispensing control flow, message passing and state variables are all contained in TeleportDispenserCommon and TeleportIngestorCommon, while functions that deal with simulator-specific physical movements remain in the individual rmf_ignition_plugins and rmf_gazebo_plugins folder. A byproduct of this is that the code is slightly more verbose in parts - mostly when binding functions to pass as callbacks - though duplication is reduced.

I also considered moving some functions that are similar between both the Ignition plugins, or between both the Gazebo plugins to some common library, e.g. ignition_utils.hpp, but this may make the code even more complex because it would involve passing a lot of class member variables to the functions, and would also make defining callback functions to pass to on_update() harder. Would love to hear your opinion on this - I can change it if needed.

mrushyendra commented 4 years ago

Sure, done!