Skip to content

Refactored use of LOG_X(LOG_TAG, ...) to log_x(...) #2672

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 15 commits into from
Apr 15, 2019

Conversation

Bascy
Copy link
Contributor

@Bascy Bascy commented Apr 14, 2019

LOG_TAG is not used in implementation of logging

@@ -82,7 +82,7 @@ void MDNSResponder::setInstanceName(String name) {

void MDNSResponder::enableArduino(uint16_t port, bool auth){
mdns_txt_item_t arduTxtData[4] = {
{(char*)"board" ,(char*)ARDUINO_VARIANT},
{(char*)"board" ,(char*)"esp32"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be reverted, the ARDUINO_VARIANT variable is injected with the board name typically. The ESPmDNS library checks explicitly for this define and if missing defines it itself as you have done here: https://github.com/espressif/arduino-esp32/blob/master/libraries/ESPmDNS/src/ESPmDNS.h#L48-L51

Copy link
Member

Choose a reason for hiding this comment

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

yeah, why would you change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why all these commits are part of this pull request, these are from months ago, and already reverted locally.
It also looks like that code isn't there anymore

@me-no-dev me-no-dev merged commit 01d7ea7 into espressif:master Apr 15, 2019
@Bascy Bascy deleted the LoggingWithoutTAG branch April 15, 2019 19:36
@robert-alfaro
Copy link
Contributor

robert-alfaro commented May 9, 2019

@Bascy @me-no-dev

Is this refactoring really necessary? It is now broken. If the kconfig does not select CONFIG_ARDUHAL_ESP_LOG (ESP Log forwarding to Arduino Log) then log_X macros are undefined because the includes are missing.

Previously, if you wanted forwarding then ESP_LOGX automatically turned into log_X, otherwise ESP_LOGX macros would be used if opted out -- this functionality/flexibility is lost now. I have updated a recent fork and this breaks code which does not use Arduino's logging.

EDIT: My apologies..I missed that this was fixed right after v3.2 tag there is a commit to fix the #include lines at the top of BLEDevice.cpp. Thanks!

@me-no-dev
Copy link
Member

this is an Arduino library and will always have CONFIG_ARDUHAL_ESP_LOG defined :) includes will be there and all. What is your use case?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants