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

Support odometry velocity in body and local frame #13934

Closed
wants to merge 2 commits into from

Conversation

kamilritz
Copy link
Contributor

@kamilritz kamilritz commented Jan 13, 2020

At the moment we expect the external vision's velocity to be expressed in world/local frame. This PR allows for the vision velocity also being expressed in body frame. For this a field is added to the vehicle_odometry message that stores the frame in which the velocity is expressed;

Related ECL PR: PX4/PX4-ECL#708
Solving issue: #13935

FYI @nicovanduijn @bresch @jkflying

TSC21
TSC21 previously approved these changes Jan 13, 2020
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

LGTM

jkflying
jkflying previously approved these changes Jan 14, 2020
Copy link
Contributor

@jkflying jkflying left a comment

Choose a reason for hiding this comment

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

LGTM. NB: needs ECL merged first.

@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #13934 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13934      +/-   ##
==========================================
- Coverage   52.97%   52.91%   -0.07%     
==========================================
  Files         605      605              
  Lines       51349    51354       +5     
==========================================
- Hits        27202    27172      -30     
- Misses      24147    24182      +35     
Flag Coverage Δ
#mavsdk ?
#python 3.15% <0.00%> (-0.01%) ⬇️
#sitl_mission_FW 29.38% <16.66%> (?)
#sitl_mission_MC_box 29.77% <16.66%> (-0.04%) ⬇️
#sitl_mission_MC_offboard_att 28.30% <16.66%> (+<0.01%) ⬆️
#sitl_mission_MC_offboard_pos 28.50% <16.66%> (-0.22%) ⬇️
#sitl_mission_Rover 26.22% <16.66%> (+<0.01%) ⬆️
#sitl_mission_VTOL_standard 33.94% <16.66%> (-0.03%) ⬇️
#sitl_mission_VTOL_tailsitter 33.94% <16.66%> (-0.03%) ⬇️
#sitl_python_FW 29.46% <16.66%> (-45.22%) ⬇️
#sitl_python_MC_box 29.84% <16.66%> (-0.04%) ⬇️
#sitl_python_MC_offboard_att 28.38% <16.66%> (?)
#sitl_python_MC_offboard_pos 28.57% <16.66%> (-0.22%) ⬇️
#sitl_python_Rover 26.30% <16.66%> (+<0.01%) ⬆️
#sitl_python_VTOL_standard 34.01% <16.66%> (-0.03%) ⬇️
#sitl_python_VTOL_tailsitter 34.01% <16.66%> (-0.03%) ⬇️
#unittest 34.81% <0.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/modules/navigator/takeoff.cpp 3.92% <0.00%> (-76.48%) ⬇️
...flight_tasks/tasks/Failsafe/FlightTaskFailsafe.cpp 0.00% <0.00%> (-69.57%) ⬇️
src/modules/navigator/rtl.cpp 14.46% <0.00%> (-44.22%) ⬇️
...lib/flight_tasks/tasks/Manual/FlightTaskManual.cpp 0.00% <0.00%> (-38.89%) ⬇️
src/modules/navigator/loiter.cpp 8.16% <0.00%> (-38.78%) ⬇️
src/modules/mavlink/mavlink_timesync.cpp 72.50% <0.00%> (-15.00%) ⬇️
...der/Arming/PreFlightCheck/checks/airspeedCheck.cpp 59.25% <0.00%> (-14.82%) ⬇️
..._tasks/tasks/Utility/ManualVelocitySmoothingXY.hpp 78.57% <0.00%> (-14.29%) ⬇️
...t_tasks/tasks/Utility/ManualVelocitySmoothingZ.hpp 4.34% <0.00%> (-13.05%) ⬇️
src/lib/flight_tasks/FlightTasks.hpp 62.50% <0.00%> (-12.50%) ⬇️
... and 38 more

@kamilritz
Copy link
Contributor Author

NOTE: Last testflights after rebase showed strange behavior. But could also be the vehicle itself. Please fly test this again before merging.

@TSC21
Copy link
Member

TSC21 commented Apr 4, 2020

@kamilritz are you able to rebase and take this further?

@kamilritz
Copy link
Contributor Author

@TSC21 Rebasing is not a problem, but I am not able to test it myself.

@TSC21 TSC21 requested a review from jkflying April 5, 2020 12:03
@TSC21
Copy link
Member

TSC21 commented Apr 5, 2020

@kamilritz ok rebase should be enough for now. Can you do that? I will sync with @jkflying regarding how this can be tested.

@kamilritz
Copy link
Contributor Author

@TSC21 Ok, I will start refactoring as soon as we have a solution for the testing.

@jkflying
Copy link
Contributor

jkflying commented Apr 8, 2020

@TSC21 is there some way we could do ground-truth VIO in SITL?

I'm happy with this PR, but we knew it was working before and there is no test coverage, so I don't want to accidentally break something.

@TSC21
Copy link
Member

TSC21 commented Apr 8, 2020

@TSC21 is there some way we could do ground-truth VIO in SITL?

I'm happy with this PR, but we knew it was working before and there is no test coverage, so I don't want to accidentally break something.

@jkflying we seemed to have that before but it looks like it was removed - 6a4a4bd#diff-436758762aea1e43c9654faf66315930L42-L43.

@kamilritz
Copy link
Contributor Author

I think there still is some basic testing: https://github.com/PX4/Firmware/blob/master/test/mavsdk_tests/configs/sitl.json#L18

@jkflying
Copy link
Contributor

jkflying commented Apr 8, 2020

Considering the current issues I'm happy to merge this based on the SITL tests. I don't think it's realistic to get better testing than that for now.

@kamilritz
Copy link
Contributor Author

I just remember that the last test I did on a real platform, did not went well. And there was not enough time left to investigate why. Based on that I am a bit hesitant to get this in without further testing a real platform.

@jkflying
Copy link
Contributor

jkflying commented Apr 8, 2020

We found that this was due to a problem on the vehicle, so that doesn't give any information to whether this PR works or not 🙂

@kamilritz
Copy link
Contributor Author

Nice. I will rebase it then.

@kamilritz
Copy link
Contributor Author

Rebased the PR and reopened it here: #14703

@kamilritz kamilritz closed this Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants