Revise feature logic to separate runtime and config settings (#9029)

Revise feature logic to separate runtime and config settings

src/main/cli/cli.c

@@ -196,9 +196,6 @@ static bool configIsInCopy = false;
 static int8_t pidProfileIndexToUse = CURRENT_PROFILE_INDEX;
 static int8_t rateProfileIndexToUse = CURRENT_PROFILE_INDEX;
 
-static bool featureMaskIsCopied = false;
-static uint32_t featureMaskCopy;
-
 #ifdef USE_CLI_BATCH
 static bool commandBatchActive = false;
 static bool commandBatchError = false;
@@ -3123,15 +3120,6 @@ static void cliMcuId(char *cmdline)
     cliPrintLinef("mcu_id %08x%08x%08x", U_ID_0, U_ID_1, U_ID_2);
 }
 
-static uint32_t *getFeatureMask(void)
-{
-    if (featureMaskIsCopied) {
-        return &featureMaskCopy;
-    } else {
-        return &featureConfigMutable()->enabledFeatures;
-    }
-}
-
 static void printFeature(dumpFlags_t dumpMask, const uint32_t mask, const uint32_t defaultMask, const char *headingStr)
 {
     headingStr = cliPrintSectionHeading(dumpMask, false, headingStr);
@@ -3162,7 +3150,7 @@ static void printFeature(dumpFlags_t dumpMask, const uint32_t mask, const uint32
 static void cliFeature(char *cmdline)
 {
     uint32_t len = strlen(cmdline);
-    const uint32_t mask = *getFeatureMask();
+    const uint32_t mask = featureConfig()->enabledFeatures;
     if (len == 0) {
         cliPrint("Enabled: ");
         for (uint32_t i = 0; ; i++) {
@@ -3185,10 +3173,6 @@ static void cliFeature(char *cmdline)
         cliPrintLinefeed();
         return;
     } else {
-        if (!featureMaskIsCopied && !configIsInCopy) {
-            featureMaskCopy = featureConfig()->enabledFeatures;
-            featureMaskIsCopied = true;
-        }
         uint32_t feature;
 
         bool remove = false;
@@ -3220,10 +3204,10 @@ static void cliFeature(char *cmdline)
                 }
 #endif
                 if (remove) {
-                    featureClear(feature, getFeatureMask());
+                    featureConfigClear(feature);
                     cliPrint("Disabled");
                 } else {
-                    featureSet(feature, getFeatureMask());
+                    featureConfigSet(feature);
                     cliPrint("Enabled");
                 }
                 cliPrintLinef(" %s", featureNames[i]);
@@ -4173,11 +4157,6 @@ static bool prepareSave(void)
 #endif
 #endif // USE_BOARD_INFO
 
-    if (featureMaskIsCopied) {
-        featureDisableAll();
-        featureEnable(featureMaskCopy);
-    }
-
     return true;
 }
 
@@ -6067,7 +6046,7 @@ static void printConfig(char *cmdline, bool doDiff)
 #endif
 #endif
 
-            printFeature(dumpMask, featureConfig_Copy.enabledFeatures, *getFeatureMask(), "feature");
+            printFeature(dumpMask, featureConfig_Copy.enabledFeatures, featureConfig()->enabledFeatures, "feature");
 
 #if defined(USE_BEEPER)
             printBeeper(dumpMask, beeperConfig_Copy.beeper_off_flags, beeperConfig()->beeper_off_flags, "beeper", BEEPER_ALLOWED_MODES, "beeper");

src/main/config/feature.c

@@ -35,32 +35,74 @@ PG_RESET_TEMPLATE(featureConfig_t, featureConfig,
     .enabledFeatures = DEFAULT_FEATURES | DEFAULT_RX_FEATURE | FEATURE_DYNAMIC_FILTER | FEATURE_ANTI_GRAVITY | FEATURE_AIRMODE,
 );
 
-void featureSet(const uint32_t mask, uint32_t *features)
+static uint32_t runtimeFeatureMask;
+
+void featureInit(void)
+{
+    runtimeFeatureMask = featureConfig()->enabledFeatures;
+}
+
+static void featureSet(const uint32_t mask, uint32_t *features)
 {
     *features |= mask;
 }
 
-void featureClear(const uint32_t mask, uint32_t *features)
+static void featureClear(const uint32_t mask, uint32_t *features)
 {
     *features &= ~(mask);
 }
 
+// Determines if the feature is enabled (active) in the runtime state.
+// This is the primary funciton used by code that wants to know if a
+// feature is available.
 bool featureIsEnabled(const uint32_t mask)
+{
+    return runtimeFeatureMask & mask;
+}
+
+// Determines if the feature is configured (set in the configuration). Doesn't mean the
+// feature is active in the runtime. This function *SHOULD ONLY* be used in the config check
+// performed at startup and when writing to EEPROM.
+bool featureIsConfigured(const uint32_t mask)
 {
     return featureConfig()->enabledFeatures & mask;
 }
 
+// Updates the configuration *AND* runtime state of a feature.
+// Used *ONLY* by the config check process that runs at startup and EEPROM save.
 void featureEnable(const uint32_t mask)
+{
+    featureSet(mask, &featureConfigMutable()->enabledFeatures);
+    featureSet(mask, &runtimeFeatureMask);
+}
+
+// Updates the configuration *AND* runtime state of a feature.
+// Used *ONLY* by the config check process that runs at startup and EEPROM save.
+void featureDisable(const uint32_t mask)
+{
+    featureClear(mask, &featureConfigMutable()->enabledFeatures);
+    featureClear(mask, &runtimeFeatureMask);
+}
+
+// Sets the configuration state of the feature and *DOES NOT* change the runtime state.
+// For example, used by the CLI "feature" command. Use this function if you want to
+// enable a feature that will be active after save/reboot.
+void featureConfigSet(const uint32_t mask)
 {
     featureSet(mask, &featureConfigMutable()->enabledFeatures);
 }
 
-void featureDisable(const uint32_t mask)
+// Sets the configuration state of the feature and *DOES NOT* change the runtime state.
+// For example, used by the CLI "feature" command. Use this function if you want to
+// disable a feature after a save/reboot.
+void featureConfigClear(const uint32_t mask)
 {
     featureClear(mask, &featureConfigMutable()->enabledFeatures);
 }
 
-void featureDisableAll(void)
+// Sets the configuration state of all features and *DOES NOT* change the runtime state.
+// For example, used by MSP to update all the configured features in one go.
+void featureConfigReplace(const uint32_t mask)
 {
-    featureConfigMutable()->enabledFeatures = 0;
+    featureConfigMutable()->enabledFeatures = mask;
 }

src/main/config/feature.h

@@ -62,10 +62,11 @@ typedef struct featureConfig_s {
 
 PG_DECLARE(featureConfig_t, featureConfig);
 
+void featureInit(void);
 bool featureIsEnabled(const uint32_t mask);
+bool featureIsConfigured(const uint32_t mask);
 void featureEnable(const uint32_t mask);
 void featureDisable(const uint32_t mask);
-void featureDisableAll(void);
+void featureConfigSet(const uint32_t mask);
-
+void featureConfigClear(const uint32_t mask);
-void featureSet(const uint32_t mask, uint32_t *features);
+void featureConfigReplace(const uint32_t mask);
-void featureClear(const uint32_t mask, uint32_t *features);

src/main/fc/config.c

@@ -272,41 +272,41 @@ static void validateAndFixConfig(void)
     buildAlignmentFromStandardAlignment(&gyroDeviceConfigMutable(1)->customAlignment, gyroDeviceConfig(1)->alignment);
 #endif
 
-    if (!(featureIsEnabled(FEATURE_RX_PARALLEL_PWM) || featureIsEnabled(FEATURE_RX_PPM) || featureIsEnabled(FEATURE_RX_SERIAL) || featureIsEnabled(FEATURE_RX_MSP) || featureIsEnabled(FEATURE_RX_SPI))) {
+    if (!(featureIsConfigured(FEATURE_RX_PARALLEL_PWM) || featureIsConfigured(FEATURE_RX_PPM) || featureIsConfigured(FEATURE_RX_SERIAL) || featureIsConfigured(FEATURE_RX_MSP) || featureIsConfigured(FEATURE_RX_SPI))) {
         featureEnable(DEFAULT_RX_FEATURE);
     }
 
-    if (featureIsEnabled(FEATURE_RX_PPM)) {
+    if (featureIsConfigured(FEATURE_RX_PPM)) {
         featureDisable(FEATURE_RX_SERIAL | FEATURE_RX_PARALLEL_PWM | FEATURE_RX_MSP | FEATURE_RX_SPI);
     }
 
-    if (featureIsEnabled(FEATURE_RX_MSP)) {
+    if (featureIsConfigured(FEATURE_RX_MSP)) {
         featureDisable(FEATURE_RX_SERIAL | FEATURE_RX_PARALLEL_PWM | FEATURE_RX_PPM | FEATURE_RX_SPI);
     }
 
-    if (featureIsEnabled(FEATURE_RX_SERIAL)) {
+    if (featureIsConfigured(FEATURE_RX_SERIAL)) {
         featureDisable(FEATURE_RX_PARALLEL_PWM | FEATURE_RX_MSP | FEATURE_RX_PPM | FEATURE_RX_SPI);
     }
 
 #ifdef USE_RX_SPI
-    if (featureIsEnabled(FEATURE_RX_SPI)) {
+    if (featureIsConfigured(FEATURE_RX_SPI)) {
         featureDisable(FEATURE_RX_SERIAL | FEATURE_RX_PARALLEL_PWM | FEATURE_RX_PPM | FEATURE_RX_MSP);
     }
 #endif // USE_RX_SPI
 
-    if (featureIsEnabled(FEATURE_RX_PARALLEL_PWM)) {
+    if (featureIsConfigured(FEATURE_RX_PARALLEL_PWM)) {
         featureDisable(FEATURE_RX_SERIAL | FEATURE_RX_MSP | FEATURE_RX_PPM | FEATURE_RX_SPI);
     }
 
 #if defined(USE_ADC)
-    if (featureIsEnabled(FEATURE_RSSI_ADC)) {
+    if (featureIsConfigured(FEATURE_RSSI_ADC)) {
         rxConfigMutable()->rssi_channel = 0;
         rxConfigMutable()->rssi_src_frame_errors = false;
     } else
 #endif
     if (rxConfigMutable()->rssi_channel
 #if defined(USE_PWM) || defined(USE_PPM)
-        || featureIsEnabled(FEATURE_RX_PPM) || featureIsEnabled(FEATURE_RX_PARALLEL_PWM)
+        || featureIsConfigured(FEATURE_RX_PPM) || featureIsConfigured(FEATURE_RX_PARALLEL_PWM)
 #endif
         ) {
         rxConfigMutable()->rssi_src_frame_errors = false;
@@ -340,7 +340,7 @@ static void validateAndFixConfig(void)
 #endif
 
     if (
-        featureIsEnabled(FEATURE_3D) || !featureIsEnabled(FEATURE_GPS)
+        featureIsConfigured(FEATURE_3D) || !featureIsConfigured(FEATURE_GPS)
 #if !defined(USE_GPS) || !defined(USE_GPS_RESCUE)
         || true
 #endif
@@ -665,6 +665,8 @@ bool readEEPROM(void)
     // Sanity check, read flash
     bool success = loadEEPROM();
 
+    featureInit();
+
     validateAndFixConfig();
 
     activateConfig();
@@ -693,14 +695,6 @@ void writeEEPROM(void)
     writeUnmodifiedConfigToEEPROM();
 }
 
-void writeEEPROMWithFeatures(uint32_t features)
-{
-    featureDisableAll();
-    featureEnable(features);
-
-    writeEEPROM();
-}
-
 bool resetEEPROM(bool useCustomDefaults)
 {
 #if !defined(USE_CUSTOM_DEFAULTS)

src/main/fc/config.h

@@ -71,7 +71,6 @@ void initEEPROM(void);
 bool resetEEPROM(bool useCustomDefaults);
 bool readEEPROM(void);
 void writeEEPROM(void);
-void writeEEPROMWithFeatures(uint32_t features);
 void writeUnmodifiedConfigToEEPROM(void);
 void ensureEEPROMStructureIsValid(void);
 

src/main/msp/msp.c

@@ -185,18 +185,6 @@ typedef enum {
 static bool vtxTableNeedsInit = false;
 #endif
 
-static bool featureMaskIsCopied = false;
-static uint32_t featureMaskCopy;
-
-static uint32_t getFeatureMask(void)
-{
-    if (featureMaskIsCopied) {
-        return featureMaskCopy;
-    } else {
-        return featureConfig()->enabledFeatures;
-    }
-}
-
 static int mspDescriptor = 0;
 
 mspDescriptor_t mspDescriptorAlloc(void)
@@ -644,7 +632,7 @@ static bool mspCommonProcessOutCommand(uint8_t cmdMSP, sbuf_t *dst, mspPostProce
         break;
 
     case MSP_FEATURE_CONFIG:
-        sbufWriteU32(dst, getFeatureMask());
+        sbufWriteU32(dst, featureConfig()->enabledFeatures);
         break;
 
 #ifdef USE_BEEPER
@@ -2547,11 +2535,7 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, uint8_t cmdMSP,
             return MSP_RESULT_ERROR;
         }
 
-        if (featureMaskIsCopied) {
+        writeEEPROM();
-            writeEEPROMWithFeatures(featureMaskCopy);
-        } else {
-            writeEEPROM();
-        }
         readEEPROM();
 
 #ifdef USE_VTX_TABLE
@@ -2814,11 +2798,7 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, uint8_t cmdMSP,
         break;
 #endif // USE_GPS
     case MSP_SET_FEATURE_CONFIG:
-        featureMaskCopy = sbufReadU32(src);
+        featureConfigReplace(sbufReadU32(src));
-        if (!featureMaskIsCopied) {
-            featureMaskIsCopied = true;
-        }
-
         break;
 
 #ifdef USE_BEEPER

src/main/target/COLIBRI_RACE/i2c_bst.c

@@ -553,8 +553,7 @@ static bool bstSlaveProcessWriteCommand(uint8_t bstWriteCommand)
             readEEPROM();
             break;
         case BST_SET_FEATURE:
-            featureDisableAll();
+            featureConfigReplace(bstRead32()); // features bitmap
-            featureEnable(bstRead32()); // features bitmap
 #ifdef SERIALRX_UART
             if (featureIsEnabled(FEATURE_RX_SERIAL)) {
                 serialConfigMutable()->portConfigs[SERIALRX_UART].functionMask = FUNCTION_RX_SERIAL;

src/test/unit/cli_unittest.cc

@@ -282,7 +282,6 @@ uint8_t getCurrentControlRateProfileIndex(void){ return 1; }
 void changeControlRateProfile(uint8_t) {}
 void resetAllRxChannelRangeConfigurations(rxChannelRangeConfig_t *) {}
 void writeEEPROM() {}
-void writeEEPROMWithFeatures(uint32_t) {}
 serialPortConfig_t *serialFindPortConfiguration(serialPortIdentifier_e) {return NULL; }
 baudRate_e lookupBaudRateIndex(uint32_t){return BAUD_9600; }
 serialPortUsage_t *findSerialPortUsageByIdentifier(serialPortIdentifier_e){ return NULL; }