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

Consolidate post flight and non-essential save calls in to a single save action #8439

Merged
merged 7 commits into from
Dec 29, 2022

Conversation

MrD-RC
Copy link
Collaborator

@MrD-RC MrD-RC commented Oct 1, 2022

Fixes #3838

This PR reduces the number of non-essential saves processed by the flight controller. Currently, all calls to saveConfigAndNotify() are processed immediately. This means that multiple saves can be processed in the same loop. This PR changes that function to set a flag, to process the save in the next loop. Meaning multiple save calls are now consolidated in to a single save action. For example, continuous servo trim and stats would both call save. Now, there would be a single save for both features. Saving stats and continuous servo trim have been moved over to saveConfigAndNotify(). Another function saveConfig(); has been added for saves where notification is not needed. Going forward, anything that saves on disarm should use saveConfigAndNotify(). Other saves, currently using writeEEPROM(), that do not reboot after saving; can instead use saveConfig() to schedule and consolidate actual saves.

Also, messages has been added to the OSD. A ** SAVING SETTINGS ** messages appears when the save is requested. Then a ** SETTINGS SAVED ** message takes over for 5 seconds; once the save command has completed. These are shown with both standard SYSTEM MESSAGES and on the stats screens.

image

image

This PR reduces the number of non-essential saves processed by the flight controller. Currently, all calls to `saveConfigAndNotify()` are processed immediately. This means that multiple saves can be processed in the same loop. This PR changes that function to set a flag, to process the save in the next loop. Meaning multiple save calls are now consolidated in to a single call. For example, continuous servo trim and stats would both call save. Now, there would be a single save for both features. Saving stats and continuous servo trim have been moved over to `saveConfigAndNotify()`.

Also, messages has been added to the OSD. A `** SAVING SETTINGS ** messages appears when the save is requested. Then a `** SETTINGS SAVED **` message takes over for 5 seconds; once the save command has completed. These are shown with both standard `SYSTEM MESSAGES` and on the stats screen.
src/main/fc/fc_tasks.c Outdated Show resolved Hide resolved
src/main/fc/config.c Outdated Show resolved Hide resolved
This can be used instead of writeEEPROM() if the FC is not rebooted after writing. This will also consolidated multiple saves.
- removed `SAVE` task
- moved `processDelayedSave()` call in to `SYSTEM`
@breadoven
Copy link
Collaborator

breadoven commented Oct 2, 2022

Would moving processDelayedSave() to the following place work ?

armTime = 0;

e.g.:

    if (ARMING_FLAG(ARMED) && (!STATE(FIXED_WING_LEGACY) || !isNavLaunchEnabled() || (isNavLaunchEnabled() && fixedWingLaunchStatus() >= FW_LAUNCH_DETECTED))) {
        flightTime += cycleTime;
        armTime += cycleTime;
        updateAccExtremes();
    }
    if (!ARMING_FLAG(ARMED)) {
        armTime = 0;
        if (saveState != SAVESTATE_NONE) {
            processDelayedSave();
        }
    }

This would only call processDelayedSave once on disarm and only after stats and autoTrim had set the eeprom save state and motors had been stopped. processDelayedSave will trigger again if anything changes the saveState whilst disarmed such as a CMS command.

@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Oct 2, 2022

Would moving processDelayedSave() to the following place work ?

armTime = 0;

This would only call processDelayedSave once on disarm and only after stats and autoTrim had set the eeprom save state and motors had been stopped. processDelayedSave will trigger again if anything changes the saveState whilst disarmed such as a CMS command.

That would actually make sense. It would also force the system to be disarmed before the saving command can be executed, It just seems strange putting it in the PID loop.

@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Oct 2, 2022

@breadoven moved the call in to the !ARMED position recommended. Tested and working on the bench. Tested by adjusting fixed wing level trim. New, saved value present after power cycle.

@breadoven
Copy link
Collaborator

That would actually make sense. It would also force the system to be disarmed before the saving command can be executed, It just seems strange putting it in the PID loop.

I agree the PID loop doesn't exactly seem the right place but then again there seem to be various things in there already that are only loosely PID related. Maybe it needs renaming to something else other than PID, Action or Control loop ?.

@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Dec 4, 2022

@stronnag @breadoven are you happy with the changes?

@MrD-RC MrD-RC requested a review from DzikuVx December 6, 2022 21:02
@DzikuVx DzikuVx merged commit 1e90d9f into master Dec 29, 2022
@DzikuVx DzikuVx deleted the MrD_Minimise-save-calls-and-show-notification branch December 29, 2022 09:21
M0j0Risin added a commit to M0j0Risin/inav that referenced this pull request Dec 30, 2022
I also corrected some buggy behavior that was happening surrounding the stick commands to change pages for multi-page stats.
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.

OSD confirmation message when settings are saved with sticks
4 participants