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

simulator: add support for multi-distance_sensor instances #14143

Merged
merged 5 commits into from Feb 14, 2020

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Feb 12, 2020

Describe problem solved by this pull request
Extends simulator to support publishing data from multiple distance sensors, which are differentiated by the sensor ID.

Describe your solution
Add support for multi-sensor subscription in Gazebo, which sets a different sensor ID per stream. The sensor ID is what then allows to create a different uORB::PublicationMulti<distance_sensor_s> per sensor.

Describe possible alternatives
Extend to use a PX4RangeFinder per sensor stream coming from the simulation side.

Test data / coverage
To be added as soon as I add an example model with multiple sensors.

Note: Right now, the update to the sitl_gazebo submodule also brings the multiple-VTOL simulation capability.

@TSC21 TSC21 added Admin: Enhancement (improvement) 💡 Sim: gazebo classic Gazebo classic simulator Sensors All the sensors! Tools Sub-tools used within PX4 ecosystem (scripts, etc) labels Feb 12, 2020
@TSC21 TSC21 self-assigned this Feb 12, 2020
@TSC21 TSC21 force-pushed the pr-simulator-multi-distance-sensors branch 3 times, most recently from 1223b53 to 9c2d877 Compare February 13, 2020 12:10
src/modules/simulator/simulator.h Outdated Show resolved Hide resolved
src/modules/simulator/simulator.h Outdated Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved
@TSC21
Copy link
Member Author

TSC21 commented Feb 13, 2020

Tested with the extended iris_obs_avoid model: PX4/PX4-SITL_gazebo-classic#411 (comment)

@TSC21 TSC21 force-pushed the pr-simulator-multi-distance-sensors branch from 472ac32 to ca251ac Compare February 13, 2020 16:30
@TSC21
Copy link
Member Author

TSC21 commented Feb 13, 2020

@LorenzMeier @dagar @bkueng I RFC on how we should handle the different sensor streams on the firmware. The discussion I suppose will be between two options:

  1. Create a uORB topic property that allows to setup a max number of instances per topic, and that way replace ORB_MULTI_MAX_INSTANCES (I don't think we have to replace it, but rather use it as a default for all topics and then set specific values for specific topics). The idea is to remove the constrain of 4 instances, in specific for the distance_sensor topic;
  2. Create a new uORB message that contains an array of distance_sensor, where the data is stored in a single message - this would mean that on the same driver, one collects the data from different sensors, stores it in the same message, and sends it in a single instance.

I am proposing this discussion since we do not seem to agree on the approach, and we need to align on this so we are able to bring this feature to PX4 - multi distance sensor data.

Thanks in advance for your time!

@TSC21 TSC21 requested a review from bkueng February 13, 2020 16:54
@dagar
Copy link
Member

dagar commented Feb 13, 2020

I'd like to effectively remove the system wide multi limit (or at least bump it to 9), but at the moment there are too many cases throughout the code base that would blow up. Little things like logger checking the full range of multi instances, ekf2, collision_prevention, and a few other areas having an array of all possible instances, etc. If we can get away from "that" it becomes feasible to increase multi. I also think topics should be explicitly multi or not (default) to prevent potential misuse, but that's yet another discussion.

https://github.com/PX4/Firmware/blob/abd2bb4eb738eef2007c43206a353ca38d4fdf6e/src/modules/ekf2/ekf2_main.cpp#L257

Overall I think it makes more sense architecturally to combine the data early into something like a single obstacle_distance (https://github.com/PX4/Firmware/blob/master/msg/obstacle_distance.msg) if that's effectively what all these different distances form. However, if the goal is to simulate what's actually being done on some vehicles with a bunch of distance sensors thrown together I see a small benefit to mirroring that end-to-end pipeline for now.

@TSC21
Copy link
Member Author

TSC21 commented Feb 14, 2020

@dagar @bkueng I suppose we can leave the discussion to a time later and after we reach a consensus I can create a PR. In the mean time, I think this PR is good to go.

@TSC21 TSC21 requested a review from dagar February 14, 2020 10:12
@julianoes julianoes merged commit 7cafbc8 into master Feb 14, 2020
@julianoes julianoes deleted the pr-simulator-multi-distance-sensors branch February 14, 2020 10:32
@mrpollo mrpollo mentioned this pull request Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin: Enhancement (improvement) 💡 Sensors All the sensors! Sim: gazebo classic Gazebo classic simulator Tools Sub-tools used within PX4 ecosystem (scripts, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants