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

Copter: Remove Payload release on thrust loss detection #27339

Conversation

lthall
Copy link
Contributor

@lthall lthall commented Jun 20, 2024

This PR removes a potentially very dangerous "feature" added in this PR #19890.

The previous PR will release the payload any time the aircraft uses more than 90% thrust for a second while descending under control and less than 15 degrees lean angle.

The thrust loss detection is designed to be a sub check of further dependent checks like the motor out test. It is a check that is expected to happen in normal flight.

This feature above looks like a sensible check to enable but is expected to result in random releases of the payload during landings on low battery for example. The aircraft is heavily loaded because it has a payload, the aircraft is descending and it is normal for the industry to specify their aircraft maximum take-off at 80% thrust on a full battery.

Releasing payloads in an unplanned and automated way, over landing zones or on approach is literally a risk of serious or fatal injury to ground crew.


I have tried to improve the comments on thrust_loss_check() to make it clearer what this check is doing.
We may also be able to improve the output message to the user to be clearer or more appropriate. It is currently "Potential Thrust Loss (%d)" and a motor. However, it does not require a motor lost so it will say motor 1 when no motor is lost I think.

The name is also misleading. Perhaps the name "Thrust Limited Detection" would be better.

We could also reduce the likelihood of this going off by changing the check for decent to make it "less than commanded decent" in alt_hold based modes.

@lthall lthall requested review from IamPete1 and rmackay9 June 20, 2024 01:20
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

Agree. "Randomly" dropping payload is a serious concern.

It would need to check if this wasn't included in a recent releases

@lthall
Copy link
Contributor Author

lthall commented Jun 20, 2024

I have talked to Pete about this and the application was for a drone that dragged a line. If the line got snagged the drone would crash.

This RELEASE_GRIPPER_ON_THRUST_LOSS would mean that anytime the aircraft hit maximum thrust and started reducing in altitude it would release.

The interesting thing is that it would only work if the drone was leant over by less than 15 degrees. So that would mean the aircraft would need to be pulled down almost vertically. I wonder how this could prevent a crash.

@@ -96,7 +96,12 @@ void Copter::crash_check()
}
}

// check for loss of thrust and trigger thrust boost in motors library
// thrust_loss_check - returns true when the aircraft detects for 1 second that it is:
// above 90% thrust,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// above 90% thrust,
// above 90% demanded thrust or high thrust saturation,

Comment on lines -172 to -177

#if AP_GRIPPER_ENABLED
if (copter.option_is_enabled(FlightOption::RELEASE_GRIPPER_ON_THRUST_LOSS)) {
gripper.release();
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

We can't just remove this. The would need to be a arming check if the option is set and the feature is gone.

@rmackay9
Copy link
Contributor

I think we could potentially close this PR and instead focus on this updated version #27396

@rmackay9
Copy link
Contributor

I'm going to go ahead and close this because @lthall has approved the other PR

@rmackay9 rmackay9 closed this Jun 27, 2024
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.

5 participants