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

FlightTask: Fix ekf2 reset race condition during task switch #14692

Merged
merged 5 commits into from Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/lib/flight_tasks/FlightTasks.cpp
Expand Up @@ -36,6 +36,16 @@ const vehicle_local_position_setpoint_s FlightTasks::getPositionSetpoint()
}
}

const ekf_reset_counters_s FlightTasks::getResetCounters()
{
if (isAnyTaskActive()) {
return _current_task.task->getResetCounters();

} else {
return FlightTask::zero_reset_counters;
}
}

const vehicle_constraints_s FlightTasks::getConstraints()
{
if (isAnyTaskActive()) {
Expand Down Expand Up @@ -64,7 +74,8 @@ FlightTaskError FlightTasks::switchTask(FlightTaskIndex new_task_index)
}

// Save current setpoints for the next FlightTask
vehicle_local_position_setpoint_s last_setpoint = getPositionSetpoint();
const vehicle_local_position_setpoint_s last_setpoint = getPositionSetpoint();
const ekf_reset_counters_s last_reset_counters = getResetCounters();

if (_initTask(new_task_index)) {
// invalid task
Expand All @@ -84,6 +95,8 @@ FlightTaskError FlightTasks::switchTask(FlightTaskIndex new_task_index)
return FlightTaskError::ActivationFailed;
}

_current_task.task->setResetCounters(last_reset_counters);

return FlightTaskError::NoError;
}

Expand Down
6 changes: 6 additions & 0 deletions src/lib/flight_tasks/FlightTasks.hpp
Expand Up @@ -81,6 +81,12 @@ class FlightTasks
*/
const vehicle_local_position_setpoint_s getPositionSetpoint();

/**
* Get the local frame and attitude reset counters of the estimator from the current task
* @return the reset counters
*/
const ekf_reset_counters_s getResetCounters();

/**
* Get task dependent constraints
* @return setpoint constraints that has to be respected by the position controller
Expand Down
Expand Up @@ -42,8 +42,9 @@ using namespace matrix;

bool FlightTaskAutoFollowMe::update()
{
bool ret = FlightTaskAuto::update();
_position_setpoint = _target;
matrix::Vector2f vel_sp = _getTargetVelocityXY();
_velocity_setpoint = matrix::Vector3f(vel_sp(0), vel_sp(1), 0.0f);
return true;
}
return ret;
}
Expand Up @@ -49,6 +49,7 @@ bool FlightTaskAutoMapper::activate(vehicle_local_position_setpoint_s last_setpo

bool FlightTaskAutoMapper::update()
{
bool ret = FlightTaskAuto::update();
// always reset constraints because they might change depending on the type
_setDefaultConstraints();

Expand Down Expand Up @@ -107,7 +108,7 @@ bool FlightTaskAutoMapper::update()
// update previous type
_type_previous = _type;

return true;
return ret;
}

void FlightTaskAutoMapper::_reset()
Expand Down
4 changes: 3 additions & 1 deletion src/lib/flight_tasks/tasks/Descend/FlightTaskDescend.cpp
Expand Up @@ -48,6 +48,8 @@ bool FlightTaskDescend::activate(vehicle_local_position_setpoint_s last_setpoint

bool FlightTaskDescend::update()
{
bool ret = FlightTask::update();

if (PX4_ISFINITE(_velocity(2))) {
// land with landspeed
_velocity_setpoint(2) = _param_mpc_land_speed.get();
Expand All @@ -59,5 +61,5 @@ bool FlightTaskDescend::update()
_acceleration_setpoint(2) = .3f;
}

return true;
return ret;
}
4 changes: 3 additions & 1 deletion src/lib/flight_tasks/tasks/Failsafe/FlightTaskFailsafe.cpp
Expand Up @@ -49,6 +49,8 @@ bool FlightTaskFailsafe::activate(vehicle_local_position_setpoint_s last_setpoin

bool FlightTaskFailsafe::update()
{
bool ret = FlightTask::update();

if (PX4_ISFINITE(_position(0)) && PX4_ISFINITE(_position(1))) {
// stay at current position setpoint
_velocity_setpoint(0) = _velocity_setpoint(1) = 0.f;
Expand All @@ -72,5 +74,5 @@ bool FlightTaskFailsafe::update()
_acceleration_setpoint(2) = NAN;
}

return true;
return ret;
}
12 changes: 4 additions & 8 deletions src/lib/flight_tasks/tasks/FlightTask/FlightTask.cpp
Expand Up @@ -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;
Expand All @@ -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;
Copy link
Member

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 👍

Copy link
Member

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?

Copy link
Member

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?

Copy link
Member Author

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?

Ouch, yes, this is correct and not something we want. I need to find an other solution, thanks...

Copy link
Member Author

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

_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();
Copy link
Member

Choose a reason for hiding this comment

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

In principle you could leave the _checkEkfResetCounters() call in the update Initialize. They would just be checked twice on activation then... works either way.

return true;
}

void FlightTask::_checkEkfResetCounters()
Expand Down
29 changes: 19 additions & 10 deletions src/lib/flight_tasks/tasks/FlightTask/FlightTask.hpp
Expand Up @@ -55,6 +55,14 @@
#include <uORB/topics/home_position.h>
#include <lib/weather_vane/WeatherVane.hpp>

struct ekf_reset_counters_s {
uint8_t xy;
uint8_t vxy;
uint8_t z;
uint8_t vz;
uint8_t quat;
};

class FlightTask : public ModuleParams
{
public:
Expand Down Expand Up @@ -97,7 +105,7 @@ class FlightTask : public ModuleParams
* To be called regularly in the control loop cycle to execute the task
* @return true on success, false on error
*/
virtual bool update() = 0;
virtual bool update();

/**
* Call after update()
Expand All @@ -113,6 +121,9 @@ class FlightTask : public ModuleParams
*/
const vehicle_local_position_setpoint_s getPositionSetpoint();

const ekf_reset_counters_s getResetCounters() const { return _reset_counters; }
void setResetCounters(const ekf_reset_counters_s &counters) { _reset_counters = counters; }

/**
* Get vehicle constraints.
* The constraints can vary with task.
Expand All @@ -139,6 +150,11 @@ class FlightTask : public ModuleParams
*/
static const vehicle_local_position_setpoint_s empty_setpoint;

/**.
* All counters are set to 0.
*/
static const ekf_reset_counters_s zero_reset_counters;

/**
* Empty constraints.
* All constraints are set to NAN.
Expand Down Expand Up @@ -193,8 +209,8 @@ class FlightTask : public ModuleParams
/**
* Monitor the EKF reset counters and
* call the appropriate handling functions in case of a reset event
* TODO: add the delta values to all the handlers
*/
void _initEkfResetCounters();
void _checkEkfResetCounters();
virtual void _ekfResetHandlerPositionXY() {};
virtual void _ekfResetHandlerVelocityXY() {};
Expand Down Expand Up @@ -234,14 +250,7 @@ class FlightTask : public ModuleParams
matrix::Vector3f _velocity_setpoint_feedback;
matrix::Vector3f _acceleration_setpoint_feedback;

/* Counters for estimator local position resets */
struct {
uint8_t xy;
uint8_t vxy;
uint8_t z;
uint8_t vz;
uint8_t quat;
} _reset_counters{};
ekf_reset_counters_s _reset_counters{}; ///< Counters for estimator local position resets

/**
* Vehicle constraints.
Expand Down
Expand Up @@ -363,10 +363,11 @@ bool FlightTaskManualAltitude::_checkTakeoff()

bool FlightTaskManualAltitude::update()
{
bool ret = FlightTaskManual::update();
_updateConstraintsFromEstimator();
_scaleSticks();
_updateSetpoints();
_constraints.want_takeoff = _checkTakeoff();

return true;
return ret;
}
12 changes: 7 additions & 5 deletions src/lib/flight_tasks/tasks/Offboard/FlightTaskOffboard.cpp
Expand Up @@ -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();

Expand Down Expand Up @@ -104,7 +106,7 @@ bool FlightTaskOffboard::update()
}

// don't have to continue
return true;
return ret;

} else {
_position_lock.setAll(NAN);
Expand All @@ -122,7 +124,7 @@ bool FlightTaskOffboard::update()
}

// don't have to continue
return true;
return ret;

} else {
_position_lock.setAll(NAN);
Expand All @@ -142,7 +144,7 @@ bool FlightTaskOffboard::update()
}

// don't have to continue
return true;
return ret;

} else {
_position_lock.setAll(NAN);
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

omg we should to get rid of these early returns, not here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's horrible :|

}

// Possible inputs:
Expand Down Expand Up @@ -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;
}
4 changes: 2 additions & 2 deletions src/lib/flight_tasks/tasks/Orbit/FlightTaskOrbit.cpp
Expand Up @@ -162,7 +162,7 @@ bool FlightTaskOrbit::activate(vehicle_local_position_setpoint_s last_setpoint)
bool FlightTaskOrbit::update()
{
// update altitude
FlightTaskManualAltitudeSmooth::update();
bool ret = FlightTaskManualAltitudeSmooth::update();

// stick input adjusts parameters within a fixed time frame
const float r = _r - _sticks_expo(0) * _deltatime * (_radius_max / 8.f);
Expand All @@ -186,7 +186,7 @@ bool FlightTaskOrbit::update()
// publish information to UI
sendTelemetry();

return true;
return ret;
}

void FlightTaskOrbit::generate_circle_approach_setpoints()
Expand Down
Expand Up @@ -60,6 +60,7 @@ void FlightTaskTransition::checkSetpoints(vehicle_local_position_setpoint_s &set

bool FlightTaskTransition::update()
{
bool ret = FlightTask::update();
_acceleration_setpoint.xy() = matrix::Vector2f(0.f, 0.f);
// demand zero vertical velocity and level attitude
// tailsitters will override attitude and thrust setpoint
Expand All @@ -68,5 +69,5 @@ bool FlightTaskTransition::update()
_velocity_setpoint(2) = 0.0f;

_yaw_setpoint = NAN;
return true;
return ret;
}
14 changes: 9 additions & 5 deletions src/lib/flight_tasks/tasks/Utility/ManualVelocitySmoothingXY.hpp
Expand Up @@ -92,7 +92,11 @@ class ManualVelocitySmoothingXY final
_state.x = pos;
_trajectory[0].setCurrentPosition(pos(0));
_trajectory[1].setCurrentPosition(pos(1));
// TODO: check lock/unlock in case of EKF reset
_position_estimate = pos;

if (_position_lock_active) {
_position_setpoint_locked = pos;
}
}
Vector2f getCurrentPosition() const { return _position_setpoint_locked; }

Expand All @@ -108,15 +112,15 @@ class ManualVelocitySmoothingXY final

bool _position_lock_active{false};

Vector2f _position_setpoint_locked;
Vector2f _position_setpoint_locked{};

Vector2f _velocity_setpoint_feedback;
Vector2f _position_estimate;
Vector2f _velocity_setpoint_feedback{};
Vector2f _position_estimate{};

struct {
Vector2f j;
Vector2f a;
Vector2f v;
Vector2f x;
} _state;
} _state{};
};
13 changes: 9 additions & 4 deletions src/lib/flight_tasks/tasks/Utility/ManualVelocitySmoothingZ.hpp
Expand Up @@ -84,6 +84,11 @@ class ManualVelocitySmoothingZ final
{
_state.x = pos;
_trajectory.setCurrentPosition(pos);
_position_estimate = pos;

if (_position_lock_active) {
_position_setpoint_locked = pos;
}
}
float getCurrentPosition() const { return _position_setpoint_locked; }
void setCurrentPositionEstimate(float pos) { _position_estimate = pos; }
Expand All @@ -99,17 +104,17 @@ class ManualVelocitySmoothingZ final

bool _position_lock_active{false};

float _position_setpoint_locked;
float _position_setpoint_locked{};

float _velocity_setpoint_feedback;
float _position_estimate;
float _velocity_setpoint_feedback{};
float _position_estimate{};

struct {
float j;
float a;
float v;
float x;
} _state;
} _state{};

float _max_accel_up{0.f};
float _max_accel_down{0.f};
Expand Down