-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Review] feat(pubsub): change std_log.h into log.h, implement configurable loger for pubsub_ethernet module #3570
base: master
Are you sure you want to change the base?
Conversation
Hey, Thanks for the effort. I have a few requests from a maintainer point.
That said, using the configured logging is very welcome. |
…er for pubsub_ethernet module Signed-off-by: Dovydas Girdvainis <[email protected]>
5e68c83
to
220c3c0
Compare
Hi @jpfr , Thanks for your feed back! I addressed the issues you mentioned:
I removed the clang-formater changes and left in only the PR relevant changes themselves.
I think my initial comment was not really clear. This change uses a
I am not sure what you mean by this, log.h is the header that contains the definition for Correct me if I am wrong, but to me this is a typical Interface and the point of an interface is to separate concrete implementation of a functionality from the user of that functionality. Thus if log_stdout.h header stays, there is no way to implement a configurable logger, since Furthermore, this change still allows for the use of |
@jpfr, this PR is ready for review |
Ill re-add the log_stdout.h and try to do something similar to PR #3567 and contexts |
…sub_ethernet.c Signed-off-by: Dovydas Girdvainis <[email protected]>
Hey @jpfr, I moved |
The core of the change looks good.
Can you clean up the PR and rebase? Then we can merge. |
sure thing, I'll commit the fixes next week |
sorry for the long delay, applied the changes, ready for review |
Description
This PR removes the use of the standard log_stdout.h from ua_pubsub_ethernet.c and implements the use of a more generalized log.h in its place.
Simmilarly to PR#3567 this is achieved by using a
static UA_Logger
variable, which is initially set toNULL
and set during configuration when setting the transport layer byUA_PubSubTransportLayerEthernet(&config->logger)
A
NULL
logger is acceptable since log.h checks against it, and if it is found, no logging is done, which in principal, allows for full deactivation of the logging plugin during the configuration process.