Skip to content

Conversation

@netmindz
Copy link
Member

@netmindz netmindz commented Mar 11, 2025

Run a build of each usermod if a PR contains a usermod change

Summary by CodeRabbit

  • New Features

    • Introduced automated continuous integration for user modifications, enabling dynamic detection and building of usermod directories across multiple ESP32 hardware environments.
    • Added a new configuration file to support builds for various ESP32 variants, allowing for easier customization and extensibility.
  • Improvements

    • Updated Si7021_MQTT_HA usermod to include the SPI and Adafruit BusIO libraries as dependencies.
    • Simplified installation instructions for the Si7021_MQTT_HA usermod by streamlining the setup steps.
    • Enhanced buzzer usermod by improving pin configuration flexibility.
  • Chores

    • Implemented build and caching optimizations in the CI workflow to improve efficiency for usermod development.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2025

"""

Walkthrough

A new GitHub Actions workflow and a PlatformIO override configuration file have been introduced. The workflow automates continuous integration for user modifications under the usermods/ directory, dynamically discovering usermod directories and building them across multiple ESP32 hardware environments in parallel using the specified PlatformIO configuration. Additionally, the Si7021_MQTT_HA usermod's dependencies and README installation instructions were updated. The buzzer usermod's pin definition was made conditional.

Changes

Files/Groups Change Summary
.github/workflows/usermods.yml Added a GitHub Actions workflow for CI of usermods, including dynamic discovery and matrix builds for multiple ESP32 environments.
usermods/platformio_override.usermods.ini Added a PlatformIO override configuration defining build environments for ESP32, ESP32-C3, ESP32-S2, and ESP32-S3 with usermod support.
usermods/Si7021_MQTT_HA/library.json Added SPI and adafruit/Adafruit BusIO libraries as new dependencies alongside existing BME280 and Adafruit Si7021 dependencies.
usermods/Si7021_MQTT_HA/readme.md Simplified installation instructions by removing explicit build flag and library dependency additions, referencing only custom_usermods.
usermods/buzzer/buzzer.cpp Made USERMOD_BUZZER_PIN definition conditional on GPIO_NUM_32 macro, removing unconditional default and commented usage instructions.

Possibly related PRs

Suggested reviewers

  • willmmiles
    """

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@netmindz netmindz force-pushed the usermod-libs-matrix branch from 5c493aa to adb9b77 Compare April 26, 2025 13:40
@netmindz
Copy link
Member Author

Looks like the CI part is doing what I want now and trying to build each usermod for each env so we can easily see if anything breaks on certain envs

https://github.com/netmindz/WLED-MM/actions/runs/15655590001

@willmmiles

@netmindz netmindz marked this pull request as ready for review June 14, 2025 20:14
@netmindz netmindz requested a review from willmmiles June 14, 2025 20:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (7)
usermods/platformio_override.usermods.ini (2)

8-8: Remove or document commented brownout flag.
The -D WLED_DISABLE_BROWNOUT_DET flag is commented out, which may confuse maintainers. Remove it if unused or add a note explaining why it's disabled.


48-49: Document the [usermods] section.
The empty [usermods] section is populated at runtime by CI. Add a short comment explaining its purpose for future maintainers.

.github/workflows/usermods.yml (5)

31-31: Fix typo in job name.
"Build Enviornments" should be "Build Environments" to prevent confusion.

Apply this diff:

-name: Build Enviornments
+name: Build Environments

8-8: Remove trailing whitespace.
Trailing spaces on this line can cause lint failures; drop any extra spaces at end-of-line.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 8-8: trailing spaces

(trailing-spaces)


71-71: Remove trailing whitespace.
The extra spaces at end of the pio run command line should be removed.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 71-71: trailing spaces

(trailing-spaces)


54-56: Include override file in cache key.
Currently the cache key hashes only platformio.ini and pio-scripts/output_bins.py. Add the new usermods/platformio_override.usermods.ini to ensure cache invalidation when override definitions change.

Apply this diff:

-key: pio-${{ runner.os }}-${{ matrix.environment }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py') }}-${{ hashFiles('wled00/**', 'usermods/**') }}
+key: pio-${{ runner.os }}-${{ matrix.environment }}-${{ hashFiles('platformio.ini', 'pio-scripts/output_bins.py', 'usermods/platformio_override.usermods.ini') }}-${{ hashFiles('wled00/**', 'usermods/**') }}

66-67: Trim whitespace in override file injection.
The echo "custom_usermods = ..." line contains two spaces after echo, resulting in leading whitespace. Use a single space to maintain consistent formatting.

Apply this diff:

-        echo  "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini
+        echo "custom_usermods = ${{ matrix.usermod }}" >> platformio_override.ini
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a50b4f5 and b0dd969.

📒 Files selected for processing (2)
  • .github/workflows/usermods.yml (1 hunks)
  • usermods/platformio_override.usermods.ini (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/usermods.yml

[error] 8-8: trailing spaces

(trailing-spaces)


[warning] 15-15: wrong indentation: expected 6 but found 4

(indentation)


[warning] 40-40: wrong indentation: expected 6 but found 4

(indentation)


[warning] 59-59: wrong indentation: expected 8 but found 10

(indentation)


[error] 71-71: trailing spaces

(trailing-spaces)

🔇 Additional comments (2)
usermods/platformio_override.usermods.ini (2)

1-2: Default environments configured correctly.
The default_envs list includes all four target variants and aligns with the CI matrix.


36-37: Verify base environment for ESP32-S3 builds.
The S3 variant extends esp32s3, while other variants use *dev suffixes (e.g., esp32c3dev). Ensure esp32s3 exists in the primary platformio.ini or adjust to esp32s3dev.

Comment on lines +15 to +16
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix YAML indentation under get_usermod_envs steps.
Sequence items (- uses:) must be indented two spaces under steps: to be valid.

Apply this diff:

-    - uses: actions/checkout@v4
-    - uses: actions/setup-python@v5
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
📝 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.

Suggested change
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 15-15: wrong indentation: expected 6 but found 4

(indentation)

🤖 Prompt for AI Agents
In .github/workflows/usermods.yml around lines 15 to 16, the sequence items
under the steps key are not indented correctly. Fix the YAML indentation by
ensuring that each `- uses:` line is indented exactly two spaces under the
`steps:` key to comply with YAML syntax requirements.

Comment on lines +40 to +41
- uses: actions/checkout@v4
- name: Set up Node.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix YAML indentation under build steps.
The - uses: entries here should align with a two-space indent under steps:.

Apply this diff:

-    - uses: actions/checkout@v4
-    - name: Set up Node.js
+      - uses: actions/checkout@v4
+      - name: Set up Node.js
📝 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.

Suggested change
- uses: actions/checkout@v4
- name: Set up Node.js
- uses: actions/checkout@v4
- name: Set up Node.js
🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 40-40: wrong indentation: expected 6 but found 4

(indentation)

🤖 Prompt for AI Agents
In .github/workflows/usermods.yml at lines 40 to 41, the YAML indentation for
the build steps is incorrect. Adjust the indentation so that the `- uses:` and
`- name:` entries are indented exactly two spaces under the `steps:` key to
comply with YAML syntax and ensure proper workflow execution.

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.12'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct indentation for Python setup block.
Indent python-version and cache two spaces under with: (8 spaces total).

Apply this diff:

-          python-version: '3.12'
-          cache: 'pip'
+        python-version: '3.12'
+        cache: 'pip'
📝 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.

Suggested change
python-version: '3.12'
python-version: '3.12'
cache: 'pip'
🧰 Tools
🪛 YAMLlint (1.37.1)

[warning] 59-59: wrong indentation: expected 8 but found 10

(indentation)

🤖 Prompt for AI Agents
In .github/workflows/usermods.yml at line 59, the `python-version` key under the
`with:` block is not correctly indented. Adjust the indentation of
`python-version` and the `cache` keys to be two spaces further indented under
`with:`, making a total of 8 spaces, to comply with YAML formatting standards.

@willmmiles
Copy link
Member

Looks like the CI part is doing what I want now and trying to build each usermod for each env so we can easily see if anything breaks on certain envs

https://github.com/netmindz/WLED-MM/actions/runs/15655590001

@willmmiles

OK, if I'm understanding correctly, the goal is to do full-usermod builds on all platforms, but only if the usermods themselves change; while the base ESP32 platform still does an all-usermod build for core firmware changes.

From a code standpoint, I think you can clean out a lot of the environment text by picking envs/boards we're already building for. Something like:

[env:usermods_esp32s3]
extends = esp32s3dev_16MB_opi
board_build.partitions = ${esp32.big_partitions}
custom_usermods = *

... is probably sufficient.

From a resource utilization standpoint, we can get a lot of savings if we can do the usermods builds in the same session as the reference build, thanks to the build cache. That's one of the big advantages of the new usermod system -- no extra -Ds in the build_flags means the build cache is fully compatible, since all the assembly happens at link time. So. if possible, building esp32s3dev_16MB_opi and usermods_esp32s3dev_16MB_opi in the same session should save half the build time.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@netmindz
Copy link
Member Author

Looks like the CI part is doing what I want now and trying to build each usermod for each env so we can easily see if anything breaks on certain envs
https://github.com/netmindz/WLED-MM/actions/runs/15655590001
@willmmiles

OK, if I'm understanding correctly, the goal is to do full-usermod builds on all platforms, but only if the usermods themselves change; while the base ESP32 platform still does an all-usermod build for core firmware changes.

Correct

From a code standpoint, I think you can clean out a lot of the environment text by picking envs/boards we're already building for. Something like:

[env:usermods_esp32s3]
extends = esp32s3dev_16MB_opi
board_build.partitions = ${esp32.big_partitions}
custom_usermods = *

... is probably sufficient.

Incorrect, you also need board, platform and board_build.flash_mode as a minimum. build_flags needed for the release name, custom_usermods needed to limit to only the single usermod so it's really only build_unflags and lib_deps that you can save

From a resource utilization standpoint, we can get a lot of savings if we can do the usermods builds in the same session as the reference build, thanks to the build cache. That's one of the big advantages of the new usermod system -- no extra -Ds in the build_flags means the build cache is fully compatible, since all the assembly happens at link time. So. if possible, building esp32s3dev_16MB_opi and usermods_esp32s3dev_16MB_opi in the same session should save half the build time.

If you look at the build output you will see that it's already making extensive use of the build cache so doesn't need to be in the same session for that. While we could do a single session that had all the envs rather than using a matrix CI build, saving things like checking out the code and the cache, we would lose the clear visibility of what usermods are failing

@willmmiles
Copy link
Member

OK, if I'm understanding correctly, the goal is to do full-usermod builds on all platforms, but only if the usermods themselves change; while the base ESP32 platform still does an all-usermod build for core firmware changes.

Correct

How do you intend to handle inter-module dependencies? Not all modules can be built in isolation. PWM_fan is a good example - it depends on certain other usermods being included to build successfully. (Worse, it has an 'either a or b' type dependency, which can't be represented in pure PlatformIO lib_deps - it has to be analyzed via some custom script logic. :( )

From a code standpoint, I think you can clean out a lot of the environment text by picking envs/boards we're already building for. Something like:

[env:usermods_esp32s3]
extends = esp32s3dev_16MB_opi
board_build.partitions = ${esp32.big_partitions}
custom_usermods = *

... is probably sufficient.

Incorrect, you also need board, platform and board_build.flash_mode as a minimum. build_flags needed for the release name, custom_usermods needed to limit to only the single usermod so it's really only build_unflags and lib_deps that you can save

Those should be supplied by the environment being extended (in this example esp32s3dev_16MB_opi). I'm suggesting using extends over our fully specified target environments that include a board, eg. esp32s3dev_16MB_opi, instead of the "common code for all S3 boards" fragments. Since we're building for those boards anyways, should the env requirements for all boards change again (like WLED_RELEASE quoting), we won't have to maintain a second set of the extra stuff we need to go from common code to a full build.

If you look at the build output you will see that it's already making extensive use of the build cache so doesn't need to be in the same session for that. While we could do a single session that had all the envs rather than using a matrix CI build, saving things like checking out the code and the cache, we would lose the clear visibility of what usermods are failing

Ah, it's already sharing a build cache between jobs! That's good enough for me. (I should look at getting the HTML outputs in the cache, that'll save even more replication.)

@netmindz
Copy link
Member Author

How do you intend to handle inter-module dependencies? Not all modules can be built in isolation. PWM_fan is a good example - it depends on certain other usermods being included to build successfully. (Worse, it has an 'either a or b' type dependency, which can't be represented in pure PlatformIO lib_deps - it has to be analyzed via some custom script logic. :( )

I don't at the moment is the simple answer. We would need to add something to these modules to define their dependencies and then process this when the CI generates the usermod environment.

In the short term I think we just blacklist any of these usermods from the CI to avoid false positives

From a code standpoint, I think you can clean out a lot of the environment text by picking envs/boards we're already building for. Something like:

[env:usermods_esp32s3]
extends = esp32s3dev_16MB_opi
board_build.partitions = ${esp32.big_partitions}
custom_usermods = *

... is probably sufficient.

Incorrect, you also need board, platform and board_build.flash_mode as a minimum. build_flags needed for the release name, custom_usermods needed to limit to only the single usermod so it's really only build_unflags and lib_deps that you can save

Those should be supplied by the environment being extended (in this example esp32s3dev_16MB_opi). I'm suggesting using extends over our fully specified target environments that include a board, eg. esp32s3dev_16MB_opi, instead of the "common code for all S3 boards" fragments. Since we're building for those boards anyways, should the env requirements for all boards change again (like WLED_RELEASE quoting), we won't have to maintain a second set of the extra stuff we need to go from common code to a full build.

What you might expect the behaviour of extends and what actually happens in platformio are very different things. This is why I said how you still need to define platform and board in every env, just try your proposed example and you will see what I mean

@willmmiles
Copy link
Member

What you might expect the behaviour of extends and what actually happens in platformio are very different things. This is why I said how you still need to define platform and board in every env, just try your proposed example and you will see what I mean

Sorry, I should be more careful when sketching examples -- 'env:' is required in the extends to reference the correct section. The correct code should be:

[env:usermods_esp32s3]
extends = env:esp32s3dev_16MB_opi
custom_usermods = ${usermods.custom_usermods}

Suggested code is at willmmiles@68540c6 ; you can see that it works at https://github.com/willmmiles/WLED/actions/runs/15688280452

@willmmiles
Copy link
Member

How do you intend to handle inter-module dependencies? Not all modules can be built in isolation. PWM_fan is a good example - it depends on certain other usermods being included to build successfully. (Worse, it has an 'either a or b' type dependency, which can't be represented in pure PlatformIO lib_deps - it has to be analyzed via some custom script logic. :( )

I don't at the moment is the simple answer. We would need to add something to these modules to define their dependencies and then process this when the CI generates the usermod environment.

In the short term I think we just blacklist any of these usermods from the CI to avoid false positives

This is ultimately why I went with the "build all the modules in one env" CI approach; I found that the number of modules that couldn't be combined in one build was smaller than the number of modules that had build time dependencies.

I agree that a blacklist is probably the best workaround. I don't think we want to invest in a more complex solution - longer term I am hoping to move towards out-of-tree usermods, so making sure the dep list is correct can be left as an exercise for the individual module's own CI. (That said, I'll need to make sure the example repo demonstrates a working CI -- something that runs nightly and checks for WLED updates, perhaps...)

On an quasi-related note, another objective with env:usermods was to keep any extra build requirements (necessary defines, etc.) in one place. I did try and migrate such requirements to defaults in the modules themselves whenever possible, but there's still one left (-DTOUCH_CS=9). We should see if we can make it go away, or consider setting up another PlatformIO variable to store it in so it can be used by both kinds of usermod test build.

@netmindz
Copy link
Member Author

What you might expect the behaviour of extends and what actually happens in platformio are very different things. This is why I said how you still need to define platform and board in every env, just try your proposed example and you will see what I mean

Sorry, I should be more careful when sketching examples -- 'env:' is required in the extends to reference the correct section. The correct code should be:

[env:usermods_esp32s3]
extends = env:esp32s3dev_16MB_opi
custom_usermods = ${usermods.custom_usermods}

Suggested code is at willmmiles@68540c6 ; you can see that it works at https://github.com/willmmiles/WLED/actions/runs/15688280452

Thanks, that is applied now

@netmindz
Copy link
Member Author

How do you intend to handle inter-module dependencies? Not all modules can be built in isolation. PWM_fan is a good example - it depends on certain other usermods being included to build successfully. (Worse, it has an 'either a or b' type dependency, which can't be represented in pure PlatformIO lib_deps - it has to be analyzed via some custom script logic. :( )

...

I agree that a blacklist is probably the best workaround. I don't think we want to invest in a more complex solution - longer term I am hoping to move towards out-of-tree usermods, so making sure the dep list is correct can be left as an exercise for the individual module's own CI. (That said, I'll need to make sure the example repo demonstrates a working CI -- something that runs nightly and checks for WLED updates, perhaps...)

Actually, there is perhaps a simpler option - given you have proved that you can build the usermods when both A and B dependencies are included, given the new usermod system is so much more user friendly, why not just include A and B as dependencies in library.json? Ok that technically that will make the bin slightly larger than a totally slimmed down version, but it solves out build problem and makes it easier for users too as they just depend on PWM fan for example and don't then need to work out which dependencies they need to also add to build

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e97723d and 3809996.

📒 Files selected for processing (1)
  • usermods/PWM_fan/library.json (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
usermods/PWM_fan/library.json

[error] 7-7: expected , but instead found "dependencies"

Remove "dependencies"

(parse)


[error] 8-8: expected : but instead found ,

Remove ,

(parse)


[error] 8-8: expected : but instead found ,

Remove ,

(parse)


[error] 9-9: expected : but instead found ,

Remove ,

(parse)


[error] 10-10: expected : but instead found }

Remove }

(parse)


[error] 8-8: The key was already declared.

This where a duplicated key was declared again.

If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.

(lint/suspicious/noDuplicateObjectKeys)

⏰ Context from checks skipped due to timeout of 90000ms (20)
  • GitHub Check: wled_build / Build Environments (esp32_wrover)
  • GitHub Check: wled_build / Build Environments (esp32dev)
  • GitHub Check: wled_build / Build Environments (esp32_eth)
  • GitHub Check: wled_build / Build Environments (esp32c3dev)
  • GitHub Check: wled_build / Build Environments (lolin_s2_mini)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
  • GitHub Check: wled_build / Build Environments (usermods)
  • GitHub Check: wled_build / Build Environments (esp32dev_V4)
  • GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
  • GitHub Check: wled_build / Build Environments (nodemcuv2)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
  • GitHub Check: wled_build / Build Environments (nodemcuv2_160)
  • GitHub Check: wled_build / Build Environments (esp01_1m_full)
  • GitHub Check: wled_build / Build Environments (esp8266_2m)
  • GitHub Check: wled_build / Build Environments (esp8266_2m_160)
  • GitHub Check: wled_build / Test cdata.js

@willmmiles
Copy link
Member

Actually, there is perhaps a simpler option - given you have proved that you can build the usermods when both A and B dependencies are included, given the new usermod system is so much more user friendly, why not just include A and B as dependencies in library.json?

For PWM_fan, If both deps are included, the internal ifdefs bind to Temperature and ignore sht. The module would have to be recoded to allow runtime selection. If the library.json lists both as deps, sht becomes unusable for production builds.

@netmindz netmindz force-pushed the usermod-libs-matrix branch from dce4eb6 to e97723d Compare June 17, 2025 18:28
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
.github/workflows/usermods.yml (3)

15-20: Invalid YAML – step list items need 2 more spaces

The sequence items under steps: are still aligned at the same level as the key itself.
They must be indented 6 spaces (2 more than the current 4) to be valid YAML.

-    - uses: actions/checkout@v4
-    - uses: actions/setup-python@v5
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5

40-46: Repeat of the steps-indentation error in the build job

Same fix required here; otherwise the matrix job is never parsed.

-    - uses: actions/checkout@v4
-    - name: Set up Node.js
+      - uses: actions/checkout@v4
+      - name: Set up Node.js

57-60: with: block keys over-indented

Keys under with: must be exactly 2 spaces further in than with: itself (total 8).

-          python-version: '3.12'
-          cache: 'pip'
+        python-version: '3.12'
+        cache: 'pip'
🧹 Nitpick comments (2)
.github/workflows/usermods.yml (2)

31-33: Spelling: “Enviornments” → “Environments”

Minor, but visible in the Actions UI.


8-8: Remove trailing whitespace (lines 8 & 71)

Keeps YAMLlint happy and eliminates noisy diffs.

Also applies to: 71-71

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dce4eb6 and 1c4141a.

📒 Files selected for processing (1)
  • .github/workflows/usermods.yml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
🪛 YAMLlint (1.37.1)
.github/workflows/usermods.yml

[error] 8-8: trailing spaces

(trailing-spaces)


[warning] 15-15: wrong indentation: expected 6 but found 4

(indentation)


[warning] 40-40: wrong indentation: expected 6 but found 4

(indentation)


[warning] 59-59: wrong indentation: expected 8 but found 10

(indentation)


[error] 71-71: trailing spaces

(trailing-spaces)

🔇 Additional comments (1)
.github/workflows/usermods.yml (1)

50-55: Block-scalar contents indented too far

Lines 51-53 are indented 14 spaces, so YAML treats them as part of the previous indentation level instead of scalar content. Bring them back to 8 spaces (2 under |).

-              ~/.platformio/.cache
-              ~/.buildcache
-              build_output
+        ~/.platformio/.cache
+        ~/.buildcache
+        build_output

Likely an incorrect or invalid review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
.github/workflows/usermods.yml (3)

15-16: Fix YAML indentation under get_usermod_envs steps.

Sequence items (- uses:) must be indented two spaces under steps: to be valid YAML.

-    - uses: actions/checkout@v4
-    - uses: actions/setup-python@v5
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5

40-41: Fix YAML indentation under build steps.

The - uses: entries should be indented two spaces under steps:.

-    - uses: actions/checkout@v4
-    - name: Set up Node.js
+      - uses: actions/checkout@v4
+      - name: Set up Node.js

59-60: Fix indentation for Python setup parameters.

The python-version and cache keys should be indented two spaces under with:.

-          python-version: '3.12'
-          cache: 'pip'
+        python-version: '3.12'
+        cache: 'pip'
🧹 Nitpick comments (3)
.github/workflows/usermods.yml (3)

8-8: Remove trailing spaces.

-    
+

31-31: Fix typo in job name.

-    name: Build Enviornments
+    name: Build Environments

71-71: Remove trailing spaces.

-      run: pio run -e ${{ matrix.environment }}    
+      run: pio run -e ${{ matrix.environment }}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b187f94 and b1ed99d.

📒 Files selected for processing (2)
  • .github/workflows/usermods.yml (1 hunks)
  • usermods/buzzer/buzzer.cpp (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/buzzer/buzzer.cpp (5)
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
🪛 YAMLlint (1.37.1)
.github/workflows/usermods.yml

[error] 8-8: trailing spaces

(trailing-spaces)


[warning] 15-15: wrong indentation: expected 6 but found 4

(indentation)


[warning] 40-40: wrong indentation: expected 6 but found 4

(indentation)


[warning] 59-59: wrong indentation: expected 8 but found 10

(indentation)


[error] 71-71: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (4)
usermods/buzzer/buzzer.cpp (1)

8-12: LGTM! Improved platform compatibility.

The conditional pin definition enhances portability across ESP32 variants by using GPIO_NUM_32 when available and falling back to pin 21 otherwise. This is a good improvement that maintains backward compatibility while supporting newer platforms.

.github/workflows/usermods.yml (3)

22-27: Excellent usermod discovery and filtering logic.

The dynamic discovery of usermods using find and jq with filtering for problematic modules (PWM_fan, BME68X_v2) is well-implemented and aligns with the PR objectives to handle inter-module dependencies.


34-38: Well-designed matrix build strategy.

The matrix combination of usermods and environments (usermods_esp32, usermods_esp32c3, usermods_esp32s2, usermods_esp32s3) effectively tests each usermod across all supported ESP32 variants, ensuring comprehensive compatibility validation.


47-55: Comprehensive caching strategy for build optimization.

The caching setup covers PlatformIO cache, build cache, and output directory with appropriate cache keys based on relevant file hashes. This should significantly improve build times as mentioned in the PR objectives.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
.github/workflows/usermods.yml (3)

14-21: Fix broken indentation under steps – the workflow will not parse
Items inside steps: must be indented two spaces further than the key itself. The current 0-indent list entries (- uses: / - name:) violate YAML syntax and will cause the job to be skipped.

-    steps:
-    - uses: actions/checkout@v4
-    - uses: actions/setup-python@v5
-      with:
-        python-version: '3.12'
-        cache: 'pip'
-    - name: Install PlatformIO
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: '3.12'
+          cache: 'pip'
+      - name: Install PlatformIO

40-46: Repeat the indentation fix for the build job’s steps
Same problem as above: list items must sit two spaces below their parent key.

-    steps:
-    - uses: actions/checkout@v4
-    - name: Set up Node.js
+    steps:
+      - uses: actions/checkout@v4
+      - name: Set up Node.js

Make sure every following step in this block keeps that 6-space indent.


56-60: Indent the with: block correctly
python-version and cache should be indented two spaces under with: (total 8-space indent). Current 10-space indent violates YAML spec.

-       with:
-           python-version: '3.12'
-           cache: 'pip'
+       with:
+         python-version: '3.12'
+         cache: 'pip'
🧹 Nitpick comments (1)
.github/workflows/usermods.yml (1)

31-31: Typo: “Enviornments” → “Environments”
This appears in the job name and will surface in CI logs.

-    name: Build Enviornments
+    name: Build Environments
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db55fec and e227d01.

📒 Files selected for processing (1)
  • .github/workflows/usermods.yml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
🪛 YAMLlint (1.37.1)
.github/workflows/usermods.yml

[error] 8-8: trailing spaces

(trailing-spaces)


[warning] 15-15: wrong indentation: expected 6 but found 4

(indentation)


[warning] 40-40: wrong indentation: expected 6 but found 4

(indentation)


[warning] 59-59: wrong indentation: expected 8 but found 10

(indentation)


[error] 71-71: trailing spaces

(trailing-spaces)

paths:
- usermods/**
- .github/workflows/usermods.yml

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove trailing whitespace – YAML-lint treats it as an error

Also applies to: 71-71

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 8-8: trailing spaces

(trailing-spaces)

🤖 Prompt for AI Agents
In .github/workflows/usermods.yml at lines 8 and 71, there are trailing
whitespace characters that cause YAML-lint errors. Remove any spaces or tabs at
the end of these lines to comply with YAML formatting rules and prevent linting
issues.

@netmindz netmindz merged commit 7b0075d into wled:main Jun 28, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants