diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 98a9c9d934..410ce8fed2 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -63,10 +63,8 @@ #if !defined(ARCH_STM32WL) && !MESHTASTIC_EXCLUDE_I2C && !MESHTASTIC_EXCLUDE_ACCELEROMETER #include "motion/AccelerometerThread.h" #endif -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) +// Unguarded: serialConfigIsValid() is needed on every platform. The class stays guarded in the header. #include "SerialModule.h" -#endif AdminModule *adminModule; @@ -1271,14 +1269,13 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c) break; case meshtastic_ModuleConfig_serial_tag: LOG_INFO("Set module config: Serial"); -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) - if (!SerialModule::isValidConfig(c.payload_variant.serial)) { + // No architecture guard: the check and the store below must agree on every platform. + // disableBluetooth() self-guards on HAS_BLUETOOTH, so it is empty where there is no radio. + if (!serialConfigIsValid(c.payload_variant.serial)) { LOG_ERROR("Invalid serial config"); return false; } disableBluetooth(); // Disable Bluetooth to prevent interference during Serial configuration -#endif moduleConfig.has_serial = true; moduleConfig.serial = c.payload_variant.serial; break; diff --git a/src/modules/SerialModule.cpp b/src/modules/SerialModule.cpp index cb481e6a5a..ef26bc360e 100644 --- a/src/modules/SerialModule.cpp +++ b/src/modules/SerialModule.cpp @@ -49,6 +49,30 @@ #include "meshSolarApp.h" #endif +// Outside the architecture guard on purpose: config validation, not serial I/O. See SerialModule.h. +bool serialConfigIsValid(const meshtastic_ModuleConfig_SerialConfig &config) +{ + if (config.override_console_serial_port && !IS_ONE_OF(config.mode, meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, + meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, + meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG)) { + const char *warning = "Invalid Serial config: override console serial port is only supported in NMEA, CalTopo, or MS " + "Config output-only modes."; + LOG_ERROR(warning); +#ifndef PIO_UNIT_TESTING + meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); + if (cn) { + cn->level = meshtastic_LogRecord_Level_ERROR; + cn->time = getValidTime(RTCQualityFromNet); + snprintf(cn->message, sizeof(cn->message), "%s", warning); + service->sendClientNotification(cn); + } +#endif + return false; + } + + return true; +} + #if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040) || defined(ARCH_STM32WL)) && \ !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) @@ -86,29 +110,6 @@ static Print *serialPrint = &SERIAL_PRINT_OBJECT; char serialBytes[512]; size_t serialPayloadSize; -bool SerialModule::isValidConfig(const meshtastic_ModuleConfig_SerialConfig &config) -{ - if (config.override_console_serial_port && !IS_ONE_OF(config.mode, meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, - meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, - meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG)) { - const char *warning = - "Invalid Serial config: override console serial port is only supported in NMEA and CalTopo output-only modes."; - LOG_ERROR(warning); -#ifndef PIO_UNIT_TESTING - meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); - if (cn) { - cn->level = meshtastic_LogRecord_Level_ERROR; - cn->time = getValidTime(RTCQualityFromNet); - snprintf(cn->message, sizeof(cn->message), "%s", warning); - service->sendClientNotification(cn); - } -#endif - return false; - } - - return true; -} - SerialModuleRadio::SerialModuleRadio() : SinglePortModule("SerialModuleRadio", meshtastic_PortNum_SERIAL_APP) { switch (moduleConfig.serial.mode) { diff --git a/src/modules/SerialModule.h b/src/modules/SerialModule.h index 5cbca78243..8121486e60 100644 --- a/src/modules/SerialModule.h +++ b/src/modules/SerialModule.h @@ -8,6 +8,11 @@ #include #include +// Is this serial config one we will accept? Outside the architecture guard below because it touches +// no serial hardware, and AdminModule must run it on every platform - including those where +// SerialModule itself does not exist. Logs and notifies the client on rejection. +bool serialConfigIsValid(const meshtastic_ModuleConfig_SerialConfig &config); + #if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040) || defined(ARCH_STM32WL)) && \ !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) @@ -20,8 +25,6 @@ class SerialModule : public StreamAPI, private concurrency::OSThread public: SerialModule(); - static bool isValidConfig(const meshtastic_ModuleConfig_SerialConfig &config); - protected: virtual int32_t runOnce() override; diff --git a/test/test_serial/SerialModule.cpp b/test/test_serial/SerialModule.cpp index 39992b3a2d..6539d0ad34 100644 --- a/test/test_serial/SerialModule.cpp +++ b/test/test_serial/SerialModule.cpp @@ -5,20 +5,14 @@ #ifdef ARCH_PORTDUINO #include "configuration.h" -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) #include "modules/SerialModule.h" -#endif - -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) // Test that empty configuration is valid. void test_serialConfigEmptyIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = {}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that basic enabled configuration is valid. @@ -26,61 +20,71 @@ void test_serialConfigEnabledIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = {.enabled = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and NMEA mode is valid. void test_serialConfigWithOverrideConsoleNmeaModeIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA, .override_console_serial_port = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and CalTopo mode is valid. void test_serialConfigWithOverrideConsoleCalTopoModeIsValid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO, .override_console_serial_port = true}; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); +} + +// Test that configuration with override_console_serial_port and MS Config mode is valid. +void test_serialConfigWithOverrideConsoleMsConfigModeIsValid(void) +{ + meshtastic_ModuleConfig_SerialConfig config = {.enabled = true, + .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_MS_CONFIG, + .override_console_serial_port = true}; + + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and DEFAULT mode is invalid. void test_serialConfigWithOverrideConsoleDefaultModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and SIMPLE mode is invalid. void test_serialConfigWithOverrideConsoleSimpleModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and TEXTMSG mode is invalid. void test_serialConfigWithOverrideConsoleTextMsgModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that configuration with override_console_serial_port and PROTO mode is invalid. void test_serialConfigWithOverrideConsoleProtoModeIsInvalid(void) { meshtastic_ModuleConfig_SerialConfig config = { - .enabled = true, .override_console_serial_port = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO}; + .enabled = true, .mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO, .override_console_serial_port = true}; - TEST_ASSERT_FALSE(SerialModule::isValidConfig(config)); + TEST_ASSERT_FALSE(serialConfigIsValid(config)); } // Test that various modes work without override_console_serial_port. @@ -90,53 +94,45 @@ void test_serialConfigVariousModesWithoutOverrideAreValid(void) // Test DEFAULT mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_DEFAULT; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test SIMPLE mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_SIMPLE; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test TEXTMSG mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_TEXTMSG; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test PROTO mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_PROTO; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test NMEA mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_NMEA; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); // Test CALTOPO mode config.mode = meshtastic_ModuleConfig_SerialConfig_Serial_Mode_CALTOPO; - TEST_ASSERT_TRUE(SerialModule::isValidConfig(config)); + TEST_ASSERT_TRUE(serialConfigIsValid(config)); } -#endif // Architecture check - void setup() { initializeTestEnvironment(); -#if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ - !defined(CONFIG_IDF_TARGET_ESP32C3) UNITY_BEGIN(); RUN_TEST(test_serialConfigEmptyIsValid); RUN_TEST(test_serialConfigEnabledIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleNmeaModeIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleCalTopoModeIsValid); + RUN_TEST(test_serialConfigWithOverrideConsoleMsConfigModeIsValid); RUN_TEST(test_serialConfigWithOverrideConsoleDefaultModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleSimpleModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleTextMsgModeIsInvalid); RUN_TEST(test_serialConfigWithOverrideConsoleProtoModeIsInvalid); RUN_TEST(test_serialConfigVariousModesWithoutOverrideAreValid); exit(UNITY_END()); -#else - LOG_WARN("This test requires ESP32, NRF52, or RP2040 architecture"); - UNITY_BEGIN(); - UNITY_END(); -#endif } #else void setup() @@ -144,7 +140,7 @@ void setup() initializeTestEnvironment(); LOG_WARN("This test requires the ARCH_PORTDUINO variant"); UNITY_BEGIN(); - UNITY_END(); + exit(UNITY_END()); } #endif void loop() {}