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.

Sign up for GitHub

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

Jump to bottom

bsp: Add esp32_s3_korvo_1 (BSP-409) #239

Merged
merged 1 commit into from
Nov 27, 2023
Merged

bsp: Add esp32_s3_korvo_1 (BSP-409) #239

merged 1 commit into from
Nov 27, 2023

Conversation

pborcin
Copy link
Collaborator

@pborcin pborcin commented Oct 26, 2023 •
edited
Loading

Checklist for new Board Support package or Component

  • Component contains License
  • Component contains README.md
  • Project README.md updated
  • Component contains idf_component.yml file with url field defined
  • Component was added to CI upload job
  • New files were added to CI build job
  • New BSP definitions added to bsp_ext.py
  • BSP was added to SquareLine
  • New BSP supports IDF v4.4.x and later releases
  • Optional: Component contains unit tests
  • CI passing

Change description

Add support for esp32_s3_korvo_1

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@pborcin
Copy link
Collaborator Author

pborcin commented Oct 26, 2023

@tore-espressif @espzav PTAL

@github-actions github-actions bot changed the title bsp: Add esp32_s3_korvo_1 bsp: Add esp32_s3_korvo_1 (BSP-409) Oct 26, 2023
espzav
espzav reviewed Oct 27, 2023
View reviewed changes
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.

@pborcin Thank you very much for this PR! Good job! I left some comments.
Please, did you test it on IDF4.4?
I haven't HW, I cannot test it.

Missing things:

  • Update main readme (add board with image)
  • Add to tests
  • Add to CI for upload component

bsp/esp32_s3_korvo_1/idf_component.yml Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/README.md Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/esp32_s3_korvo_1_idf5.c Show resolved Hide resolved
bsp/esp32_s3_korvo_1/esp32_s3_korvo_1_idf4.c Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/esp32_s3_korvo_1.c Outdated Show resolved Hide resolved
tore-espressif
tore-espressif reviewed Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Only few nitpicks for now!

bsp/esp32_s3_korvo_1/README.md Outdated Show resolved Hide resolved
bsp/esp32_s3_korvo_1/README.md Show resolved Hide resolved
bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Show resolved Hide resolved
@tore-espressif
Copy link
Collaborator

@pborcin any update about this?

@pborcin pborcin force-pushed the feature/esp32_s3_korvo_1 branch from 85b7095 to 01a8614 Compare November 8, 2023 20:30
pborcin
pborcin commented Nov 9, 2023
View reviewed changes
Copy link
Collaborator Author

@pborcin pborcin left a comment

Choose a reason for hiding this comment

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

@tore-espressif @espzav PTAL
Tested with both versions 5 and 4.4

tore-espressif
tore-espressif approved these changes Nov 13, 2023
View reviewed changes
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Only one nitpick, LGTM!

I'll try on HW later this week

bsp/esp32_s3_korvo_1/README.md Outdated Show resolved Hide resolved
@pborcin pborcin force-pushed the feature/esp32_s3_korvo_1 branch 2 times, most recently from 056625b to b5b3493 Compare November 21, 2023 10:10
espzav
espzav approved these changes Nov 21, 2023
View reviewed changes
Copy link
Collaborator

@espzav espzav left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@pborcin Good job! Thank you. Tested on HW - OK!
@tore-espressif As a led_indicator and generic BSP are done, we decided to change the LEDs here to led_indicator now, before merging for not making breaking change in future. Are you agree?
Now question is, what API we need? Is it enought only to bsp_led_indicator_create or do we want bsp_led_set or bsp_led_rgb_set?

@tore-espressif
Copy link
Collaborator

Thanks @pborcin and @espzav

Now question is, what API we need? Is it enought only to bsp_led_indicator_create or do we want bsp_led_set or bsp_led_rgb_set?

Since the PR is open for quite some time, I'd prefer merging with only bsp_led_indicator_create. We can always add more public functions after we are sure about their usage in the future

@pborcin pborcin force-pushed the feature/esp32_s3_korvo_1 branch from debf464 to 5fbc261 Compare November 24, 2023 12:55
@pborcin
Copy link
Collaborator Author

pborcin commented Nov 24, 2023

@tore-espressif @espzav PTAL

tore-espressif
tore-espressif requested changes Nov 27, 2023
View reviewed changes
Copy link
Collaborator

@tore-espressif tore-espressif left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@pborcin I left one comment. Also, please squash your commits, before merging!

@pborcin pborcin force-pushed the feature/esp32_s3_korvo_1 branch from 312dc7d to 604f666 Compare November 27, 2023 08:13
@tore-espressif
Copy link
Collaborator

@espzav please hit the merge button when you're done with the review, thanks!

espzav
espzav approved these changes Nov 27, 2023
View reviewed changes
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.

@pborcin Thank you very much for changes for led_indicator! Great work! Only one comment left. LGTM

bsp/esp32_s3_korvo_1/include/bsp/esp32_s3_korvo_1.h Show resolved Hide resolved
@pborcin pborcin force-pushed the feature/esp32_s3_korvo_1 branch from 604f666 to 8ac0e0d Compare November 27, 2023 10:49
@espzav espzav merged commit 493b81f into espressif:master Nov 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@espzav espzav espzav approved these changes

@tore-espressif tore-espressif tore-espressif approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pborcin @CLAassistant @tore-espressif @espzav