Skip to content

feat(led_indicator): Refactor the components into the Factory Pattern #532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

zhongchaoesp
Copy link

@zhongchaoesp zhongchaoesp commented Jun 17, 2025

Description

This PR refactors the led_indicator component into a factory pattern, reducing the coupling of API functions.

Original Implementation (Before Refactoring)

Original Configuration Structure:

typedef struct {
    led_indicator_mode_t mode;                  /*!< LED work mode, e.g., GPIO or PWM mode */
    union {
        led_indicator_gpio_config_t *led_indicator_gpio_config;       /*!< LED GPIO configuration */
        led_indicator_ledc_config_t *led_indicator_ledc_config;       /*!< LED LEDC configuration */
        led_indicator_rgb_config_t *led_indicator_rgb_config;         /*!< LED RGB configuration */
        led_indicator_strips_config_t *led_indicator_strips_config;   /*!< LED LEDC RGB strips configuration */
        led_indicator_custom_config_t *led_indicator_custom_config;   /*!< LED custom configuration */
    }; /**< LED configuration */
    blink_step_t const **blink_lists;           /*!< User-defined LED blink lists */
    uint16_t blink_list_num;                    /*!< Number of blink lists */
} led_indicator_config_t;

Original Creation API:

led_indicator_handle_t led_indicator_create(const led_indicator_config_t *config);

Major Changes (After Refactoring)

  1. Struct Splitting:
  • Generic Configuration (common to all LED types):
    typedef struct {
        blink_step_t const **blink_lists;       /*!< User-defined blink patterns */
        uint16_t blink_list_num;                /*!< Number of blink lists */
    } led_indicator_config_t;
  • Device-Specific Configurations (e.g., GPIO LED):
    typedef struct {
       bool is_active_level_high;     /*!< Set true if GPIO level is high when ON */
       int32_t gpio_num;              /*!< GPIO pin number */
    } led_indicator_gpio_config_t;
    (Similar structs created for LEDC, RGB, Strips)
  1. API Refactoring:
// Factory functions for different LED types
esp_err_t led_indicator_new_gpio_device(const led_indicator_config_t *led_config, const led_indicator_gpio_config_t *gpio_cfg, led_indicator_handle_t *handle);
esp_err_t led_indicator_new_ledc_device(const led_indicator_config_t *led_config, const led_indicator_ledc_config_t *ledc_cfg, led_indicator_handle_t *handle);
esp_err_t led_indicator_new_rgb_device(const led_indicator_config_t *led_config, const led_indicator_rgb_config_t *rgb_cfg, led_indicator_handle_t *handle);
esp_err_t led_indicator_new_strips_device(const led_indicator_config_t *led_config, const led_indicator_strips_config_t *strips_cfg, led_indicator_handle_t *handle);

Testing

Before:

led_indicator_gpio_config_t led_indicator_gpio_config = {
    .gpio_num = LED_IO_NUM_0,              /**< num of GPIO */
    .is_active_level_high = 1,
};

led_indicator_config_t config = {
    .led_indicator_gpio_config = &led_indicator_gpio_config,
    .mode = LED_GPIO_MODE,
    .blink_lists = led_blink_lst,
    .blink_list_num = BLINK_NUM,
};
led_handle_0 = led_indicator_create(&config);

After:

led_indicator_gpio_config_t led_indicator_gpio_config = {
    .gpio_num = LED_IO_NUM_0,              /**< num of GPIO */
    .is_active_level_high = 1,
};

led_indicator_config_t config = {
    .blink_lists = NULL,
    .blink_list_num = 0,
};

esp_err_t ret = led_indicator_new_gpio_device(&config, &led_indicator_gpio_config, &led_handle_0);

Checklist

  • GPIO type LED testing
  • LEDC type LED testing
  • RGB type LED testing
  • Strips type LED testing

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

github-actions bot commented Jun 17, 2025

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "feat(led_indicator): Fixed a few bugs":
    • summary looks too short

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 12 commits (simplifying branch history).
Messages
📖 This PR seems to be quite large (total lines of code: 1875), you might consider splitting it into smaller PRs

👋 Hello zhongchaoesp, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 38499d5

@github-actions github-actions bot changed the title switch_case_to_factory switch_case_to_factory (AEGHB-1115) Jun 17, 2025
@zhongchaoesp zhongchaoesp changed the title switch_case_to_factory (AEGHB-1115) feat(led_indicator): Refactor the components into the Factory Pattern Jun 18, 2025
@leeebo leeebo requested review from Copilot and leeebo June 18, 2025 07:51
Copilot

This comment was marked as outdated.

@leeebo leeebo requested a review from Copilot June 19, 2025 01:36
Copilot

This comment was marked as outdated.

@leeebo leeebo requested a review from Copilot July 9, 2025 02:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the led_indicator component into a Factory Pattern, introducing per-device creation APIs and removing the legacy mode-based led_indicator_create function.

  • Added new factory functions (led_indicator_new_*_device) for GPIO, LEDC, RGB, and Strips.
  • Updated all examples, tests, and documentation to use the new APIs and include per-device headers.
  • Cleaned up source and header layout, removed legacy files, and conditioned blink defaults on CONFIG_USE_MI_BLINK_DEFAULT.

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.

Show a summary per file
File Description
examples/usb/host/usb_cdc_4g_module/main/app_main.c Switched to led_indicator_new_gpio_device and include gpio header
examples/indicator/ws2812_strips/main/main.c Switched to led_indicator_new_strips_device and include strips header
examples/indicator/rgb/main/main.c Switched to led_indicator_new_rgb_device and include rgb header
examples/indicator/ledc/main/main.c Switched to led_indicator_new_ledc_device and include ledc header
examples/indicator/gpio/main/main.c Switched to led_indicator_new_gpio_device and include gpio header
docs/zh_CN/display/led_indicator.rst Updated Chinese docs for factory API; fixed syntax typos
docs/en/display/led_indicator.rst Updated English docs for factory API; fixed syntax typos
components/led/led_indicator/test_apps/main/test_led_indicator.c Updated tests to use factory APIs and added deinit guard
components/led/led_indicator/src/mi_led_indicator_blink_default.c Added MI-specific default blink lists under CONFIG_USE_MI_BLINK_DEFAULT
components/led/led_indicator/src/led_indicator_strips.c Refactored strips driver; added led_indicator_new_strips_device
components/led/led_indicator/src/led_indicator_rgb.c Refactored RGB driver; added led_indicator_new_rgb_device
components/led/led_indicator/src/led_indicator_ledc.c Refactored LEDC driver; added led_indicator_new_ledc_device
components/led/led_indicator/src/led_indicator_gpio.c Added GPIO driver; added led_indicator_new_gpio_device
components/led/led_indicator/src/led_indicator.c Removed legacy led_indicator_create, updated internal API calls
components/led/led_indicator/src/led_gpio.c Removed legacy GPIO implementation
components/led/led_indicator/include/led_types.h Introduced shared types and moved enums/structs
components/led/led_indicator/include/led_indicator_strips.h Added factory API header for strips
components/led/led_indicator/include/led_indicator_rgb.h Added factory API header for RGB
components/led/led_indicator/include/led_indicator_ledc.h Added factory API header for LEDC
components/led/led_indicator/include/led_indicator_gpio.h Added factory API header for GPIO
components/led/led_indicator/include/led_indicator_blink_default.h Split blink-default enum under CONFIG_USE_MI_BLINK_DEFAULT
components/led/led_indicator/include/led_indicator.h Updated core header; removed mode-based create
components/led/led_indicator/include/led_gpio.h Removed legacy header
components/led/led_indicator/include/led_rgb.h Removed legacy header
components/led/led_indicator/include/led_ledc.h Removed legacy header
components/led/led_indicator/include/led_strips.h Removed legacy header
components/led/led_indicator/include/led_custom.h Removed legacy header
components/led/led_indicator/include/led_convert.h Updated copyright year
components/led/led_indicator/Kconfig Added CONFIG_USE_MI_BLINK_DEFAULT option
components/led/led_indicator/CMakeLists.txt Updated source list for new implementations and conditional blink default
Comments suppressed due to low confidence (4)

docs/zh_CN/display/led_indicator.rst:293

  • Remove the extra comma to fix the syntax: .blink_lists = led_blink_lst,.
        .blink_lists = led_blink_lst,,

docs/zh_CN/display/led_indicator.rst:294

  • Remove the extra comma to fix the syntax: .blink_list_num = BLINK_MAX,.
        .blink_list_num = BLINK_MAX,,

docs/en/display/led_indicator.rst:296

  • Remove the extra comma to fix the syntax: .blink_lists = led_blink_lst,.
        .blink_lists = led_blink_lst,,

docs/en/display/led_indicator.rst:297

  • Remove the extra comma to fix the syntax: .blink_list_num = BLINK_MAX,.
        .blink_list_num = BLINK_MAX,,

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.

3 participants