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

Fix the F_CPU frequency definition for the ESP32-S3 in esp32-hal.h #7913

Merged
merged 4 commits into from
Mar 31, 2023
Merged

Fix the F_CPU frequency definition for the ESP32-S3 in esp32-hal.h #7913

merged 4 commits into from
Mar 31, 2023

Conversation

devrim-oguz
Copy link
Contributor

@devrim-oguz devrim-oguz commented Mar 1, 2023

Description of Change

Hello, I was using the FastLED library and it was complaining about F_CPU not being defined. So, I just noticed that it is not defined for the ESP32-S3 module. So I made this change in the header file and it compiled. Therefore I wanted to propose this change to the HAL library to improve compatibility. Thank you for your time.

Tests scenarios

I have tested this with the ESP32-S3-WROOM-1-N16R2 module for the FastLED library. Haven't done any extensive testing since it is a trivial change (I hope).

#Fix #8005

VojtechBartoska and others added 2 commits February 20, 2023 18:50
Hello, I was using the FastLED library and it was complaining about F_CPU not being defined. So, I just noticed that it is not defined for the ESP32-S3 module. So I made this change in the header file and it compiled. Therefore I wanted to propose this change to the HAL library to improve compatibility. Thank you for your time.
@CLAassistant
Copy link

CLAassistant commented Mar 1, 2023

CLA assistant check
All committers have signed the CLA.

@mrengineer7777
Copy link
Collaborator

Interesting. LGTM. I wonder if C3 and other variants also need an entry?

@devrim-oguz
Copy link
Contributor Author

devrim-oguz commented Mar 2, 2023

Interesting. LGTM. I wonder if C3 and other variants also need an entry?

They probably would need to. And every single board that comes after. This is not a good way of doing it tbh, but that's how its defined in the sdkconfig. The interesting thing is that this error does not occur in PlatformIO, probably because it gets defined in somewhere else and this part is skipped. But it does so in esp-idf when arduino used as a component. So this is how I solved it for my own application. It would be better if the other boards are added to this, but I didn't look much about how many other variants there are.

Works for ESP32, ESP32C3, ESP32S2, ESP32S3
@SuGlider
Copy link
Collaborator

Interesting. LGTM. I wonder if C3 and other variants also need an entry?

They probably would need to. And every single board that comes after. This is not a good way of doing it tbh, but that's how its defined in the sdkconfig. The interesting thing is that this error does not occur in PlatformIO, probably because it gets defined in somewhere else and this part is skipped. But it does so in esp-idf when arduino used as a component. So this is how I solved it for my own application. It would be better if the other boards are added to this, but I didn't look much about how many other variants there are.

@mrengineer7777 @devrim-oguz

I just modified the PR to be generic, based on the sdkconfig definition of each SoC.
It shall now work for the ESP32, ESP32S2, ESP32C3 and ESP32S3.

@devrim-oguz - please test and confirm that it works for the FastLED library with the S3.

@SuGlider
Copy link
Collaborator

Interesting. LGTM. I wonder if C3 and other variants also need an entry?

They probably would need to. And every single board that comes after. This is not a good way of doing it tbh, but that's how its defined in the sdkconfig. The interesting thing is that this error does not occur in PlatformIO, probably because it gets defined in somewhere else and this part is skipped. But it does so in esp-idf when arduino used as a component. So this is how I solved it for my own application. It would be better if the other boards are added to this, but I didn't look much about how many other variants there are.

@mrengineer7777 @devrim-oguz

I just modified the PR to be generic, based on the sdkconfig definition of each SoC. It shall now work for the ESP32, ESP32S2, ESP32C3 and ESP32S3.

@devrim-oguz - please test and confirm that it works for the FastLED library with the S3.

OK... Actually the fix I did works for the IDF 5.1 and Arduino 3.0.0.
For Arduino 2.0.8, it shall follow the previous model.
Let me create 2 PRs, one for each Arduino Core version.

Necessary for ESP32 Arduino Core 2.0.x based on IDF 4.4
Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

Just added also support to ESP32C3.
Thanks @devrim-oguz and @mrengineer7777 !

@SuGlider SuGlider self-assigned this Mar 30, 2023
@devrim-oguz
Copy link
Contributor Author

Just added also support to ESP32C3. Thanks @devrim-oguz and @mrengineer7777 !

Thank you for making it available for everyone

Copy link
Collaborator

@mrengineer7777 mrengineer7777 left a comment

Choose a reason for hiding this comment

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

LGTM

@me-no-dev me-no-dev changed the base branch from master to release/v2.x March 31, 2023 11:37
@me-no-dev me-no-dev merged commit 93903fc into espressif:release/v2.x Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting triage Issue is waiting for triage Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.

esp32-s3 cannot find F_CPU for FastLed
6 participants