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/Plane: Judge by return value #20057

Closed
wants to merge 2 commits into from

Conversation

muramura
Copy link
Contributor

The angle_error variable is used only once.
I make the decision in the return value of the method, which eliminates the need for this variable.

Copy link
Member

@hendjoshsr71 hendjoshsr71 left a comment

Choose a reason for hiding this comment

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

I'm tempted to close this as it doesn't make a material difference and the compiler removes the extra variable anyway. (Thus the zero size change.)

It doesn't really change the clarity of the code for the better or worse. It does add one more step during debugging though.

@rmackay9 is the code style master though so his call?

@@ -294,8 +293,7 @@ void Copter::parachute_check()
parachute.check_sink_rate();

// check for angle error over 30 degrees
const float angle_error = attitude_control->get_att_error_angle_deg();
if (angle_error <= PARACHUTE_CHECK_ANGLE_DEVIATION_DEG) {
if (attitude_control->get_att_error_angle_deg() <= CRASH_CHECK_ANGLE_DEVIATION_DEG) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right...

Suggested change
if (attitude_control->get_att_error_angle_deg() <= CRASH_CHECK_ANGLE_DEVIATION_DEG) {
if (attitude_control->get_att_error_angle_deg() <= PARACHUTE_CHECK_ANGLE_DEVIATION_DEG) {

@tridge
Copy link
Contributor

tridge commented Feb 14, 2022

this doesn't look like an improvement and makes debugging harder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants