Skip to content

Conversation

@xznhj8129
Copy link
Contributor

@xznhj8129 xznhj8129 commented Dec 14, 2025

User description

Added asin/acos/atan2 functions to IPF

@github-actions
Copy link

Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

  • maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.

  • maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.


This is an automated suggestion to help route contributions to the appropriate branch.

@xznhj8129 xznhj8129 marked this pull request as ready for review December 17, 2025 20:51
@xznhj8129 xznhj8129 changed the base branch from master to maintenance-10.x December 17, 2025 20:51
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

Comment on lines +412 to +415
case LOGIC_CONDITION_ASIN:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(asin_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Correct the LOGIC_CONDITION_ASIN implementation, which currently computes ACOS instead of ASIN, by applying the acos_to_asin_approx macro to the result. [possible issue, importance: 9]

Suggested change
case LOGIC_CONDITION_ASIN:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(asin_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;
case LOGIC_CONDITION_ASIN:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(acos_to_asin_approx(acos_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f))));
break;

Comment on lines +417 to +419
case LOGIC_CONDITION_ATAN2:
return RADIANS_TO_DEGREES(atan2_approx((float)operandA, (float)operandB));
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Normalize the returned degrees to a defined range (e.g., (-180, 180]) and add an explicit fallback for the (0,0) input so consumers don't depend on undefined/implementation-specific angle conventions. [Learned best practice, importance: 6]

Suggested change
case LOGIC_CONDITION_ATAN2:
return RADIANS_TO_DEGREES(atan2_approx((float)operandA, (float)operandB));
break;
case LOGIC_CONDITION_ATAN2: {
if (operandA == 0 && operandB == 0) {
return 0;
}
float deg = RADIANS_TO_DEGREES(atan2_approx((float)operandA, (float)operandB));
while (deg <= -180.0f) deg += 360.0f;
while (deg > 180.0f) deg -= 360.0f;
return (int32_t)deg;
} break;

Comment on lines +407 to +415
case LOGIC_CONDITION_ACOS:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(acos_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;

case LOGIC_CONDITION_ASIN:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(asin_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Use a float denominator (with a safe default) and compute the clamped ratio once to avoid repeated casts and make the boundary behavior explicit and consistent. [Learned best practice, importance: 5]

Suggested change
case LOGIC_CONDITION_ACOS:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(acos_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;
case LOGIC_CONDITION_ASIN:
temporaryValue = (operandB == 0) ? 1000 : operandB;
return RADIANS_TO_DEGREES(asin_approx(constrainf((float)operandA / (float)temporaryValue, -1.0f, 1.0f)));
break;
case LOGIC_CONDITION_ACOS: {
const float denom = (operandB == 0) ? 1000.0f : (float)operandB;
const float ratio = constrainf((float)operandA / denom, -1.0f, 1.0f);
return RADIANS_TO_DEGREES(acos_approx(ratio));
} break;
case LOGIC_CONDITION_ASIN: {
const float denom = (operandB == 0) ? 1000.0f : (float)operandB;
const float ratio = constrainf((float)operandA / denom, -1.0f, 1.0f);
return RADIANS_TO_DEGREES(asin_approx(ratio));
} break;

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.

2 participants