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

Changing _ESPLOGLEVEL_ requires editing debug.h #88

Closed
Llaves opened this issue Feb 28, 2017 · 8 comments
Closed

Changing _ESPLOGLEVEL_ requires editing debug.h #88

Llaves opened this issue Feb 28, 2017 · 8 comments

Comments

@Llaves
Copy link

Llaves commented Feb 28, 2017

Of course, this is not the end of the world, but if debug.h contained

#ifndef _ESPLOGLEVEL_
#define _ESPLOGLEVEL_ 3
#endif 

then if the user defines ESPLOGLEVEL before

#include "WiFiEsp.h"

then the user value will be used.
(Or so I think unless I'm having brain spasm.)

@bportaluri
Copy link
Owner

Fix committed

@per1234
Copy link

per1234 commented Feb 14, 2018

This is incorrect. The debug macros are used by a different compilation unit from the sketch. The macro defined in the sketch will not be defined in EspDrv.cpp and so _ESPLOGLEVEL_ will still be 3 when debug.h is included by EspDrv.cpp. Spend a minute to check if you don't believe me.

I think it would be a huge improvement to this library to allow the user to control debug output from the sketch. There is a way to do this. You need to put all the code that uses the debug macros in the header file where the macro defined in the sketch will take effect.

@Llaves
Copy link
Author

Llaves commented Feb 14, 2018

per1234- you are right, of course. The various .cpp files of the library are separate compilation units and thus won't see the #define in the sketch. However, it's not clear to me the cure is any better than the disease. Because the various debug/logging macros are used extensively in the code, large fractions of the code will have to be moved to the .h file. For example, nearly the entire EspDrv.cpp file will have to be moved to EspDrv.h. While this would be valid (ie, compilable) code, it is unusual to see so much code in a header file.

A better solution would be for the IDE to standardize the use of debug/logging flags, as this is almost certainly not the only library where this capability would be useful. The IDE would then provide a means to set the flag and pass the value to the compiler. A general means to pass -D options to the compiler would be powerful, but risks confusing novice users of the IDE.

@per1234
Copy link

per1234 commented Feb 14, 2018

Well I see four options:

  1. No action. The library doesn't work out of the box for many users (example: debug.h defaults to writing output to hardware serial #84). The process of making it work is undocumented and not user friendly. There is unnecessary overhead for users who no longer need the debug output.
  2. Wait for the flag feature to be added to the Arduino IDE. This request has been brought up regularly but has consistently not received support from the Arduino team so don't hold your breath.
  3. Document the problems associated with the debug output and how to edit the library to turn off debugging. This will still not be very user friendly and @bportaluri already ignored that suggestion when I made it long ago.
  4. Move some code to the header file. This is really not so unusual. In fact there are quite a few popular header-only Arduino libraries, often done so for this exact reason.

The best option seems quite clear to me. I regularly recommend this library to people on the Arduino forum but always must go into a long explanation of this debug output issue so I would like to see it fixed.

@JAndrassy
Copy link

if _ESPLOGLEVEL_ is already implemented then adding it to board.txt will apply it to all sources

uno.build.extra_flags=-D_ESPLOGLEVEL_=3

@per1234
Copy link

per1234 commented Feb 14, 2018

Sure, or you can even add a custom Tools menu to change the level if you like but this is not a reasonable thing to expect from the average user who's just trying to use the library with their ESP8266 shield.

@Llaves
Copy link
Author

Llaves commented Feb 14, 2018

To make this work by moving code into the header file will require moving the entire code into a single header file. As long as there are multiple compilation units, the problem will remain, just in a different form.
(Minor nit - you could have multiple header files, but still can't have separate compilation units.)

Of the choices in per1234's message, I favor option 3 with _ESPLOGLEVEL_ defaulting to 0. If you are too novice a user to be able to understand instructions on how to change the log/debug level, you're probably too novice to be able to make sense of the debugging/logging information. This also solves the problem of the conflict with hardware serial.

@aster94
Copy link

aster94 commented Feb 16, 2018

+1 to enable/disable the debug level from the sketch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants