-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use GeographicLib tools to guarantee ROS msg def and enhance features #693
Conversation
Update: So seems that |
…eginning each compile time
bc2c50c
to
0a768e1
Compare
Update: Ok I'm more happy with the result now. Since I don't have a GPS module available, I'm going to try to publish fake gps coordinates and see what do I get. |
gps_cov(2, 0), gps_cov(2, 1), gps_cov(2, 2), 0.0, 0.0, 0.0, | ||
0.0, 0.0, 0.0, rot_cov, 0.0, 0.0, | ||
0.0, 0.0, 0.0, 0.0, rot_cov, 0.0, | ||
0.0, 0.0, 0.0, 0.0, 0.0, rot_cov; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change
mavros/CMakeLists.txt
Outdated
./tools/geographiclib-get-geoids -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data egm96-5 && | ||
./tools/geographiclib-get-gravity -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data egm96 && | ||
./tools/geographiclib-get-magnetic -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data emm2015 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be better solution. And Why use external project???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's user independent. so if there's no GeographicLib detected, it installs it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is horrible. I wouldn't want to install ~500MB of stuff just for this to work, and anyway most embedded targets on a drone may will not have internet connections.
This change will greatly kill the portability and lightweight nature of mavros.
fix->altitude = msg.alt / 1E3; // m | ||
fix->altitude = msg.alt / 1E3 + // conversion from height abov geoid (AMSL) | ||
GeographicLib::Geoid::GEOIDTOELLIPSOID // to height above ellipsoid (WGS-84) | ||
* egm96(fix->latitude, fix->longitude); // in meters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why egm96 is local?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variable you say? does it make a difference?
@vooon are we good to merge this? |
mavros/CMakeLists.txt
Outdated
./tools/geographiclib-get-geoids -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data egm96-5 && | ||
./tools/geographiclib-get-gravity -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data egm96 && | ||
./tools/geographiclib-get-magnetic -p ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/share/data emm2015 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is horrible. I wouldn't want to install ~500MB of stuff just for this to work, and anyway most embedded targets on a drone may will not have internet connections.
This change will greatly kill the portability and lightweight nature of mavros.
It's not 500 not even close. The geoid dataset is around 20mb. And I'm replacing this with the deb package. Just have to find a way around to install the dataset. |
Just to clarify datasets sizes so there's no misunderstanding: |
ece5286
to
b6d5ac8
Compare
@TSC21 Hello, I test on odroid C1+ with Ubuntu 16.04 and ROS Kinetic connect to ardupilot SITL copter on another computer. Two things I notice at install : updating from source install need a rosdep update ! and libgeographic-dev is needed as dependency. It could be nice to write it somewhere for those (like me ) that rarely use rosdep install I also test with my PC and no other problem noticed |
Those are great news! Thanks for sharing @khancyr!
That will happen if you have already the package installed from
That's perfect! Thank you! |
@khancyr BTW are you able to share the resulting output, with and without GeographicLib, from |
I will look at it tomorrow, from memory on SITL default location the use of geo lib give 604m on altitude and 584m without. It should be better to check with a real gps as 584m is the default altitude of SITL, I think . |
Thank you! I think it should be very similar. But in any case always better to test in a real GPS. BTW, instructions updated on f6e6d7c. |
@khancyr can you compare load averages, top? |
@vooon, PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
4977 tsc21 34 14 1028604 74048 28384 S 11.3 0.2 4:27.39 mavros_node it varies between 10.9 and 13%. Load average: 0.29 without: PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
7870 tsc21 20 0 991796 33352 28492 S 10.7 0.1 0:07.06 mavros_node varies between 9.7 and 12.3%. Load average: 0.16 So I don't see a significant change, at least for mavros running on a laptop. Let's check @khancyr results if he's able too. |
@khancyr you were able to test the load balance on your Odroid-C1? Please when you can let us know of your results so we can safely merge this. |
laptop (i5-4200H 8gb ram) using SITL on default sim_vehicle.py -v ArduCopter --console --map -w . Mode guided, takeoff 10, and just wait at 10m
rostopic hz /mavros/global_position/local
With :
rostopic hz /mavros/global_position/local
|
@khancyr are yoi able to run |
@TSC21 yep . I am doing it now ! I separate the test to make it easier to write and read ^^ |
odroid C1+ With : |
Are those the results of running |
Results we got a little more CPU usage and lot more memory usage |
I use htop as I don't know how to show only one process on top ... |
The memory is normal because it requires loading the datasets to memory. But since they are small, the increase is not so relevant (5% on the odroid). |
But when you run |
odroid |
Is that on the Odroid C1 or the laptop? |
on odroid |
Seems a small difference but if it is like that it is awesome. Thanks for the results! |
…mavlink#693) * first commit * Check for GeographicLib first without having to install it from the beginning each compile time * add necessary cmake files * remove gps_conversions.h and use GeographicLib to obtain the UTM coordinates * move conversion functions to utils.h * geographic conversions: update CMakeLists and package.xml * geographic conversions: force download of the datasets * geographic conversions: remove unneeded cmake module * dependencies: use SHARED libs of geographiclib * dependencies: correct FindGeographicLib.cmake so it can work for common Debian platforms * CMakeList: do not be so restrict about GeographicLib dependency * global position: odometry-use ECEF instead of UTM; update other fields * global position: make travis happy * global position: fix ident * global_position: apply correct frames and frame transforms given each coordinate frame * global_position: convert rcvd global origin to ECEF * global_position: be more explicit about the ecef-enu transform * global position: use home position as origin of map frame * global position: minor refactoring * global position: shield code with exception catch * fix identation * move dataset install to script; update README with new functionalities * update README with warning * global_position: fix identation * update HomePosition to be consistent with the conversions in global_position to ensure the correct transformation of height * home|global_position: fix compile errors, logic and dependencies * home position: add height conversion * travis: update to get datasets * install geo dataset: update to verify alternative dataset folders * travis: remove dataset install to allow clean build * hp and gp: initialize geoid dataset once and make it thread safe * README: update description relative to GeographicLib; fix typos * global position: improve doxygen references * README: update with some tips on rosdep install
The purpose is to use the GeographicLib to compute:
global_position.cpp
plugin for now)HomePosition.msg
and add height conversionI'm not very happy to include a build for it as an
ExternalProject
onCMakeList.txt
, so probably I will add a dependency only togeographiclib-tools
deb pkg and then add a command onCMakeList.txt
to install the datasets.The lib also allows to compute magnetic fields and gravity field for each coordinate, so we can probably add some usage for that in sensors.