Fix: Handle WLED 0.16.0+ binary naming changes on OTA updates#65
Fix: Handle WLED 0.16.0+ binary naming changes on OTA updates#65Moustachauve wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in WLED OTA updates where binary naming conventions changed in version 0.16.0+. It introduces robust semantic version parsing to intelligently remap legacy device release names (e.g., Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an important issue with OTA updates for WLED devices by handling changes in binary naming conventions from version 0.16.0 onwards. The introduction of a SemanticVersion struct is a solid approach, and the changes are accompanied by a good set of unit tests. My review includes a few suggestions to improve efficiency in the DeviceUpdateService and points out two issues in the new tests: one test case will fail as written, and another suggests a potential flaw in the logic for handling pre-release versions, which could prevent the fix from working correctly in those scenarios.
| #expect(DeviceUpdateService.determineAsset(byRelease: "esp32_v4", targetVersion: targetVersion) == "ESP32") | ||
|
|
||
| // Same for ESP8266 if configured | ||
| #expect(DeviceUpdateService.determineAsset(byRelease: "ESP8266_V4", targetVersion: targetVersion) == "ESP8266") |
There was a problem hiding this comment.
This test case will fail because the releaseNameOverrides dictionary in DeviceUpdateService does not contain an entry for "ESP8266_V4". The function will return the original release name "ESP8266_V4", not "ESP8266".
If ESP8266_V4 is another legacy name that needs remapping, please add it to the releaseNameOverrides dictionary. Otherwise, this test case should be removed or corrected.
| let targetVersionOlderBeta = "0.15.0-b5" | ||
|
|
||
| #expect(DeviceUpdateService.determineAsset(byRelease: "ESP32_V4", targetVersion: targetVersionOlderBeta) == "ESP32_V4") | ||
| #expect(DeviceUpdateService.determineAsset(byRelease: "ESP32_V4", targetVersion: targetVersionBeta) == "ESP32_V4") |
There was a problem hiding this comment.
This test asserts that for a pre-release version like 0.16.0-b2, the asset name override should not be applied. According to standard semantic versioning, 0.16.0-b2 is indeed less than 0.16.0, so the test passes with the current implementation.
However, it's likely that all builds for the 0.16.0 release cycle, including betas, use the new asset naming convention. If that's the case, the override should be applied, and this test should expect "ESP32".
This would require adjusting the version comparison logic in DeviceUpdateService.determineAsset to treat 0.16.0-b2 as being at or beyond the 0.16.0 threshold, perhaps by ignoring pre-release identifiers for this specific check.
| /** | ||
| * Maps deprecated or transitional release names to the release name that should be used | ||
| * for OTA updates. This handles cases where the binary name changes between WLED releases. | ||
| * | ||
| * For example, devices running the ESP32_V4 tech-preview (WLED 0.15.x) must be updated | ||
| * with the standard ESP32 binary when upgrading to 0.16.0 or later, as ESP32_V4 assets will | ||
| * no longer be published in those releases. Within 0.15.x, ESP32_V4 assets still exist | ||
| * and no remapping is needed. | ||
| * | ||
| * These overrides are only applied when the target version is [RELEASE_OVERRIDES_MIN_VERSION] | ||
| * or greater. | ||
| * | ||
| * See: https://github.com/Moustachauve/WLED-Android/issues/129 | ||
| */ | ||
| static let releaseNameOverrides: [String: String] = [ | ||
| "ESP32_V4": "ESP32", | ||
| ] | ||
|
|
||
| /** | ||
| * The minimum target version from which [RELEASE_NAME_OVERRIDES] are applied. | ||
| * Overrides only take effect when upgrading to this version or later. | ||
| */ |
There was a problem hiding this comment.
The documentation for these constants contains placeholders like [RELEASE_OVERRIDES_MIN_VERSION] (line 46) and [RELEASE_NAME_OVERRIDES] (line 55). These should be replaced with the actual variable names (e.g., releaseOverridesMinVersion) for better clarity and to support IDE features like jump-to-definition.
| guard let minOverridesVersion = SemanticVersion(releaseOverridesMinVersion) else { | ||
| return releaseName | ||
| } |
There was a problem hiding this comment.
For improved performance, the releaseOverridesMinVersion constant should be parsed into a SemanticVersion only once and stored as a private static property, rather than being re-parsed on every call to this function. Since you control this constant, you can use force-unwrapping (!) to ensure it's valid at compile time.
Example to be placed with other static properties:
private static let minOverridesVersion = SemanticVersion(releaseOverridesMinVersion)!You could then use Self.minOverridesVersion directly in the comparison on line 149, removing this guard statement.
| let key = releaseName.uppercased() | ||
| if let override = releaseNameOverrides.first(where: { $0.key.uppercased() == key })?.value { | ||
| return override | ||
| } |
There was a problem hiding this comment.
This lookup iterates through the releaseNameOverrides dictionary using first(where:), which is an O(n) operation. A direct dictionary lookup is much more efficient (O(1) on average) and readable. Since the keys in the dictionary are already uppercase, you can perform a case-insensitive lookup directly.
if let override = releaseNameOverrides[releaseName.uppercased()] {
return override
}1 participant
This PR replicates the recent fix from the Android app (WLED-Android#129) to handle WLED binary naming convention changes introduced in version 0.16.0+.
Previously, older devices had release names like
ESP32_V4. Starting with WLED 0.16.0, the assets published on GitHub are simply namedESP32. When a user attempts an OTA update from an older version (e.g.,0.15.x) using anESP32_V4device to0.16.0+, the app would previously fail to find the matchingESP32_V4asset in the new release.This update introduces semantic version parsing to conditionally remap these legacy release names to their new, standard equivalents only when upgrading to
0.16.0or greater, ensuring a smooth OTA update experience.Changes Included
semver4jin Java), correctly handling major, minor, patch, and</>comparisons for pre-release identifiers (e.g.,-b1,-a2).releaseNameOverridesdictionary to map legacy names like"ESP32_V4"->"ESP32".releaseOverridesMinVersion("0.16.0").determineAsset(byRelease:targetVersion:)method. It parses the target version, checks if it crosses the0.16.0threshold, and safely applies the transition mappings. If the target version fails to parse, it gracefully logs a warning and falls back to the exact raw release name.Testingframework to validate standard vs. threshold semantic version comparisons as well as test various edge cases when determining the final firmware asset string.Testing
SemanticVersionTeststo verify version inequalities resulting from parsing stable releases vs beta pre-releases.DeviceUpdateServiceTeststo verify the asset threshold mapping logic against a matrix of various legacy labels and target WLED versions.