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 Introduction Replace Legacy Multicopter Position Controller #9731

Merged
merged 164 commits into from Jul 20, 2018

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Jun 21, 2018

It's a follow up to #9253 and #9368.

I rebased all changes to any FlightTasks and the PositionControl logic and left out all the changes to the mc_pos_control_main.cpp file because of nasty rebase conflicts. Then in the last commit "mc_pos_control: replace legacy functionality" I switched the mc_pos_control file to the new refactored version that does not contain any unwanted, replaced legacy code anymore.
Note: The original changes are mostly by @Stifael as correctly represented in the metadata.

We know what's missing right now is:

closes #9368

@dagar
Copy link
Member

dagar commented Jun 22, 2018

Given #9270 (comment) is it worth pushing the core FlightTasks interface improvements to master in a separate PR? I'm wondering if that would help unblock obstacle avoidance migration.

@Stifael
Copy link
Contributor

Stifael commented Jun 25, 2018

@dagar, why would obstacle avoidance be blocked by Flighttask?

@Stifael
Copy link
Contributor

Stifael commented Jun 25, 2018

@priseborough @mhkabir
I ported the mc_pos_control_main changes from #9613 to flighttask (commits:
8405d90,
a10a9a3,
34c881c).

A couple questions about the desired distance to ground behavior:

  • if MPC_ALT_MODE is 0:
    • Is the range sensor used for landing and takeoff only as explained here? If yes, why is then there an altitude limit based on distance to ground during normal altitude mode here.
    • what are the scenarios when ekf2 sets the dist_to_bottom_valid flag to false? if outside the hagl_min/max?

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jun 25, 2018

I cherry-picked the yaw-speed correction from the orbit pr because it's important for the functionality here and the orbit needs to be rebased onto this anyways.

@MaEtUgR MaEtUgR changed the title [WIP] FlightTask Introduction Replace Legacy Multicopter Position Controller FlightTask Introduction Replace Legacy Multicopter Position Controller Jun 25, 2018

/* check if stick direction and current velocity are within 60angle */
const bool is_aligned = (stick_xy_norm * stick_xy_prev_norm) > 0.5f;
if (PX4_ISFINITE(_states.velocity(2))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this stuff outside. this requires to check for landdetector and if in air, then slowly descend, if landed, then just go idle

@mhkabir
Copy link
Member

mhkabir commented Jun 25, 2018

@Stifael

Is the range sensor used for landing and takeoff only as explained here?

The range sensor will be fused into the Local Z estimate for low altitude flight if Range Aid is enabled, however this doesn't explicitly affect the controller.

If yes, why is then there an altitude limit based on distance to ground during normal altitude mode here.

This is to stay within limits of additional sensors (e.g optical flow or visual odometry), but whether it should be active for altitude-control only is up for discussion.

what are the scenarios when ekf2 sets the dist_to_bottom_valid flag to false? if outside the hagl_min/max?

Generally yes. If the rangefinder sensor is out of range, then these flags will go to false.

@priseborough
Copy link
Contributor

priseborough commented Jun 26, 2018

I have some planned changes which add functionality to improve height control when using optical flow over uneven surfaces. I will create them on top of this PR as another branch so this PR will be incorporated into my testing.

// A change in velocity is demanded. Set velocity to the derivative of position
// because it has less bias but blend it in across the landing speed range
float weighting = fminf(fabsf(vel_sp_z) / _land_speed.get(), 1.0f);
_states.velocity(2) = _local_pos.z_deriv * weighting + _local_pos.vz * (1.0f - weighting);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Stifael @MaEtUgR Aren't you overwriting this again a few lines below?

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

}
}

void
MulticopterPositionControl::reset_alt_sp()
MulticopterPositionControl::check_vehicle_states(const float &vel_sp_z)
Copy link
Contributor

Choose a reason for hiding this comment

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

@MaEtUgR @Stifael Maybe call this function set_vehicle_states()?

return ((PX4_ISFINITE(_pos_sp_triplet.current.cruising_speed) && !(_pos_sp_triplet.current.cruising_speed < 0.0f)) ?
_pos_sp_triplet.current.cruising_speed : _vel_cruise_xy.get());
if (PX4_ISFINITE(_local_pos.yaw)) {
_states.yaw = _local_pos.yaw;
Copy link
Contributor

Choose a reason for hiding this comment

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

@MaEtUgR @Stifael We need to check is not isfinite implies that it is NaN. Isn't ther also stuff like INFINITY?

Copy link
Contributor

@Stifael Stifael Jun 27, 2018

Choose a reason for hiding this comment

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

INFINITY and NAN should both return 0 if checked with PX4_ISFINITE.

/* we do manual deceleration */
intention = deceleration;
// limit altitude only if local position is valid
if (PX4_ISFINITE(_states.position(2))) {limit_altitude(setpoint);}
Copy link
Contributor

Choose a reason for hiding this comment

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

@MaEtUgR @Stifael I was confused a while so let's put it on a new line

if (_jerk_hor_max.get() > _jerk_hor_min.get()) {
_manual_jerk_limit_xy = (_jerk_hor_max.get() - _jerk_hor_min.get()) / _velocity_hor_manual.get() *
sqrtf(_vel(0) * _vel(0) + _vel(1) * _vel(1)) + _jerk_hor_min.get();
mavlink_log_info(&_mavlink_log_pub, "[mpc] stopped");
Copy link
Contributor

Choose a reason for hiding this comment

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

@MaEtUgR @Stifael I think we can get rid of this.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jun 27, 2018

I rebased again in the hope CI will be happier.
EDIT: No success, MacOS CI seems broke, cannot even checkout the repo.

@Stifael
Copy link
Contributor

Stifael commented Jun 28, 2018

Here is again the summary of the PR:
#9368 (comment)

Here is again the summary of the different files:
#9368 (comment)

What needs to be tested:
#9368 (comment)

@dagar
Copy link
Member

dagar commented Jun 28, 2018

@dagar
Copy link
Member

dagar commented Jun 28, 2018

@Stifael @MaEtUgR @thomasgubler @RomanBapst @bkueng can we document all the current/expected multicopter position controller capabilities here? https://docs.google.com/document/d/1q1wkqufilFYRzskx9OJcmOHpEQOAgs8fEKEHX2V4ooo/edit?usp=sharing

Then let's use it to structure exhaustive manual (flight cards) and automated (ROS or Dronecode SDK) testing to look for regressions and edge cases.

@Stifael
Copy link
Contributor

Stifael commented Jun 28, 2018

@dagar sounds good.
But once this document is finished, one really needs to compare it to the legacy code. This PR does not fix what is already broken except of some smaller stuff that were obvious to fix. For instance, in the legacy code some parts of follow-me is not executed properly and i have not changed that.

@dagar
Copy link
Member

dagar commented Jun 28, 2018

That's fine, we really just need to have a thorough understanding of the state before and after. Functionality doesn't necessarily need to be replicated 100%. For example if follow me doesn't actually work in master we might be better off leaving it temporarily disabled in FlightTasks until someone wants to get it working properly.

Stifael and others added 14 commits July 17, 2018 11:42
It scales the yawspeed setpoint arbitrarily by default with 0.5.
This makes no sense because when you give a setpoint of 1rad/s then
you expect the setpoint to get executed. If you want manual yawspeed
response to be less agressive on the stick use the scaling parameter
for the stick MPC_MAN_Y_MAX.
…ng()

Without this failsafe will be overwritten
invert direction to point upwards and increase to 70% of throttle range between min and hover
@Stifael
Copy link
Contributor

Stifael commented Jul 17, 2018

@PX4/testflights do you mind to test this branch once more, just to get some more flight hours, which should bring us closer to a version that we can finally merge.

@santiago3dr
Copy link

santiago3dr commented Jul 17, 2018

@LorenzMeier
Copy link
Member

@dagar AV-X target failed: Makefile:154: recipe for target 'av-x-v1_default' failed

@dannyfpv
Copy link

Flew on a Pixhack (V5) with new commits
Full auto flight.
https://review.px4.io/plot_app?log=df816e7c-0bdc-41a1-8561-77e229eb25df

stabilized, altitud and position mode.
https://review.px4.io/plot_app?log=c79567c9-2985-4c99-8379-522d5193ed50

Flew on a Pixmini (V3) with new commits
Full auto flight.
https://review.px4.io/plot_app?log=0cc6fbab-58d7-494c-98ff-d6d3d30517d4
stabilized, altitud and position mode.
https://review.px4.io/plot_app?log=c1918a39-18b9-4a64-a397-6ddcf9b0dc93

Flew on a Pixhawk pro (V4) with new commits
auto flight, position mode.
https://review.px4.io/plot_app?log=22eed7b3-6d19-47dd-8e3d-2fbde1545dd5
stabilized, rc fail safe
https://review.px4.io/plot_app?log=db2b1af2-9c19-48f6-b559-ec21bcb8afd2

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.

None yet