New versioning #39
New versioning #39
Conversation
Using YAML anchors instead. It makes it cleaner and easier to understand.
Clean up LED files & new versioning
|
Warning Rate limit exceeded@edwardtfn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces comprehensive changes across multiple files in the ESPHome project, focusing on workflow improvements, versioning, and LED configuration. The modifications include updating GitHub Actions workflows for validation and versioning, restructuring LED configuration files, and implementing an automated versioning system. The changes aim to enhance project management, streamline configuration, and provide more flexible light control mechanisms. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Workflow as GitHub Actions
participant Repo as Repository
Dev->>Repo: Push changes to main branch
Workflow->>Repo: Trigger versioning workflow
Workflow->>Workflow: Read current version
Workflow->>Workflow: Calculate new version
Workflow->>Repo: Update VERSION files
Workflow->>Repo: Commit and tag new version
Poem
🪧 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: 6
🔭 Outside diff range comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml (2)
Line range hint
784-853: Enhance error handling and logging in light state management.The light state management script could benefit from improved error handling and more appropriate log levels.
Consider these improvements:
- Add validation for brightness values
- Use appropriate log levels for different scenarios
- Add error recovery mechanism
switch (light_group) { case 1: if (light_index >= id(gb_lights_1).size()) { - ESP_LOGE("core_hw_leds", "Invalid set with %s light for Relay %" PRIu8, + ESP_LOGW("core_hw_leds", "Skipping invalid %s light for Relay %" PRIu8, light_mode_group_1.c_str(), light_index); return; } if (state and !id(gb_lights_1)[light_index]->current_values.is_on()) { - ESP_LOGI("core_hw_leds", "Turn-on %s light for Relay %" PRIu8, + ESP_LOGD("core_hw_leds", "Turn-on %s light for Relay %" PRIu8, light_mode_group_1.c_str(), light_index); auto call = id(gb_lights_1)[light_index]->turn_on(); if (LIGHT_TRANSITION_TURN_ON > 0) call.set_transition_length(LIGHT_TRANSITION_TURN_ON); // in ms - if (LIGHT_BRIGHTNESS_TURN_ON > 0 and LIGHT_BRIGHTNESS_TURN_ON <= 100) + if (LIGHT_BRIGHTNESS_TURN_ON > 0 and LIGHT_BRIGHTNESS_TURN_ON <= 100) { call.set_brightness(LIGHT_BRIGHTNESS_TURN_ON / 100.0f); + } else if (LIGHT_BRIGHTNESS_TURN_ON != 0) { + ESP_LOGW("core_hw_leds", "Invalid brightness value: %d", LIGHT_BRIGHTNESS_TURN_ON); + } call.perform(); + if (!id(gb_lights_1)[light_index]->current_values.is_on()) { + ESP_LOGW("core_hw_leds", "Failed to turn on light"); + }
Consider using esp32_rmt_led_strip platform instead of neopixelbus
Based on the codebase analysis, I found that the project already uses the
esp32_rmt_led_stripplatform in other configurations, which is a more modern and optimized approach for ESP32 devices. Instead of adding performance settings to neopixelbus, consider migrating to this platform:- id: light_full name: Light - All internal: true platform: esp32_rmt_led_strip rgb_order: GRB rmt_channel: 0 chipset: ws2812 max_refresh_rate: 33ms num_leds: 32 pin: GPIO13This matches the project's existing patterns and provides better out-of-the-box performance for ESP32 devices.
🔗 Analysis chain
Line range hint
37-47: Consider adding performance and stability settings.The neopixelbus configuration could benefit from additional settings for better performance and stability.
Consider adding these recommended settings:
- id: light_full name: Light - All internal: true platform: neopixelbus type: GRB variant: WS2811 - # method: esp32_rmt + method: esp32_rmt + # Add performance settings + chip: esp32 + channel: 0 + dma_channel: 1 num_leds: 32 pin: GPIO13Also, please verify if the esp32_rmt method is required for your hardware setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other neopixelbus configurations in the codebase rg -l 'platform:\s*neopixelbus.*method:\s*esp32_rmt' --type yamlLength of output: 66
Script:
#!/bin/bash # Let's try a broader search for neopixelbus configurations rg -A 10 'platform:\s*neopixelbus' --type yaml # Also search for any ESP32 RMT method mentions rg -i 'esp32.*rmt|rmt.*esp32' --type yamlLength of output: 1062
Script:
#!/bin/bash # Let's check the ESP32 RMT LED strip configuration for comparison rg -A 10 'platform:\s*esp32_rmt_led_strip' --type yaml # Also check if there are any other LED strip configurations that might provide insights rg -A 10 'platform:.*led.*strip' --type yamlLength of output: 1553
🧹 Nitpick comments (4)
.github/workflows/versioning.yml (1)
24-26: Use more specific git configurationThe current git configuration uses generic values.
- git config user.name "GitHub Actions" - git config user.email "[email protected]" + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com"versioning/README.md (1)
33-38: Clarify YAML include syntaxThe YAML include syntax shown in the example might be confusing.
- version: <<: !include ../versioning/VERSION + version: !include ../versioning/VERSION_YAMLAlso add a note explaining that VERSION_YAML contains the version in YAML format while VERSION contains it in plain text.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml (2)
24-26: Improve substitution value types and documentation.The substitutions use string values ('0') for numeric settings and have inconsistent comment formatting.
Apply this diff to improve the configuration:
- LIGHT_TRANSITION_TURN_ON: '0' # Transition time in msec - LIGHT_TRANSITION_TURN_OFF: '0' # Transition time in msec - LIGHT_BRIGHTNESS_TURN_ON: '0' # Brightness (1 to 100%, 0 to not set it) + # Transition time in milliseconds (0 to disable) + LIGHT_TRANSITION_TURN_ON: 0 + LIGHT_TRANSITION_TURN_OFF: 0 + # Brightness level (0 to disable, 1-100 for percentage) + LIGHT_BRIGHTNESS_TURN_ON: 0
55-92: Consider optimizing effect parameters for better visual appeal.The light effects configuration could be enhanced with additional customization options for better visual experience.
Consider these optimizations:
- addressable_rainbow: name: "Rainbow" + speed: 30 + width: 50 - pulse: name: "Pulse" transition_length: 1s update_interval: 1s + min_brightness: 0.2 - addressable_fireworks: name: "Fireworks" update_interval: 32ms spark_probability: 10% use_random_color: false fade_out_rate: 120 + spark_brightness: 0.8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/validate_esphome_beta.yml(1 hunks).github/workflows/versioning.yml(1 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml(8 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_base.yaml(0 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_effects.yaml(0 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_internal.yaml(0 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(1 hunks)README.md(1 hunks)versioning/README.md(1 hunks)versioning/bump_version.sh(1 hunks)
💤 Files with no reviewable changes (3)
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_internal.yaml
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_base.yaml
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds_light_effects.yaml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/validate_esphome_beta.yml
👮 Files not reviewed due to content moderation or server errors (1)
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/versioning.yml
21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
versioning/README.md
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...t as plain text. - bump_version.sh: A script to increment the version based...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ... and release sequence. - README.md: Documentation for the versioning proces...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~43-~43: This phrase is redundant. Consider writing “point” or “time”.
Context: ...y**: Each version is tied to a specific point in time, making it easy to track releases. 2. *...
(MOMENT_IN_TIME)
🔇 Additional comments (2)
versioning/bump_version.sh (1)
3-4:
Verify file paths are relative to repository root
The script assumes VERSION files are in ./versioning/, but this might fail if the script is executed from a different directory.
Consider using absolute paths based on script location:
-VERSION_FILE="./versioning/VERSION"
-VERSION_YAML_FILE="./versioning/VERSION_YAML"
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+VERSION_FILE="${SCRIPT_DIR}/VERSION"
+VERSION_YAML_FILE="${SCRIPT_DIR}/VERSION_YAML"README.md (1)
101-103: LGTM! Branch strategy change aligns with versioning objectives.
The change from using dev to main as the base branch simplifies the workflow and aligns with the new versioning system.
Let's verify that this branching strategy is consistently documented throughout the repository:
✅ Verification successful
Branch strategy is consistently documented across the repository
The search results confirm that all branch-related documentation consistently refers to main as the target branch. The references found in versioning/README.md and README.md are aligned with the new branching strategy, and there are no lingering references to the old dev branch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old 'dev' branch
# that might need updating
rg -i "dev branch|branch.*dev" --type md
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search for 'dev' references in markdown files
rg -i "\bdev\b" --type md
# Also search for any branch-related instructions
rg -i "branch" --type md
Length of output: 408
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores