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

Posix iris stereo fix #16993

Closed
wants to merge 4 commits into from
Closed

Posix iris stereo fix #16993

wants to merge 4 commits into from

Conversation

canberkgurel
Copy link

@canberkgurel canberkgurel commented Feb 27, 2021

Describe problem solved by this pull request

I typically only use the iris model for my PX4-SITL testing, lately I migrated to a new Linux machine and installed a later version of the PX4-Autopilot (following this documentation). I wanted to use the iris-stereo-camera model for an application I'm working on. In the bottom of the documentation I used, there's a short code snippet showing how the Gazebo simulation can be modified to integrate sensors publishing directly to ROS topics; to support this feature, Gazebo is launched with the appropriate ROS wrappers. I realized that the launch file (posix_sitl.launch) that's referred in this code snippet doesn't support the iris-stereo-camera.

This wasn't working:

cd /path/to/PX4-Autopilot
DONT_RUN=1 make px4_sitl_default gazebo
source ~/catkin_ws/devel/setup.bash    # (optional)
source Tools/setup_gazebo.bash $(pwd) $(pwd)/build/px4_sitl_default
export ROS_PACKAGE_PATH=$ROS_PACKAGE_PATH:$(pwd)
export ROS_PACKAGE_PATH=$ROS_PACKAGE_PATH:$(pwd)/Tools/sitl_gazebo
roslaunch px4 posix_sitl.launch vehicle:=iris_stereo_camera

Describe your solution

I've originally created a post on discuss.px4/ROS to find out what the problem is. I spent a little more time digging it. I found the script printing the error message that I was receiving. I noticed that the platforms/posix/cmake/sitl_target.cmake excludes the iris-stereo-camera. I fixed the problem with the modifications I made in this PR. The branch I'm trying to merge works with the posix_sitl.launch file when used as it is stated in the documentation.

Describe possible alternatives

posix_sitl.launch is documented as the plain SITL launch script that needs to be used to run the simulation wrapped in ROS. There's no other alternative to use posix_sitl.launch and iris_stereo_camera as far as I know. However, it's possible to use the iris-stereo-camera with px4.launch. My intention here is to fix the problem in the posix_sitl.launch as it appears to be the preferred method.

Test data / coverage

I ran the code snippet I've provided above and confirmed that it was working by monitoring the published topics via rostopic list, viewed the images published via rosrun image_view ..., and ensured that the camera_info topic contained the camera specs modeled in the stereo_camera.sdf file.

Additional context

  • OS: Ubuntu 18.04.5 LTS
  • Gazebo version: Gazebo 9.16.0
  • ROS version: Melodic
  • Firmware version: 1.11.2

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution,

  • Can you check the conflicts and rebase this PR?
    Seems like the base branch is outdated therefore the diff includes a lot more than you intent to merge

  • Also, adding iris_stereo_camera as a make target or adding an airframe config is not the correct way to handle your use case. Rather, you need to handle the launchfile where the airframe config name and the sdf model names can be defined separately.

@canberkgurel
Copy link
Author

@Jaeyoung-Lim

Seems like the base branch is outdated therefore the diff includes a lot more than you intent to merge

I've been using Firmware v1.11.2. So, I made the changes in there, I think all of the additional commits are the ones submitted between v1.11.2 and the current master branch.

Can you check the conflicts and rebase this PR?

I'll rebase as requested.

Also, adding iris_stereo_camera as a make target or adding an airframe config is not the correct way to handle your use case. Rather, you need to handle the launchfile where the airframe config name and the sdf model names can be defined separately.

Having not understood why it isn't the correct way, I know what you're suggesting is an alternative solution that also solves the problem. So, I'll undo the changes I made and fix the launch file. (It might be easier to create a separate PR for this.)

@canberkgurel
Copy link
Author

canberkgurel commented Mar 1, 2021

Argh! Rebasing caused me to solve some merge conflicts that deviated my PR away from its intended purpose; I'm closing this one, I'll open a new one.

MaEtUgR and others added 4 commits March 1, 2021 14:28
It loads the battery parameters but then overwrites them
with hardcoded values and it breaks the ModuleParams
parent/child hierarchy. Both is undesired.
iris_stereo_camera wasn't working with the posix. I fixed that problem.
@canberkgurel canberkgurel reopened this Mar 6, 2021
@LorenzMeier
Copy link
Member

I'm sorry, but would you mind rebasing it again?

@canberkgurel
Copy link
Author

canberkgurel commented Mar 11, 2021

@LorenzMeier I originally modified platforms/posix/cmake/sitl_target.cmake and ROMFS/px4fmu_common/init.d-posix/10017_iris_stereo_camera. These modifications solved the problem I mentioned about in the original post. Unfortunately, my original modifications got entangled with some other commits that weren't part of my original PR. Can someone help me remove the unintended commits out of my PR? I've just learned about rebasing from an online tutorial, I can indeed re-do it if you think that'll solve the problem!

@canberkgurel
Copy link
Author

It looks like the main branch is very dynamic, files are changing very quickly. There've been some merge conflicts since I opened the PR. We need to act quickly if we want to merge this in. So, please give me some guidance on how to remove some of the commits out of my PR, and how to solve the new merge conflicts.

@Jaeyoung-Lim
Copy link
Member

@canberkdurmus Adding the iris_stereo_camera airframe is not the right solution for your problem.

The correct solution for your use case it to use iris for

<arg name="vehicle" default="iris"/>
and use the iris_stereo_camera sdf file in
<arg name="sdf" default="$(find mavlink_sitl_gazebo)/models/$(arg vehicle)/$(arg vehicle).sdf"/>

Please let me know if you have issues using the model this way. I am closing this PR for since it doesn't seem applicable to solve your problem.

@canberkdurmus
Copy link
Contributor

Hi, @Jaeyoung-Lim I am not the right Canberk, he is @canberkgurel. Not a problem of course 😄

@canberkgurel
Copy link
Author

@Jaeyoung-Lim You tagged the wrong person. In the original post, I've clearly stated that roslaunch px4 posix_sitl.launch vehicle:=iris_stereo_camera doesn't work w/o the modification I've committed. I've already tested setting vehicle to iris_stereo_camera, if that was working I wouldn't have created this PR in the first place. When I set vehicle=iris_stereo_camera, the sdf variable is equal to $(find mavlink_sitl_gazebo)/models/iris_stereo_camera/iris_stereo_camera.sdf. You're suggesting me to use the mavros_posix_sitl.launch file instead of posix_sitl.launch. I checked the contents, the only difference is that in the former, mavros is launched in addition to everything else in the posix_sitl.launch.

@Jaeyoung-Lim
Copy link
Member

Jaeyoung-Lim commented Mar 20, 2021

@canberkdurmus Oh, very sorry about that.

In the original post, I've clearly stated that roslaunch px4 posix_sitl.launch vehicle:=iris_stereo_camera doesn't work w/o the modification I've committed.

@canberkgurel
I am quite doubtful about this statement. I am using in other projects sdf models that don't have an airframe config (exactly your usecase) and this works. In your PR, you also added a make target for the iris_stereo_camera which isn't related to starting models with ROS.

What I am saying applies also for posix_sitl.launch

you just need to leave

    <arg name="vehicle" default="iris"/>

as it is and set the sdf path as

    <arg name="sdf" default="$(find mavlink_sitl_gazebo)/models/iris_stereo_camera/iris_stereo_camera.sdf"/>

or if you want to do it as you are launching by command line

roslaunch px4 posix_sitl.launch vehicle:=iris sdf:=<path_to_sdf>

@ADI201998
Copy link

@canberkdurmus Oh, very sorry about that.

In the original post, I've clearly stated that roslaunch px4 posix_sitl.launch vehicle:=iris_stereo_camera doesn't work w/o the modification I've committed.

@canberkgurel I am quite doubtful about this statement. I am using in other projects sdf models that don't have an airframe config (exactly your usecase) and this works. In your PR, you also added a make target for the iris_stereo_camera which isn't related to starting models with ROS.

What I am saying applies also for posix_sitl.launch

you just need to leave

    <arg name="vehicle" default="iris"/>

as it is and set the sdf path as

    <arg name="sdf" default="$(find mavlink_sitl_gazebo)/models/iris_stereo_camera/iris_stereo_camera.sdf"/>

or if you want to do it as you are launching by command line

roslaunch px4 posix_sitl.launch vehicle:=iris sdf:=<path_to_sdf>

So I ran the command roslaunch px4 mavros_posix_sitl.launch vehicle:=iris sdf:=~/PX4-Autopilot/Tools/simulation/gazebo-classic/sitl_gazebo-classic/models/iris_depth_camera/iris_depth_camera.sdf.
I was able to see a white cube in gazebo, which I'm assuming is the stereo camera.

But when I run rostopic list, I dont see any topic related to depth image.

But when i replace iris_depth_camera with iris_fpv_cam, I can see topics for rgb image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants