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

Upgrade to Ignition Fortress #70

Merged
merged 9 commits into from
Mar 8, 2022
Merged

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

Closes #69, migrating to Ignition Fortress.

Implementation description

Together with bumping dependencies on CMakeLists and package.xml, we can use a new utility function to enable components which allows us to remove a lot of boilerplate component creation code resulting in a hundred or so lines of code being removed.
Note that this PR will make the binary ros_ign available in Galactic incompatible with rmf_simulation since the ros_ign package in galactic supports Edifice while the ros_ign in Rolling (and future Humble) will support Fortress.
Hence users who wish to use Ignition Fortress with Galactic should switch to build ros_ign from source

Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@luca-della-vedova luca-della-vedova added the enhancement New feature or request label Feb 22, 2022
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@luca-della-vedova luca-della-vedova added this to In Review in Research & Development via automation Mar 3, 2022
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for handling the migration! I've been running some tests and have been unable to get loop requests to run for more than 1 loop. I tried using both the RMF panel and dashboard to issue the request, Could you help take a look into that, just in case it is happening only on my end?

Could you perhaps start a PR to add ros_ign to rmf/rmf.repos as well? Might be a good idea to have that merged in first before this migration.

rmf_building_sim_ignition_plugins/src/lift.cpp Outdated Show resolved Hide resolved
Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@luca-della-vedova
Copy link
Member Author

Thanks for handling the migration! I've been running some tests and have been unable to get loop requests to run for more than 1 loop. I tried using both the RMF panel and dashboard to issue the request, Could you help take a look into that, just in case it is happening only on my end?

I can reproduce the issue with loop requests with more than one loop not happening, it seems once the first loop is finished the robot doesn't receive a new path request so my guess is that the robot's state estimation by the fleet adapter puts it in some state where it doesn't detect the robot's first loop as completed, hence doesn't issue the followup loop request.
It's tricky since really 90% of the logic between Gazebo and Ignition happens in a common source so there should really not be such an issue.
I see this behavior in main also so I guess it's not necessarily related to this PR? But I agree it's quite a deal breaker, will look into it.

Could you perhaps start a PR to add ros_ign to rmf/rmf.repos as well? Might be a good idea to have that merged in first before this migration.

Done open-rmf/rmf#144

@aaronchongth
Copy link
Member

aaronchongth commented Mar 8, 2022

I see this behavior in main also so I guess it's not necessarily related to this PR? But I agree it's quite a deal breaker, will look into it.

Yup, as long as we know that it is not related to this PR we can just start an issue. I can spend a little bit more time to diagnose it.

Per DM, after the lift plugin segfault is fixed, this should be good to merge.

Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@luca-della-vedova
Copy link
Member Author

Per DM, after the lift plugin segfault is fixed, this should be good to merge.

Should be good now 254d720

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the issue! LGTM

@aaronchongth aaronchongth merged commit 35bf411 into main Mar 8, 2022
Research & Development automation moved this from In Review to Done Mar 8, 2022
@aaronchongth aaronchongth deleted the feature/ignition_fortress branch March 8, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Upgrade to Ignition Fortress
2 participants