-
Notifications
You must be signed in to change notification settings - Fork 50
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
Allow redefinition of where logging goes #111
Conversation
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.
Thank you, please check the comments.
Artnet/Receiver.h
Outdated
protected: | ||
void attach(S& s) | ||
{ | ||
this->stream = &s; | ||
} | ||
|
||
#if defined(NO_GLOBAL_INSTANCES) || defined(NO_GLOBAL_SERIAL) |
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.
Could you let me know what these preprocessors are?
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.
They're the same macros that allow disabling Serial
et al in the esp32 arduino core (see here).
I had assumed when I was developing this that they were standard across all arduino cores, but it seems that they are not.
Artnet/Receiver.h
Outdated
protected: | ||
void attach(S& s) | ||
{ | ||
this->stream = &s; | ||
} | ||
|
||
#if defined(NO_GLOBAL_INSTANCES) || defined(NO_GLOBAL_SERIAL) | ||
Print* log = nullptr; // No Serial -> no output |
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.
I think if the user doesn’t set this, the program crashes. (nullptr->print() will be called)
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.
Upon further research: this is actually undefined behaviour (not neccesarily a crash), which is why I didn't catch it earlier.
Solution coming soon.
With logging disabled by default via the log variable it's somewhat redundant
Added a subclass of Removed This way we also don't have to worry about what happens if |
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.
Almost LGTM. Please check the comment
Sometimes I want to log to places other than
Serial
(when I have a display connected, or when my adapter is attached toSerial1
for example)Also adds guards so things don't break when the user disables
Serial