From ec350b522aeb9bb52140165ee3b3d55deb933d8e Mon Sep 17 00:00:00 2001 From: Kyle Schwarz Date: Mon, 6 Oct 2025 15:36:55 -0400 Subject: [PATCH] DeviceSettings: Remove checksum validation --- .../icsneopy/device/idevicesettings.cpp | 2 +- device/idevicesettings.cpp | 44 ++++--------------- include/icsneo/device/idevicesettings.h | 5 +-- .../device/tree/radpluto/radplutosettings.h | 1 - .../device/tree/radstar2/radstar2settings.h | 1 - .../device/tree/vividcan/vividcansettings.h | 4 +- 6 files changed, 14 insertions(+), 43 deletions(-) diff --git a/bindings/python/icsneopy/device/idevicesettings.cpp b/bindings/python/icsneopy/device/idevicesettings.cpp index 31d091a..ea4771e 100644 --- a/bindings/python/icsneopy/device/idevicesettings.cpp +++ b/bindings/python/icsneopy/device/idevicesettings.cpp @@ -40,7 +40,7 @@ void init_idevicesettings(pybind11::module_& m) { .def("set_phy_enable", &IDeviceSettings::setPhyEnable, pybind11::call_guard()) .def("set_phy_mode", &IDeviceSettings::setPhyMode, pybind11::call_guard()) .def("set_phy_speed", &IDeviceSettings::setPhySpeed, pybind11::call_guard()) - .def("refresh", &IDeviceSettings::refresh, pybind11::arg("ignoreChecksum") = 0, pybind11::call_guard()); + .def("refresh", &IDeviceSettings::refresh, pybind11::call_guard()); } } // namespace icsneo diff --git a/device/idevicesettings.cpp b/device/idevicesettings.cpp index 81408a3..1af9c60 100644 --- a/device/idevicesettings.cpp +++ b/device/idevicesettings.cpp @@ -4,9 +4,9 @@ using namespace icsneo; -std::optional IDeviceSettings::CalculateGSChecksum(const std::vector& settings, std::optional knownSize) { +std::optional IDeviceSettings::CalculateGSChecksum(const std::vector& settings) { const uint16_t* p = reinterpret_cast(settings.data()); - size_t words = std::min(knownSize.value_or(0), settings.size()); + size_t words = settings.size(); if(words % 2 == 1) return std::nullopt; // Somehow settings is not word aligned words /= 2; @@ -159,15 +159,12 @@ bool IDeviceSettings::ValidateLINBaudrate(int64_t baudrate) { } } -bool IDeviceSettings::refresh(bool ignoreChecksum) { +bool IDeviceSettings::refresh() { if(disabled) { report(APIEvent::Type::SettingsNotAvailable, APIEvent::Severity::Error); return false; } - if(disableGSChecksumming) - ignoreChecksum = true; - std::vector rxSettings; bool ret = com->getSettingsSync(rxSettings); if(!ret) { @@ -175,24 +172,14 @@ bool IDeviceSettings::refresh(bool ignoreChecksum) { return false; } - constexpr size_t GsSize = 3 * sizeof(uint16_t); + constexpr size_t GsSize = 3 * sizeof(uint16_t); // if(rxSettings.size() < GsSize) { // We need to at least have the header of GLOBAL_SETTINGS report(APIEvent::Type::SettingsReadError, APIEvent::Severity::Error); return false; } - // The length of the settings structure sent to us - // This is the length the firmware thinks the current version of the structure is - const size_t rxLen = rxSettings.size() - GsSize; - const uint16_t gsVersion = rxSettings[0] | (rxSettings[1] << 8); - // The length of the settings last saved - // If the firmware is updated, it will have either extended (with zeros) or truncated - // the structure, but this value will continue to be set to the last saved value - const uint16_t gsLen = rxSettings[2] | (rxSettings[3] << 8); - - const uint16_t gsChecksum = rxSettings[4] | (rxSettings[5] << 8); rxSettings.erase(rxSettings.begin(), rxSettings.begin() + GsSize); if(gsVersion != GS_VERSION) { @@ -200,19 +187,6 @@ bool IDeviceSettings::refresh(bool ignoreChecksum) { return false; } - if(rxLen < gsLen) { - // We got less data, i.e. the firmware thinks the strucure is smaller than what - // was last saved. Usually this is due to a firmware downgrade. We'll ignore the - // checksum for now, because it will definitely be wrong. - ignoreChecksum = true; - } - - // We check the checksum against the data last saved - if(!ignoreChecksum && gsChecksum != CalculateGSChecksum(rxSettings, gsLen)) { - report(APIEvent::Type::SettingsChecksumError, APIEvent::Severity::Error); - return false; - } - settings = std::move(rxSettings); settingsInDeviceRAM = settings; settingsLoaded = true; @@ -261,7 +235,7 @@ bool IDeviceSettings::apply(bool temporary) { std::shared_ptr msg = std::dynamic_pointer_cast(com->waitForMessageSync([this, &bytestream]() { return com->sendCommand(Command::SetSettings, bytestream); - }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(1000))); + }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(2000))); if(!msg || msg->data[0] != 1) { // We did not receive a response // Attempt to get the settings from the device so we're up to date if possible @@ -272,7 +246,7 @@ bool IDeviceSettings::apply(bool temporary) { return false; } - refresh(true); // Refresh ignoring checksum + refresh(); // The device might modify the settings once they are applied, however in this case it does not update the checksum // We refresh to get these updates, update the checksum, and send it back so it's all in sync gsChecksum = CalculateGSChecksum(settings); @@ -287,7 +261,7 @@ bool IDeviceSettings::apply(bool temporary) { msg = std::dynamic_pointer_cast(com->waitForMessageSync([this, &bytestream]() { return com->sendCommand(Command::SetSettings, bytestream); - }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(1000))); + }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(2000))); if(!msg || msg->data[0] != 1) { // Attempt to get the settings from the device so we're up to date if possible if(refresh()) { @@ -342,7 +316,7 @@ bool IDeviceSettings::applyDefaults(bool temporary) { // This short wait helps on FIRE devices, otherwise the checksum might be wrong! std::this_thread::sleep_for(std::chrono::milliseconds(3)); - refresh(true); // Refresh ignoring checksum + refresh(); // The device might modify the settings once they are applied, however in this case it does not update the checksum // We refresh to get these updates, update the checksum, and send it back so it's all in sync std::vector bytestream; @@ -364,7 +338,7 @@ bool IDeviceSettings::applyDefaults(bool temporary) { msg = std::dynamic_pointer_cast(com->waitForMessageSync([this, &bytestream]() { return com->sendCommand(Command::SetSettings, bytestream); - }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(1000))); + }, std::make_shared(Command::SetSettings), std::chrono::milliseconds(2000))); if(!msg || msg->data[0] != 1) { // Attempt to get the settings from the device so we're up to date if possible if(refresh()) { diff --git a/include/icsneo/device/idevicesettings.h b/include/icsneo/device/idevicesettings.h index aaeae79..4f53b75 100644 --- a/include/icsneo/device/idevicesettings.h +++ b/include/icsneo/device/idevicesettings.h @@ -729,7 +729,7 @@ public: using TerminationGroup = std::vector; static constexpr uint16_t GS_VERSION = 5; - static std::optional CalculateGSChecksum(const std::vector& settings, std::optional knownSize = std::nullopt); + static std::optional CalculateGSChecksum(const std::vector& settings); static CANBaudrate GetEnumValueForBaudrate(int64_t baudrate); static int64_t GetBaudrateValueForEnum(CANBaudrate enumValue); static bool ValidateLINBaudrate(int64_t baudrate); @@ -738,7 +738,7 @@ public: virtual ~IDeviceSettings() {} bool ok() const { return !disabled && settingsLoaded; } - virtual bool refresh(bool ignoreChecksum = false); // Get from device + virtual bool refresh(); // Get from device // Send to device, if temporary device keeps settings in volatile RAM until power cycle, otherwise saved to EEPROM virtual bool apply(bool temporary = false); @@ -946,7 +946,6 @@ public: bool disabled = false; bool readonly = false; - bool disableGSChecksumming = false; std::atomic applyingSettings{false}; protected: diff --git a/include/icsneo/device/tree/radpluto/radplutosettings.h b/include/icsneo/device/tree/radpluto/radplutosettings.h index eee8c06..04fc199 100644 --- a/include/icsneo/device/tree/radpluto/radplutosettings.h +++ b/include/icsneo/device/tree/radpluto/radplutosettings.h @@ -75,7 +75,6 @@ static_assert(sizeof(radpluto_settings_t) == 322, "RAD-Pluto Settings are not pa class RADPlutoSettings : public IDeviceSettings { public: RADPlutoSettings(std::shared_ptr com) : IDeviceSettings(com, sizeof(radpluto_settings_t)) { - disableGSChecksumming = true; } const CAN_SETTINGS* getCANSettingsFor(Network net) const override { diff --git a/include/icsneo/device/tree/radstar2/radstar2settings.h b/include/icsneo/device/tree/radstar2/radstar2settings.h index 4630955..cbd12de 100644 --- a/include/icsneo/device/tree/radstar2/radstar2settings.h +++ b/include/icsneo/device/tree/radstar2/radstar2settings.h @@ -70,7 +70,6 @@ typedef struct { class RADStar2Settings : public IDeviceSettings { public: RADStar2Settings(std::shared_ptr com) : IDeviceSettings(com, sizeof(radstar2_settings_t)) { - disableGSChecksumming = true; } const CAN_SETTINGS* getCANSettingsFor(Network net) const override { diff --git a/include/icsneo/device/tree/vividcan/vividcansettings.h b/include/icsneo/device/tree/vividcan/vividcansettings.h index fd80c74..ab97f54 100644 --- a/include/icsneo/device/tree/vividcan/vividcansettings.h +++ b/include/icsneo/device/tree/vividcan/vividcansettings.h @@ -89,11 +89,11 @@ public: }; } - bool refresh(bool ignoreChecksum = false) override { + bool refresh() override { // Because VividCAN uses a nonstandard 16-bit termination_enables // we need to keep the standard 64-bit values in memory and update // the structure when applying - if(!IDeviceSettings::refresh(ignoreChecksum)) + if(!IDeviceSettings::refresh()) return false; auto cfg = getStructurePointer(); if(cfg == nullptr)