-
Notifications
You must be signed in to change notification settings - Fork 90
Fix ESP8266 logging to not require global Serial object #380
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 ESP8266 logging to not require global Serial object #380
Conversation
|
tested on a real esp8266 with |
willmmiles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! This is strictly superior to the previous version.
Two non-blocking questions:
- Is there any value in moving the hardcoded buffer size constant
64to a separate #define, in case it needs to be adjusted? Or - given that the macro is instantiated at each call site - could the buffer size be set tosizeof(__fmt), so it'll float to the correct size for each format string? - As another way to save code space, could the fixed portion of the format string (
"%c async_ws %d: ") be stored in a common location for all calls instead of replicated for every call?
I didn't add a
Moving the prefix to a shared location would require two |
|
I will merge and issue a fix release tomorrow. |
That's true, but we aren't expanding text in to that buffer: it exists only to hold the format string. |
You are right, I was thinking about the earlier iteration. Let me fix that. |
still works fine |
|
Thanks for the quick release and review |
Fixes #379
PR #371 switched ESP8266 logging from
ets_printftoSerial.printf_P, while nice and convenient for PROGMEM strings on ESP8266, it requires the globalSerialobject. This breaks builds usingNO_GLOBAL_SERIALto free up UART0 pins for other purposes.This change restores using
ets_printfwith PROGMEM format strings and%cfor the log level (char literals don't consume RAM).Note: ESPHome may additionally use
ASYNCWEBSERVER_LOG_CUSTOMto integrate with its own logging system, but that's a separate decision. This PR fixes the regression for all other users.Untested - written while traveling and only verified it compiles ok for now. Will test on a real device when I reach a stable work environment.Tested on an esp8266