CMS bugfix and cleanup. #112

AndersHoglund · 2024-05-07T11:39:43Z

Bugfix profile handling. Was a mixed use of global config items, local statics and dynamics. Profiles did get out of sync if external changes was made, from RFC or via adjustments. Now only picks up current profiles on main profile menu entry, and sticks to that until exit.

Menu string changes for a bit more clarity:
CYCL GAIN - CY XC GAIN etc, to indicate "Cross Coupling" (XC).
Use TAIL as in RFC, not YAW, in menu heading.

rotorflight

If I understood this correctly, there is a possible issue with the PID profile init.
If the PID profile has changed, pidInitProfile(pidProfile); will load incorrect values from the old profile.

I think you should check first if the system is still using the same profile that CMS is editing, and only then call pidInitProfile.

rotorflight · 2024-05-08T09:54:45Z

And to make it even more bulletproof, do

if (profile-not-changed)
    pidInitProfile(currentPidProfile);

i.e. use currentPidProfile pointer instead.

AndersHoglund · 2024-05-08T12:11:31Z

Well, I have tested this case. i.e. modifying items in profile 1 and selecting profile 2 via adjustments. and CMS saved to profile 1.
Using the externally modifiable "currentPidProfile" and "getCurrentPidprofile" was part of the problem when used together with the CMS locally cached pidProfile and pidProfileIndex. Mixed usage of globals and locals that are out of sync. Now we ONLY work with locally cached profiles, initialized ONCE at entry. At exit all is written back based on the local pidProfile, NOT the global currentPidProfile that might have been changed externally. Your suggestion would bring back the original problem.

Weather or not profiles and other items are to be saved are selected at exit, "exit" or "save&exit". Not exactly sure how this is conceived to work since there are only one "onExit" method. Multiple layers of config storage i think. So, as CMS is designed now on menu exit, current runtime profiles will be changed regardless of "exit" or "exit&save". But after a power cycle, old values will be back unless "exit&save" is used. Not 100% sure on this, but that is how I interpret my tests. More tests needed I think. This could be regarded as a bug too, but a case of it's own. Not directly connected to this PR.

Edit: Did some more investigations, "onExit" method is only called when "Save&Exit" is selected. Bad method name. Changes should be discarded if only "Exit" is selected. See cms/cms.c, cmsMenuExit() function. Will retest this, not sure what I saw....
Edit2: Menu selection BACK (and rudder stick left) are doing "Save&Exit" without asking. Going directly to EXIT menu will do it properly, depending on selection "Exit" (discard) or "Save&Exit".

AndersHoglund · 2024-05-08T15:40:07Z

Poor testing. One case missed where profile index change by menu selection did not update properly. Fix in commit bd479fd.

rotorflight

You must not call pidInitProfile() with a pointer that isn't the current profile.
(it should not have that argument at all... it's a BF relic.) It must be called always with the currently active profile.
If I am reading this correctly, you are caching the current profile pointer in pidProfile and in the end calling pidInitProfile(pidProfile).
This will break many things if the pidProfile != currentPidProfile

It may be needed to rethink how this works.

AndersHoglund · 2024-05-13T09:48:44Z

The intention is to update and re-init the cached profile we are working with in CMS, NOT the current profile which may have changed externally. The old code did overwrite the wrong profile in this way, I tried to fix that by NOT re-init the current profile. But I'll re-review the code and see if I might have misunderstood the old BF thinking.

rotorflight · 2024-05-13T09:53:49Z

pidInitProfile() is for activating the new profile when the profile is changed.
Or if the parameters are changed in the currently active profile.
(The BF interface for changing profiles is poorly designed, and is confusing)

You can save the edited values into the cached profile pointer - that's totally fine.
But you can call pidInitProfile() only if the currentPidProfile == pidProfile

AndersHoglund · 2024-05-13T10:27:58Z

Well, that is the though; CMS switching active profile to the one being edited and updating the actual pid items. If this is not done, the edited items would not be in effect until saved, a reboot is done. This is connected to the issue of how menus are exited, saved and system rebooted. I am looking into the possible need to add the REBOOT_REQUIRED flag.
Then the only option is to NOT call pidInitProfile at all from CMS and add this REBOOT_REQUIRED flag. Will look into that and re-test. Not sure this will fix the different handling if backing out from a sub-menu or doing a direct exit. Might need a new menu state flag, a dirty bit. And maybe a state machine around that.....phew.
Not right now though, flying weather outside.....

rotorflight · 2024-05-13T10:33:30Z

I am afraid you can't switch the active profile in CMS. If the profile has changed, it is because the adjustment functions has changed it, which means it would cause a conflict.

rotorflight · 2024-05-13T10:34:22Z

changing PID profile parameters in RF does not require reboot (unlike BF).
You can freely change them, and they will be used whenever the user changes to that profile later.

rotorflight · 2024-05-13T10:37:02Z

Also, you don't need to save for the new values to take effect. Saving them just means they are still there after reboot.

AndersHoglund · 2024-05-13T11:13:23Z

OK, then it seems correct for CMS to call pidInitProfile as is now, taking effect directly. Either this or reboot I think.
I insist it was wrong to call pidInitProfile with the currentPidProfile arg, caused over-writing the wrong profile if profile index was changed externally. Using the locally cached pidProfile is more correct. Either this or not at all.

AndersHoglund · 2024-05-14T05:59:17Z

Reviewing the code I see what you mean and you are absolutely correct. Calling pidInitProfile is not the correct way, instead changePidProfile should be used. This function makes sure pid controllers currentPidIndex and the actual pid item values are in sync. Commit 4b5cf52 (after rebase).
.Also, this should probably not be done if no changes are made. Will need figure out how to fix this, not sure how yet. If required (?).

…larity.

…t. 240 dps was shown as 0.240. CMS now shows 24, smae as cli.

…change.

rotorflight · 2024-05-16T10:34:30Z

src/main/cms/cms_menu_imu.c

@@ -190,7 +205,7 @@ static const void *cmsx_PidWriteback(displayPort_t *pDisp, const OSD_Entry *self
        pidProfile->pid[i].O = tempPidO[i];
        pidProfile->pid[i].B = tempPidB[i];
    }
-    pidInitProfile(currentPidProfile);
+    changePidProfile(pidProfileIndex);


You MUST NOT try to change the PID profile from CMS.
It will go out-of-sync with the Tx control.

rotorflight · 2024-05-16T10:34:57Z

src/main/cms/cms_menu_imu.c

    pidProfile->yaw_cw_stop_gain =       cmsx_yawStopCW;
    pidProfile->yaw_ccw_stop_gain =      cmsx_yawStopCCW;
    pidProfile->yaw_cyclic_ff_gain =     cmsx_yawCyclicFF;
    pidProfile->yaw_collective_ff_gain = cmsx_yawCollectiveFF;
    pidProfile->governor.tta_gain =      cmsx_yawTTA;
    pidProfile->yaw_precomp_cutoff =     cmsx_yawPrecompCutoff;

+    changePidProfile(pidProfileIndex);


Ditto. Can't do this.

rotorflight · 2024-05-16T10:35:05Z

src/main/cms/cms_menu_imu.c

@@ -410,14 +402,15 @@ static const void *cmsx_profileCtrlOnExit(displayPort_t *pDisp, const OSD_Entry
      pidProfile->iterm_relax_cutoff[i] = cmsx_pidItermRelaxCutoff[i];
    }

+    changePidProfile(pidProfileIndex);


rotorflight · 2024-05-16T10:35:15Z

src/main/cms/cms_menu_imu.c

    for (uint8_t i = 0; i < 3; i++) {
      pidProfile->gyro_cutoff[i] =  cmsx_pidBandwidth[i];
      pidProfile->dterm_cutoff[i] = cmsx_pidDCutoff[i];
      pidProfile->bterm_cutoff[i] = cmsx_pidBCutoff[i];
    }

+    changePidProfile(pidProfileIndex);


rotorflight · 2024-05-16T10:35:20Z

src/main/cms/cms_menu_imu.c

    pidProfile->angle.level_strength =   cmsx_angleStrength;
    pidProfile->angle.level_limit =      cmsx_angleLimit;
    pidProfile->horizon.level_strength = cmsx_horizonStrength;
    pidProfile->horizon.transition =     cmsx_horizonTransition;

+    changePidProfile(pidProfileIndex);


rotorflight · 2024-05-16T10:35:26Z

src/main/cms/cms_menu_imu.c

@@ -619,6 +589,7 @@ static const void *cmsx_profileRescueOnExit(displayPort_t *pDisp, const OSD_Entr
    pidProfile->rescue.level_gain =         cmsx_rescueLevelGain;
    pidProfile->rescue.flip_gain =          cmsx_rescueFlipGain;

+    changePidProfile(pidProfileIndex);


rotorflight · 2024-05-16T10:35:32Z

src/main/cms/cms_menu_imu.c

    pidProfile->governor.headspeed = cmsx_govHeadspeed;
    pidProfile->governor.gain =      cmsx_govMasterGain;
    pidProfile->governor.p_gain =    cmsx_govP;
    pidProfile->governor.i_gain =    cmsx_govI;
    pidProfile->governor.d_gain =    cmsx_govD;
    pidProfile->governor.f_gain =    cmsx_govF;

+    changePidProfile(pidProfileIndex);


rotorflight · 2024-05-16T10:39:05Z

Okay. You can't change PID profile from the CMS. The only way at the moment is the adjustment function, which is associated to a Tx channel controlling it.

You are free to store the adjusted parameters to the cached PID profile pointer.
Then, you need to do the following:

If PID profile has changed (by adjfunc), DO NOTHING
If PID profile is the same the CMS has edited, call pidInitProfile(currentPidProfile)

In other words, if you are editing the active PID profile, and it is still active when the CMS has saved the new values, only then you need to call pidInitProfile. Otherwise you should not.

AndersHoglund · 2024-05-16T11:05:51Z

Profile change from CMS was visible in RFC. So at least CMS and RFC are in sync. But OK, if adj function does not, then we can not change profile from CMS. But then the CMS profile selection entry will have no other effect than what profile CMS will work on, no actual profile selection. Or should we allow profile selection only there, but not when working on profile items in CMS? I can see a use case for selecting profile via CMS, if user has no other way to change in the field. Lack of Tx channels/switches for the adj function for example. How are LUA dealing with this?

…MS has been working on.

rotorflight requested changes May 8, 2024

View reviewed changes

rotorflight requested changes May 13, 2024

View reviewed changes

AndersHoglund force-pushed the cms_cleanup_profiles branch 2 times, most recently from 9b0b054 to 4b5cf52 Compare May 14, 2024 06:15

AndersHoglund added 4 commits May 14, 2024 08:16

Bugfix CMS handling of profiles. Menu string changes for a bit more c…

6ef4c93

…larity.

CMS cleanup. RC rate was using FLOAT_t, native storage type is UNIT8_…

c7e35d1

…t. 240 dps was shown as 0.240. CMS now shows 24, smae as cli.

CMS profiles. Keep external and local profile items in sync on index …

2ecca06

…change.

CMS should not use pidInitProfile, instead use changePidProfile.

4cace94

AndersHoglund requested a review from rotorflight May 14, 2024 06:19

rotorflight requested changes May 16, 2024

View reviewed changes

Update current active PID profile only if it is the same as the one C…

990b386

…MS has been working on.

AndersHoglund force-pushed the cms_cleanup_profiles branch from 4b5cf52 to 990b386 Compare May 16, 2024 12:24

rotorflight approved these changes May 21, 2024

View reviewed changes

rotorflight merged commit 27b48d9 into rotorflight:master May 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMS bugfix and cleanup. #112

CMS bugfix and cleanup. #112

AndersHoglund commented May 7, 2024

rotorflight left a comment

rotorflight commented May 8, 2024 •

edited

Loading

AndersHoglund commented May 8, 2024 •

edited

Loading

AndersHoglund commented May 8, 2024

rotorflight left a comment •

edited

Loading

AndersHoglund commented May 13, 2024

rotorflight commented May 13, 2024

AndersHoglund commented May 13, 2024

rotorflight commented May 13, 2024

rotorflight commented May 13, 2024

rotorflight commented May 13, 2024

AndersHoglund commented May 13, 2024

AndersHoglund commented May 14, 2024 •

edited

Loading

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight May 16, 2024

rotorflight commented May 16, 2024

AndersHoglund commented May 16, 2024

CMS bugfix and cleanup. #112

CMS bugfix and cleanup. #112

Conversation

AndersHoglund commented May 7, 2024

rotorflight left a comment

Choose a reason for hiding this comment

rotorflight commented May 8, 2024 • edited Loading

AndersHoglund commented May 8, 2024 • edited Loading

AndersHoglund commented May 8, 2024

rotorflight left a comment • edited Loading

Choose a reason for hiding this comment

AndersHoglund commented May 13, 2024

rotorflight commented May 13, 2024

AndersHoglund commented May 13, 2024

rotorflight commented May 13, 2024

rotorflight commented May 13, 2024

rotorflight commented May 13, 2024

AndersHoglund commented May 13, 2024

AndersHoglund commented May 14, 2024 • edited Loading

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight May 16, 2024

Choose a reason for hiding this comment

rotorflight commented May 16, 2024

AndersHoglund commented May 16, 2024

rotorflight commented May 8, 2024 •

edited

Loading

AndersHoglund commented May 8, 2024 •

edited

Loading

rotorflight left a comment •

edited

Loading

AndersHoglund commented May 14, 2024 •

edited

Loading