Merge upstream #4593
Merge upstream #4593
Conversation
…nge strobe frequency range
… from intensity slider, revert dumb "fix"
…, add new effects to list
increase the random call upper limit, so the last edge is also considered
Merge master into Aircookie-master
- Increase upper limit for random call to allow the last edge to be chosen - code style changes
…o Aircookie-master
Add semicolons and braces in WS2812FX::mode_HIVE_strobing_segments that got lost in auto-merge
Update upstream to 0.13.3
WalkthroughThis update introduces several changes across multiple files. The repository now ignores the VS Code extensions file by adding an entry in the ignore list. The metadata in the package file has been updated with new URLs. The naming convention for release binaries has been modified in the output script. Changes to the build configuration include adjustments to environments, exception handling flags, and upload speed. A new section describing hexagon-based effects has been added to the README. Finally, the effect definitions have been expanded with new HIVE modes and a constant has been adjusted in the FX function file. Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
readme.md (2)
22-22: Grammar issue with article before ESP32The article "a" should be "an" before "ESP32" since ESP32 starts with a vowel sound.
-Using a ESP32 over an ESP8266 is highly recommended (especially on strobing effects). +Using an ESP32 over an ESP8266 is highly recommended (especially on strobing effects).🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: Use the indefinite article “an” before nouns that start with a vowel sound.
Context: ... chosen palette) - more to come Using a ESP32 over an ESP8266 is highly recomme...(AI_EN_LECTOR_REPLACEMENT_DETERMINER_A_AN)
25-25: Missing alt text for imageThe image doesn't have alternative text, which is important for accessibility.
-<img src="/images/hives.jpg" width="50%"> +<img src="/images/hives.jpg" alt="Hexagon structure with WLED effects" width="50%">🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
25-25: Images should have alternate text (alt text)
null(MD045, no-alt-text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
images/hives.jpgis excluded by!**/*.jpg
📒 Files selected for processing (7)
.gitignore(1 hunks)package.json(1 hunks)pio-scripts/output_bins.py(1 hunks)platformio.ini(4 hunks)readme.md(1 hunks)wled00/FX.h(7 hunks)wled00/FX_fcn.cpp(1 hunks)
🧰 Additional context used
🪛 LanguageTool
readme.md
[grammar] ~14-~14: The article “a” is not needed here.
Context: ... edge length of 10 LEDs (=16.67 cm with a 60 LEDs/m strip). The basic functionality ...
(A_CD_NNS)
[uncategorized] ~22-~22: Use the indefinite article “an” before nouns that start with a vowel sound.
Context: ... chosen palette) - more to come Using a ESP32 over an ESP8266 is highly recomme...
(AI_EN_LECTOR_REPLACEMENT_DETERMINER_A_AN)
🪛 markdownlint-cli2 (0.17.2)
readme.md
25-25: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
🔇 Additional comments (15)
pio-scripts/output_bins.py (1)
38-38: Updated naming convention for release binariesThe file naming convention was changed to include "hex" in the filename, which appropriately reflects the hexagon-specific nature of this fork as described in the README changes.
wled00/FX_fcn.cpp (1)
258-258:❓ Verification inconclusive
Reduced power consumption constant for ESP
The
MA_FOR_ESPconstant was reduced from 100mA to 80mA, which affects power limiting calculations. This may result in higher permitted brightness before power limiting takes effect, since less power is reserved for the ESP.Verify that 80mA is sufficient for the ESP's current consumption needs, especially when using an ESP32 as recommended in the README for the hexagon effects. ESP32 typically has higher power requirements than ESP8266.
🏁 Script executed:
#!/bin/bash # Search for any other code referencing the ESP power consumption value rg --type cpp --no-heading "ESP.*(power|current|mA|milliamp)" -A 2 -B 2Length of output: 1813
Verify ESP Power Consumption Constant
The constant
MA_FOR_ESPwas reduced from 100mA to 80mA in the update. However, note the following:
- The macro comment indicates that while a Wemos D1 uses about 80mA, an ESP32 typically draws around 120mA. This difference is critical since the project README for the hexagon effects recommends using an ESP32.
- In the power calculations within
wled00/FX_fcn.cpp, the subtraction for the ESP’s current reserve—and the comment beside it (still stating 100mA for ESP power)—may now be inconsistent.- Adding
MA_FOR_ESPback into thecurrentMilliampsestimate could lead to miscalculations on ESP32 boards, potentially allowing a higher brightness before power limiting kicks in.Please verify that using 80mA is indeed sufficient for your target hardware. If the installation is expected to run on an ESP32 (which typically requires ~120mA), either adjust the constant accordingly or implement a mechanism (or document clearly) for selecting the proper value based on the board type.
readme.md (1)
13-21: Well-documented WLED hexagon forkThe new section clearly explains the purpose of this fork and the specialized LED effects for hexagon structures. This helps users understand the unique features of this variant.
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: The article “a” is not needed here.
Context: ... edge length of 10 LEDs (=16.67 cm with a 60 LEDs/m strip). The basic functionality ...(A_CD_NNS)
package.json (3)
16-16: Repository URL updated to forked repositoryThe repository URL has been changed from the original "Aircoookie/WLED" to "G0RIAN/WLED", which aligns with the PR title "Merge upstream". This indicates this is a fork being updated from the upstream repository.
21-21: Issues URL updated to match repository ownerThe issues URL has been updated to point to G0RIAN's repository issues page, maintaining consistency with the repository URL change.
23-23: Homepage URL updated to match repository ownerThe homepage URL has been updated to point to G0RIAN's repository readme, maintaining consistency with the other URL changes.
platformio.ini (3)
15-15: Default environments for release builds disabledThe line defining multiple default environments for release builds has been commented out. This will affect how release binaries are generated, limiting them to only the environments explicitly specified elsewhere.
Verify that this change is intentional and that you have an alternative method to build release binaries for all required platforms.
31-31: Default environment set to esp32dev onlyA single environment (esp32dev) is now set as the default, which means builds will target only ESP32 by default instead of multiple platforms.
154-156: Upload speed increased to 921600 baudYou've increased the upload speed from 115200 to 921600 baud. This significantly faster upload speed will reduce firmware upload times, but may cause issues with some USB-serial adapters or connections.
Verify that all your development environments and hardware can reliably handle this higher upload speed. If there are connectivity issues during uploads, you may need to revert to the lower speed.
wled00/FX.h (6)
123-123: Increased effect mode countMODE_COUNT has been increased from 118 to 126 to accommodate the new HIVE effects being added.
243-250: Added new HIVE effect mode definitionsEight new effect modes have been added for the HIVE feature, with sequential mode identifiers from 118 to 125. The naming convention and organization are consistent with the existing codebase.
621-628: Added function pointers for new HIVE effects in constructorThe constructor has been updated to map the new effect modes to their respective implementation functions. This ensures the new effects will be available within the system.
854-861: Added function declarations for HIVE effectsThe function declarations for the new HIVE effect implementations have been added to the class. The naming is consistent and the functions return the expected uint16_t type like other effect functions.
908-909: Added helper functions for HIVE effectsNew helper functions
HIVE_segment_swipe()anddisplay_frame()have been added, which will be used by the HIVE effect implementations. These follow the pattern of existing helper functions.
966-967: Updated JSON mode names for UI displayThe JSON_mode_names array has been updated to include the names of the new HIVE effects for display in the UI. Line 966 also includes the names for previously added effects like "Flow" and "Chunchun".
| @@ -15,3 +15,4 @@ | |||
| node_modules | |||
| .idea | |||
| .direnv | |||
| .vscode/extensions.json | |||
There was a problem hiding this comment.
Conflicting ignore settings for extensions.json
The newly added line ignores .vscode/extensions.json, but line 6 has !.vscode/extensions.json which explicitly includes this file. These directives contradict each other, causing ambiguous behavior.
Apply this diff to resolve the conflict:
-!.vscode/extensions.json
+# .vscode settings tracked except for extensions.json
.vscode/extensions.jsonAlternatively, if the intent is to include extensions.json in version control, remove line 18 instead.
| -fexceptions | ||
|
|
||
|
|
||
| build_unflags = | ||
| -fno-exceptions |
There was a problem hiding this comment.
Contradictory exception handling flags
You've added both -fexceptions to build_flags and -fno-exceptions to build_unflags. These flags contradict each other - one enables C++ exceptions and the other disables them.
This configuration may lead to unexpected behavior during builds. PlatformIO applies build_unflags before build_flags, so the final result would be enabling exceptions. If that's your intention, you should remove one of these flags to avoid confusion:
build_flags =
-DMQTT_MAX_PACKET_SIZE=1024
-DSECURE_CLIENT=SECURE_CLIENT_BEARSSL
-DBEARSSL_SSL_BASIC
-D CORE_DEBUG_LEVEL=0
-D NDEBUG
#build_flags for the IRremoteESP8266 library (enabled decoders have to appear here)
-D _IR_ENABLE_DEFAULT_=false
-D DECODE_HASH=true
-D DECODE_NEC=true
-D DECODE_SONY=true
-D DECODE_SAMSUNG=true
-D DECODE_LG=true
-DWLED_USE_MY_CONFIG
; -D USERMOD_SENSORSTOMQTT
-fexceptions
build_unflags =
- -fno-exceptions📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -fexceptions | |
| build_unflags = | |
| -fno-exceptions | |
| build_flags = | |
| -DMQTT_MAX_PACKET_SIZE=1024 | |
| -DSECURE_CLIENT=SECURE_CLIENT_BEARSSL | |
| -DBEARSSL_SSL_BASIC | |
| -D CORE_DEBUG_LEVEL=0 | |
| -D NDEBUG | |
| #build_flags for the IRremoteESP8266 library (enabled decoders have to appear here) | |
| -D _IR_ENABLE_DEFAULT_=false | |
| -D DECODE_HASH=true | |
| -D DECODE_NEC=true | |
| -D DECODE_SONY=true | |
| -D DECODE_SAMSUNG=true | |
| -D DECODE_LG=true | |
| -DWLED_USE_MY_CONFIG | |
| ; -D USERMOD_SENSORSTOMQTT | |
| -fexceptions | |
| build_unflags = |
|
what's this? |
|
Please see the docs regarding contributing. You need to create a PR from a specific branch, with only the appropriate changes |
Summary by CodeRabbit
New Features
Documentation
Chores