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

Improved initialisation order. Stopped calling pidInit before gyro detected #4218

Merged
merged 9 commits into from
Sep 25, 2017

Conversation

martinbudden
Copy link
Contributor

No description provided.

@martinbudden martinbudden added the BUG Bugs are excluded from automatically being marked as stale label Sep 23, 2017
@martinbudden martinbudden added this to the Betaflight v3.2 milestone Sep 23, 2017
@@ -342,6 +342,13 @@ void validateAndFixConfig(void)
#endif

#ifndef USE_OSD_SLAVE
if (systemConfig()->activeRateProfile >= CONTROL_RATE_PROFILE_COUNT) {// sanity check
Copy link
Member

Choose a reason for hiding this comment

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

A space would be nice here. ;-)

if (systemConfig()->activeRateProfile >= CONTROL_RATE_PROFILE_COUNT) {// sanity check
systemConfigMutable()->activeRateProfile = 0;
}
if (systemConfig()->pidProfileIndex >= MAX_PROFILE_COUNT) {// sanity check
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@@ -267,7 +267,6 @@ static void setPidProfile(uint8_t pidProfileIndex)
if (pidProfileIndex < MAX_PROFILE_COUNT) {
systemConfigMutable()->pidProfileIndex = pidProfileIndex;
currentPidProfile = pidProfilesMutable(pidProfileIndex);
pidInit(currentPidProfile); // re-initialise pid controller to re-initialise filters and config
Copy link
Contributor

Choose a reason for hiding this comment

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

removing pidInit() from here will mean that when readEEPROM is called, after changing pid profile, e.g. using sticks, the pid and all the filters will not be correctly re-configured.

Check the call graph of setPidProfile:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

it feels like activateConfig() should call pidInit(), but that still won't fix the underlying problem on first boot.

@@ -594,19 +607,13 @@ void readEEPROM(void)
if (!loadEEPROM()) {
failureMode(FAILURE_INVALID_EEPROM_CONTENTS);
}

validateAndFixConfig();
validateAndFixGyroConfig();
Copy link
Member

Choose a reason for hiding this comment

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

I think this has to be guarded by #ifndef USE_OSD_SLAVE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

@martinbudden martinbudden merged commit db8698d into betaflight:master Sep 25, 2017
@martinbudden martinbudden deleted the bf_initialisation branch September 25, 2017 04:15
@hydra hydra mentioned this pull request Oct 2, 2017
@VoicOfReason
Copy link

Has this fix been added to RC6? I just upgraded to RC6 and bug is still present.

@martinbudden
Copy link
Contributor Author

@VoicOfReason , this code has been added to RC6. Please could you describe the bug that you are still experiencing?

@VoicOfReason
Copy link

VoicOfReason commented Oct 3, 2017 via email

@VoicOfReason
Copy link

VoicOfReason commented Oct 3, 2017 via email

@richard-scott
Copy link

This looks to be causing a double initialisation on FC boot: #4257

@mikeller mikeller mentioned this pull request Oct 11, 2017
@modellbobby
Copy link

Is this drifting issue now fixed and merged into a release? Would like to test it again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Bugs are excluded from automatically being marked as stale RN: BUGFIX Testing Required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants