-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
FlightTask: Fix ekf2 reset race condition during task switch #14692
Changes from all commits
b8da950
ca96289
18bf120
f85571e
2e5cdb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,14 +6,14 @@ constexpr uint64_t FlightTask::_timeout; | |
// First index of empty_setpoint corresponds to time-stamp and requires a finite number. | ||
const vehicle_local_position_setpoint_s FlightTask::empty_setpoint = {0, NAN, NAN, NAN, NAN, NAN, NAN, NAN, NAN, {NAN, NAN, NAN}, {NAN, NAN, NAN}, {NAN, NAN, NAN}, {}}; | ||
|
||
const ekf_reset_counters_s FlightTask::zero_reset_counters = {}; | ||
const vehicle_constraints_s FlightTask::empty_constraints = {0, NAN, NAN, NAN, NAN, NAN, NAN, NAN, false, {}}; | ||
const landing_gear_s FlightTask::empty_landing_gear_default_keep = {0, landing_gear_s::GEAR_KEEP, {}}; | ||
|
||
bool FlightTask::activate(vehicle_local_position_setpoint_s last_setpoint) | ||
{ | ||
_resetSetpoints(); | ||
_setDefaultConstraints(); | ||
_initEkfResetCounters(); | ||
_time_stamp_activate = hrt_absolute_time(); | ||
_gear = empty_landing_gear_default_keep; | ||
return true; | ||
|
@@ -37,17 +37,13 @@ bool FlightTask::updateInitialize() | |
|
||
_evaluateVehicleLocalPosition(); | ||
_evaluateDistanceToGround(); | ||
_checkEkfResetCounters(); | ||
return true; | ||
} | ||
|
||
void FlightTask::_initEkfResetCounters() | ||
bool FlightTask::update() | ||
{ | ||
_reset_counters.xy = _sub_vehicle_local_position.get().xy_reset_counter; | ||
_reset_counters.vxy = _sub_vehicle_local_position.get().vxy_reset_counter; | ||
_reset_counters.z = _sub_vehicle_local_position.get().z_reset_counter; | ||
_reset_counters.vz = _sub_vehicle_local_position.get().vz_reset_counter; | ||
_reset_counters.quat = _sub_attitude.get().quat_reset_counter; | ||
_checkEkfResetCounters(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle you could leave the |
||
return true; | ||
} | ||
|
||
void FlightTask::_checkEkfResetCounters() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,8 @@ bool FlightTaskOffboard::activate(vehicle_local_position_setpoint_s last_setpoin | |
|
||
bool FlightTaskOffboard::update() | ||
{ | ||
bool ret = FlightTask::update(); | ||
|
||
// reset setpoint for every loop | ||
_resetSetpoints(); | ||
|
||
|
@@ -104,7 +106,7 @@ bool FlightTaskOffboard::update() | |
} | ||
|
||
// don't have to continue | ||
return true; | ||
return ret; | ||
|
||
} else { | ||
_position_lock.setAll(NAN); | ||
|
@@ -122,7 +124,7 @@ bool FlightTaskOffboard::update() | |
} | ||
|
||
// don't have to continue | ||
return true; | ||
return ret; | ||
|
||
} else { | ||
_position_lock.setAll(NAN); | ||
|
@@ -142,7 +144,7 @@ bool FlightTaskOffboard::update() | |
} | ||
|
||
// don't have to continue | ||
return true; | ||
return ret; | ||
|
||
} else { | ||
_position_lock.setAll(NAN); | ||
|
@@ -153,7 +155,7 @@ bool FlightTaskOffboard::update() | |
_position_setpoint.setNaN(); // Don't require any position/velocity setpoints | ||
_velocity_setpoint.setNaN(); | ||
_acceleration_setpoint = Vector3f(0.f, 0.f, 100.f); // High downwards acceleration to make sure there's no thrust | ||
return true; | ||
return ret; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. omg we should to get rid of these early returns, not here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's horrible :| |
||
} | ||
|
||
// Possible inputs: | ||
|
@@ -238,5 +240,5 @@ bool FlightTaskOffboard::update() | |
// use default conditions of upwards position or velocity to take off | ||
_constraints.want_takeoff = _checkTakeoff(); | ||
|
||
return true; | ||
return ret; | ||
} |
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.
Ah clever so you went around the problem that the counters got initialized on task start with the new frame by not initializing them before the update and the first update now always initializes them given the reset counter is never exactly zero 👍
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.
Can we be sure the reset counter is never zero when there's a valid estimate?
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.
Don't you then get an e.g.
_ekfResetHandlerPositionZ()
and_smoothing.setCurrentPosition(_position(2))
after every task switch which would override taking over the setpoint from the last task?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.
Ouch, yes, this is correct and not something we want. I need to find an other solution, thanks...
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.
Now, the previous value of the counters is transferred to from the previous to the next FlightTask using a get/set function