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 for issue_3209. #3210

Merged
merged 3 commits into from Oct 1, 2019

Conversation

@william-ferguson-au
Copy link
Contributor

commented Sep 15, 2019

#include "esp32-hal-log.h" is mandatory is order to build BLEDevice.cpp.
It can't be left up to a compiler variable.

#include "esp32-hal-log.h" is mandatory is order to build BLEDevice.cpp.
It can't be left up to a compiler variable.
@me-no-dev

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

leftovers from when this library was hosted elsewhere :) it was used so it can compile in IDF without Arduino. Just removing it can cause other issues, like if you use Arduino as IDF component and have not selected to forward IDF errors to Arduino's logging. Then you will get compile error for undefined ESP_LOGx

@william-ferguson-au

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

So what's the solution then?

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

do as you have done for all files in the library and change ESP_LOGx(TAG, to log_x(

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

that will force the library to use Arduino's logging and since it's a part of it, that is fine :)

@william-ferguson-au

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Sorry, that seems backward to me.
The other arduino-esp32 libraries all use ESP_LOGx(Tag,

Isn't the aim of the arduino-esp32 libraries to provide components usable in an Arduino context that map to esp32 APIs?

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

what arduino libraries use ESP_LOGx(Tag, ? arduino libs use log_x(

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

ESP_LOGx is IDF API that is not controllable through the Arduino IDE and API. Once included IDF libs are precompiled, that is what it is for them. Therefore Arduino uses log_x as logging API and that is controllable through the IDE.

@william-ferguson-au

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

My mistake.

@william-ferguson-au

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

OK, reopening because I believe I was correct and have run into this again trying to upgrade from 1.0.3 to master. Our conversation became derailed at this comment #3210 (comment).

BLEDevice.cpp is failing to compile because it can't find the log_x() functions.

It can't find them because they are declared in esp32-hal-log.h and unlike all the other BLE sources which always include esp32-hal-log.h, BLEDevice.cpp only includes esp32-hal-log.h when the compiler var CONFIG_ARDUHAL_ESP_LOG is defined.

BLEDevice.spp should always include esp32-hal-log.h instead of omitting it if CONFIG_ARDUHAL_ESP_LOG is not defined.

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

so... you made one change to one file, because you had issues with that file. I asked you to make the same change to all files and to replace any occurrence of ESP_LOGx with log_x. If you are not willing to do it, just say so and I'll do the whole thing myself. As-is, I will not merge just that change.

@william-ferguson-au

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Sorry, there seems to be some confusion here.
I changed BLEDevice.cpp because it was the only BLE file whose includes had not been changed when Bascy refactored all the BLE files to switch from ESP_LOGX to log_x (16-APR-2019).

As far as I am aware there aren't any arduino-esp32 files that are using ESP_LOGX.
So the only change that needs to be made is the one that I have changed.

@me-no-dev

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

yup confusion :)

@me-no-dev me-no-dev merged commit 8a46697 into espressif:master Oct 1, 2019
20 checks passed
20 checks passed
Build Arduino IDE Tests 0
Details
Build Arduino IDE Tests 1
Details
Build Arduino IDE Tests 2
Details
Build Arduino IDE Tests 3
Details
Build Arduino IDE Tests 4
Details
Build Arduino IDE Tests 5
Details
Build Arduino IDE Tests 6
Details
Build Arduino IDE Tests 7
Details
Build Arduino IDE Tests 8
Details
Build Arduino IDE Tests 9
Details
Build Arduino IDE Tests 10
Details
Build Arduino IDE Tests 11
Details
Build Arduino IDE Tests 12
Details
Build Arduino IDE Tests 13
Details
Build Arduino IDE Tests 14
Details
Build Arduino IDE Tests 15
Details
Build Arduino IDE Tests 16
Details
Build Arduino IDE Tests 17
Details
Build PlatformIO Tests
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.