From e64703308bc79597dc06e22177d2265ddad85011 Mon Sep 17 00:00:00 2001 From: Bruce Luckcuck Date: Mon, 23 Sep 2019 19:58:43 -0400 Subject: [PATCH] Revise feature logic to separate runtime and config settings Isolates and prevents changes to runtime active features. Any changes to enabled features are deferred until after a save/reboot. Simplifies the previous logic. Prevents potential failures when features are changed at runtime but the underlying code is not capabile of dynamic reconfiguration. --- src/main/cli/cli.c | 29 ++------------ src/main/config/feature.c | 52 +++++++++++++++++++++++--- src/main/config/feature.h | 9 +++-- src/main/fc/config.c | 28 ++++++-------- src/main/fc/config.h | 1 - src/main/msp/msp.c | 26 ++----------- src/main/target/COLIBRI_RACE/i2c_bst.c | 3 +- src/test/unit/cli_unittest.cc | 1 - 8 files changed, 71 insertions(+), 78 deletions(-) diff --git a/src/main/cli/cli.c b/src/main/cli/cli.c index 3dfb03335..689535ce6 100644 --- a/src/main/cli/cli.c +++ b/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"); diff --git a/src/main/config/feature.c b/src/main/config/feature.c index b703592e6..1ebadc557 100644 --- a/src/main/config/feature.c +++ b/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; } diff --git a/src/main/config/feature.h b/src/main/config/feature.h index 28fefe0e2..92c580a06 100644 --- a/src/main/config/feature.h +++ b/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 featureSet(const uint32_t mask, uint32_t *features); -void featureClear(const uint32_t mask, uint32_t *features); +void featureConfigSet(const uint32_t mask); +void featureConfigClear(const uint32_t mask); +void featureConfigReplace(const uint32_t mask); diff --git a/src/main/fc/config.c b/src/main/fc/config.c index b28cc6051..c5bf5cbd1 100644 --- a/src/main/fc/config.c +++ b/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) diff --git a/src/main/fc/config.h b/src/main/fc/config.h index 9e18317f5..5b3065d7c 100644 --- a/src/main/fc/config.h +++ b/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); diff --git a/src/main/msp/msp.c b/src/main/msp/msp.c index ef046d91b..f5b1d06b2 100644 --- a/src/main/msp/msp.c +++ b/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) { - writeEEPROMWithFeatures(featureMaskCopy); - } else { - writeEEPROM(); - } + 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); - if (!featureMaskIsCopied) { - featureMaskIsCopied = true; - } - + featureConfigReplace(sbufReadU32(src)); break; #ifdef USE_BEEPER diff --git a/src/main/target/COLIBRI_RACE/i2c_bst.c b/src/main/target/COLIBRI_RACE/i2c_bst.c index 4f925001f..f1303bb66 100644 --- a/src/main/target/COLIBRI_RACE/i2c_bst.c +++ b/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(); - featureEnable(bstRead32()); // features bitmap + featureConfigReplace(bstRead32()); // features bitmap #ifdef SERIALRX_UART if (featureIsEnabled(FEATURE_RX_SERIAL)) { serialConfigMutable()->portConfigs[SERIALRX_UART].functionMask = FUNCTION_RX_SERIAL; diff --git a/src/test/unit/cli_unittest.cc b/src/test/unit/cli_unittest.cc index c6d9fa3f9..6fa3d5554 100644 --- a/src/test/unit/cli_unittest.cc +++ b/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; }