Skip to content
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

component(bsp_devkit): change led_indicator version to ^1, , button to ^4 (BSP-646) #526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

InfiniteYuan
Copy link

@InfiniteYuan InfiniteYuan commented Feb 28, 2025

ESP-BSP Pull Request checklist

Note: For new BSPs create a PR with this link.

  • Version of modified component bumped
  • CI passing

Change description

  • component(bsp_devkit): change led_indicator version to ^1, , button to ^4
  • esp-matter want to use cmake_utilities 1.*, but bsp_devkit depends on low version

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title component(bsp_devkit): change led_indicator version to ^1 component(bsp_devkit): change led_indicator version to ^1 (BSP-646) Feb 28, 2025
@InfiniteYuan InfiniteYuan force-pushed the feat/change_led_indicator_version branch from a2d2434 to 2d5d736 Compare February 28, 2025 03:04
@InfiniteYuan InfiniteYuan changed the title component(bsp_devkit): change led_indicator version to ^1 (BSP-646) component(bsp_devkit): change led_indicator version to ^1, , button to ^4 (BSP-646) Feb 28, 2025
@espzav
Copy link
Collaborator

espzav commented Feb 28, 2025

This PR is duplicated to #523 but you cannot only change the version of button component. The BSP must be changed a lot.

@InfiniteYuan InfiniteYuan force-pushed the feat/change_led_indicator_version branch 4 times, most recently from bf8c7ad to 73db02a Compare March 4, 2025 07:47
Copy link
Collaborator

@espzav espzav left a comment

Choose a reason for hiding this comment

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

@InfiniteYuan Great job! Thank you for these changes! Left some comments.

#define BUTTON_TYPE_ADC(n) CONFIG_BSP_BUTTON_##n##_TYPE_ADC
#define BUTTON_TYPE_GPIO(n) CONFIG_BSP_BUTTON_##n##_TYPE_GPIO

#define BSP_IOT_BUTTON_CREATE_INTERNAL(i) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to make it as runtime function?
static esp_err_t bsp_iot_button_create_internal(int i){ ... }

Copy link
Author

Choose a reason for hiding this comment

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

Can't,

Because in BSP_IOT_BUTTON_CREATE_INTERNAL macro definition, have another macro definition bsp_btn_cfg(n)

btn_array[i] = iot_button_create(&bsp_button_config[i]);
if (btn_array[i] == NULL) {
ret = ESP_FAIL;
switch (i) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you simplify it? (remove switch) and make somethink like that:

if (CONFIG_BSP_BUTTONS_NUM > i) {
ret |= bsp_iot_button_create_internal(i+1);
}

Copy link
Author

Choose a reason for hiding this comment

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

Also can't do it, Because Macro definitions need to be determined at compile time.

@InfiniteYuan InfiniteYuan force-pushed the feat/change_led_indicator_version branch from 73db02a to 6e5c753 Compare March 4, 2025 09:01
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