Skip to content

Deepanshu/maximum arming angle#198

Open
scientistPargain wants to merge 4 commits intortlopez:masterfrom
scientistPargain:deepanshu/maximum_arming_angle
Open

Deepanshu/maximum arming angle#198
scientistPargain wants to merge 4 commits intortlopez:masterfrom
scientistPargain:deepanshu/maximum_arming_angle

Conversation

@scientistPargain
Copy link

Changes in this pr

This pull request introduces new configuration options related to arming logic and accelerometer trim, along with improvements to attitude calculation and arming safety checks. The changes enhance configurability and safety by allowing fine-tuning of arming behavior and ensuring the aircraft cannot be armed when tilted beyond a user-defined angle. Additionally, accelerometer trim values are now applied to attitude calculations, improving accuracy.

  • Made minimum arming angle work.
    (previously it was default to 180 deg which can result in accidents because it can arm in any angle)

  • Fixed Accelerometer trim to work from UI.
    (This is very useful as fix drone drifts)

@scientistPargain scientistPargain marked this pull request as ready for review January 25, 2026 06:38
//
// Convert trim from 0.1 degrees to radians and apply.
euler.y += _model.config.accel.trim[0] * 0.1f * (PI / 180.0f); // pitch trim
euler.x += _model.config.accel.trim[1] * 0.1f * (PI / 180.0f); // roll trim
Copy link
Owner

Choose a reason for hiding this comment

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

This solution is not correct. It'll cause that euler angles are out of allowed range for AHRS algorithms. It may result in very bad behaviour in some specific attitude. My recommendation is to build rotation matrix and apply rotation to accel readings somehere in AccelSensor class.

Copy link
Author

Choose a reason for hiding this comment

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

I see, sure I'll try to build rotation matrix then. thanks

};

struct FusionConfig
namespace Espfc
Copy link
Owner

Choose a reason for hiding this comment

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

such reformatting kills whole change history in git. Please leave only changed lines.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, my formatter just formatted it automatically, didn't noticed. Will revert these formatting changes

Param(PSTR("failsafe_delay"), &c.failsafe.delay),
Param(PSTR("failsafe_kill_switch"), &c.failsafe.killSwitch),

Param(PSTR("arming_auto_disarm_delay"), &c.arming.autoDisarmDelay),
Copy link
Owner

Choose a reason for hiding this comment

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

do not add fields that are not handled in any way.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sorry, my bad. Reverted

{
const float maxTiltRad = Utils::toRad(_model.config.arming.smallAngle);
const float currentTilt = std::max(std::abs(_model.state.attitude.euler[AXIS_PITCH]),
std::abs(_model.state.attitude.euler[AXIS_ROLL]));
Copy link
Owner

Choose a reason for hiding this comment

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

technically correct formula should be acos(cos(roll)*cos(pitch)) here

Copy link
Author

Choose a reason for hiding this comment

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

I have used the approach that you suggested but the tilt calculation using trigonometric functions (acosf, cosf) is computationally more expensive than the previous max(abs(pitch), abs(roll)) approach. This code runs in the arming check path which may execute frequently.

Is the increased accuracy justifies the performance cost ?

Copy link
Owner

Choose a reason for hiding this comment

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

You are right, I'm still wondering if your initial solution will be sufficient. Although the actual solution is more computationally complex, this fragment is only executed 50 times per second. The total impact can be seen in the CLI by entering the "stats" command. The result will then be in the "rx_a:" line. Of course, we can still ask the AI ​​to propose an intermediate solution. Give me some more time to think about it.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

This commit introduces a mechanism to apply pitch and roll trim adjustments to the accelerometer readings when the calibration state is idle. The adjustments are based on configuration values and are applied using a rotation matrix to ensure accurate sensor data representation.
if(trimPitch != 0.f || trimRoll != 0.f)
{
RotationMatrixFloat trimRotation;
trimRotation.init(VectorFloat(trimRoll, trimPitch, 0.f));
Copy link
Owner

Choose a reason for hiding this comment

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

On the other hand init() is pretty expensive function, and executed at about 500Hz. it would be good if we can move it somewhere to the begin() section. If we move trimRotation to the ModelState we would be able to recalculate it only if trims changes via msp.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll work on this.

…and structure

- Updated ModelState.h to enhance code formatting and organization, including consistent indentation and spacing.
- Moved enums and structs within the Espfc namespace to improve encapsulation.
- Refactored AccelSensor class methods for clarity, including the addition of updateTrimRotation method.
- Improved initialization and calibration logic in AccelSensor to streamline the setup process.
- Enhanced debug logging for better traceability during sensor initialization and updates.
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.

2 participants