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

Add CMakeLists.txt for ESP-IDF integration and fix abs() call ambiguity #81

Merged
merged 2 commits into from
Sep 11, 2021

Conversation

gagank1
Copy link
Contributor

@gagank1 gagank1 commented Sep 5, 2021

I added a CMakeLists.txt file to use it as an ESP-IDF component. It assumes that arduino is available as a component. #80

The compiler threw some ambiguity errors with the call to abs() due to multiple definitions. Casting to int fixes in the call fixes this.

Tested ESP-IDF component integration on PlatformIO Espressif32 Platform v3.3.0

@gagank1 gagank1 changed the title Gagank1/esp idf component Add CMakeLists.txt for ESP-IDF integration and fix abs() call ambiguity Sep 5, 2021
@gin66
Copy link
Owner

gin66 commented Sep 7, 2021

Thanks for the patch. It is more or less only the CMakeLists.txt. Would have expected to be more complicated :-)

Unfortunately the abs patch, does not work for avr. The cast to int is not the same for avr and esp32. In the driver there should not be a single int, only uint32_t and so on, because of that reason. Otherwise this is a mistake on my side.

Perhaps it is better to avoid the name clash with abs by renaming the driver’s version with e.g. fas_abs(). Or at least, use the proper cast: remaining steps is an uint32_t. Actually the first solution seems to be the better one.

One more request: Could you please add a couple of words in the readme, how to use this with esp_idf ?

@gin66 gin66 merged commit 0954531 into gin66:master Sep 11, 2021
gin66 added a commit that referenced this pull request Sep 11, 2021
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.

2 participants