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

Add MSP support for gyro_cal_on_first_arm #13626

Merged
merged 6 commits into from
May 22, 2024

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented May 6, 2024

@haslinghuis haslinghuis added this to the 4.6 milestone May 6, 2024
@haslinghuis haslinghuis self-assigned this May 6, 2024
Copy link

github-actions bot commented May 6, 2024

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13626 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@@ -2698,7 +2698,7 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, int16_t cmdMSP,
#endif
case MSP_SET_ARMING_CONFIG:
armingConfigMutable()->auto_disarm_delay = sbufReadU8(src);
sbufReadU8(src); // reserved
armingConfigMutable()->gyro_cal_on_first_arm = sbufReadU8(src); // reserved
Copy link
Member

Choose a reason for hiding this comment

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

I suppose now the comment is not necessary. Someone knows what was the reserved for?

Copy link
Member

Choose a reason for hiding this comment

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

It seems it was for the disarm_kill_switch 45a6588

It's a good idea to reuse old MSP fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

@McGiverGim the parameter was removed in firmware 3.x as configurator only supports firmware 4.x so we can reuse safely.

@McGiverGim
Copy link
Member

The reason of not reusing, was that third party tools can talk with the FC, not only the Configurator. If we don't plan to let this, no problem to reusing it.

@haslinghuis
Copy link
Member Author

This was removed in 2018 so don't think it would have impact on 3d party tools.

@blckmn
Copy link
Member

blckmn commented May 20, 2024

I'm not sure on this one. Why reuse? Configurator is not the only tool using msp.

@haslinghuis
Copy link
Member Author

Happy to add a new parameter to the message. But as this was removed six years ago don't think it would be used anymore.

@haslinghuis haslinghuis requested a review from a team May 21, 2024 17:31
@blckmn
Copy link
Member

blckmn commented May 21, 2024

Happy to add a new parameter to the message. But as this was removed six years ago don't think it would be used anymore.

The issue is we don't know what's out there (OSDs use MSP). And breaking this interface would have unpredictable results.

@haslinghuis haslinghuis merged commit e602243 into betaflight:master May 22, 2024
23 checks passed
@haslinghuis haslinghuis deleted the gyro_cal_on_first_arm branch May 22, 2024 14:39
@ctzsnooze
Copy link
Member

The PR itself looks fine.
Can I ask one question? Does enabling gyro cal on first arm delay arming until the calibration is complete? I’ve been concerned that if it did not delay arming, then the motors might shake the quad at the time the calibration is being performed.

@nerdCopter
Copy link
Member

@ctzsnooze , this PR only add MSP to already existing routines,

i did not deep dive, but i found this: cleanflight/cleanflight#427 which alludes wait-for-arming.

@haslinghuis
Copy link
Member Author

We use

static bool isCalibrating(void)
{
return (sensors(SENSOR_GYRO) && !gyroIsCalibrationComplete())
#ifdef USE_ACC
|| (sensors(SENSOR_ACC) && !accIsCalibrationComplete())
#endif
#ifdef USE_BARO
|| (sensors(SENSOR_BARO) && !baroIsCalibrated())
#endif
#ifdef USE_MAG
|| (sensors(SENSOR_MAG) && !compassIsCalibrationComplete())
#endif
;
}

if (isCalibrating()) {
setArmingDisabled(ARMING_DISABLED_CALIBRATING);
} else {
unsetArmingDisabled(ARMING_DISABLED_CALIBRATING);
}

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

Successfully merging this pull request may close these issues.

Add gyro_cal_on_first_arm
6 participants