RIOT: make use of log.h for dsrv_log() #177
Conversation
|
Your contribution is very welcome, I considered to investigate for such a solution to improve the support of zephyr. Did you spend some time into analyze other ports, e.g. zephyr? Maybe you split this in logical "without logging" and the select that for RIOT? That would make it easier to adapt and use it for zephyr. |
|
This mostly just makes use of the pre-existing
But maybe it's better to do it the way it's done for the Zephyr port and provide custom log macros and not touch |
Makefile.riot
Outdated
|
|
||
| SRC := ccm.c crypto.c dtls.c dtls_debug.c dtls_time.c hmac.c netq.c peer.c session.c dtls_prng.c | ||
| ifneq (,$(filter tinydtls_debug, $(USEMODULE))) | ||
| SRC += dtls_debug.c |
There was a problem hiding this comment.
There are functions declared in dtls_debug.h, which are removed by this.
const char *dtls_package_name(void);
const char *dtls_package_version(void);
log_t dtls_get_log_level(void);
void dtls_set_log_level(log_t level);
void dtls_set_log_handler(dtls_log_handler_t app_handler);
void dsrv_log(log_t level, const char *format, ...);
I would consider to keep the dtls_package_name and dtls_package_version. If agreed, then the dtls_debug.c should not be excluded.
The "log" functions may then be added to the empty inlines.
And the excluded section in "dtls_debig.c" (NDEBUG) must be adjusted.
|
I tested the approach, and generally that works independent from the platform. FMPOV, it's needed to be more careful about the declared functions, not to result on linker errors. Please check, which of the declared function you think should be removed with NDEBUG, and which should be kept. |
|
I am a bit concerned about the usage of NDEBUG. It is used in <assert.h> to control whether assert() is a no-op or not, and then is also used to control whether there is other debug output or not. Is this the intent? What if you want assert() but no logging or vice versa? For example, the autoconf AC_HEADER_ASSERT (not currently used in TinyDTLS) controls whether NDEBUG is set or not with the intent of controlling assert() only. Another thought is that you configure the ability to only retain log entries for a defined logging level and above. For instance, if the configured/built max logging level is set to DTLS_LOG_WARN, then any logging entries that are DTLS_LOG_NOTICE, DTLS_LOG_INFO, DTLS_LOG_DEBUG will not get logged and the appropriate code is not there. |
NDEBUG is already used to reduce the code by removing the functions only used in log-level debug (logging of ip-addresses and hexdumps). For zephyr (and RIOT?) the other log-levels are using the platform specific functions, I guess that ended up in the idea of remove dtls_debug.c in that cases completely. This PR does this only for RIOT. So, I see two questions:
Overall, that may require to "consolidate" the "dtls_debug.c" file a little more in order to have it more transparent, when platform logging is used, and when "dtls_debug" is used. |
|
I think that NDEBUG should be renamed to DTLS_NDEBUG for the TinyDTLS specific stuff so there is no confusion / clashes with the use of NDEBUG in header file <assert.h>. It would help if AC_HEADER_ASSERT is in configure.ac. to give granular control over assert(). I do not think the public API functions should be compiled out when DTLS_NDEBUG is not set - the API must remain consistent for publicly available functions no matter how things are internally compiled. If there are internal functions that are not in the public API, then anything can be done with them as it will not affect any linking for a program. So, in answer to bullet 2 - No. |
OK, but I guess in an other PR.
This PR doesn't "compile them out", it replaces the function by empty inline ones (I remember a discussion about backwards compatibility. Removing renegotiation breaks that anyway. So if at all, I think it should be done before the 1.0). And using the platform's logging functions, some of the public API functions are without or limited effect (e.g. dtls_set_log_level or dtls_set_log_handler). FMPOV: |
So this relies on the higher level code using the same compiler flags that the library was built with when using the public header files to get the inline function version as opposed to getting a linking error because a function is defined in the header files, but there is no code. I really don't like that error prone route and have been burnt by it elsewhere. The public API must be consistent in how functions are provided no matter what the definitions used to compile the actual library. |
OK, so my conclusion:
(I guess, this will still save 19.5 of the 20 k bytes. Zephyr usually drops unused function when linking so there my saving using the approach of this PR was less, I guess mainly the other effects of the NDEBUG.) Are you OK, if we go that way? |
|
Sounds good to me. But should the existing occurences of |
|
If all agree, we will revise all usage of the NDEBUG macro. |
|
Removing the |
Signed-off-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Those flags are already configured by the RIOT package. Signed-off-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
|
LGTM |
|
What's the status on this? |
|
OK from your side? Or should I merge it to develop? |
|
@boaks Yes, please merge (= LGTM). My 2 cents on |
tinydtls allows to switch log levels at run-time.
This means that all debug log strings and formaters are included in the binary, which comes at a significant ROM cost.
On RIOT we can define a
tinydtls_debugpseudo-module that enables the debug facilities, but in it's absence we can just skip buildingdtls_debug.cand turn log functions into no-ops.We can still make use of RIOT's log facilities, but there the log level is set at build time so the compiler will turn everything below that into a no-op.