update comms logs with new logging API #1169
Conversation
- makes logs available by default without compiling with DEBUG_BUILD=y - improves logging in MBEDTLS to include \r\n after messages - added INFO logs around main parts of comms process
| @@ -70,7 +70,7 @@ static inline void debug_send_line( const mbedtls_ssl_context *ssl, int level, | |||
| */ | |||
| #if defined(MBEDTLS_THREADING_C) | |||
| char idstr[20 + DEBUG_BUF_SIZE]; /* 0x + 16 nibbles + ': ' */ | |||
| mbedtls_snprintf( idstr, sizeof( idstr ), "%p: %s", ssl, str ); | |||
| mbedtls_snprintf( idstr, sizeof( idstr ), "%p: %s\r\n", ssl, str ); | |||
There was a problem hiding this comment.
Can we just add single "\r\n" sequence to my_debug callback which all mbedtls debugging output is being forwarded to?
As I side note, I think we should avoid making non-critical changes in our third party dependencies whenever possible, since it complicates merging. Ideally, when we know that we didn't make any changes in some library, we should be able to just replace its sources with newer version.
There was a problem hiding this comment.
I did originally do that via debug_send_line, but there are some cases where it's not appropriate, like a buffer dump. So the higher level API needed modification instead. The above change to line 73 I believe was residual testing that should have been removed (MBEDTLS_THREADING_C is not currently defined). If we add \r\n to my_debug most of the log messages would end with \n\r\n adding a bunch of space to the log output. I agree we should try to minimize changes to 3rd party libraries, but we also need to have effective logs. I think these changes should merge with upstream MBEDTLS library changes without conflict. I'll revert the change on line 73.
There was a problem hiding this comment.
Ideally, new line sequence should be specific to a log handler, as "\r\n", for example, makes sense only for serial logging. We can make the logging framework perform new line conversions on the fly, but, surely, that's not something that needs to be discussed in the scope of this PR.
| @@ -17,17 +17,19 @@ | |||
| ****************************************************************************** | |||
| */ | |||
|
|
|||
| #include "logging.h" | |||
| LOG_SOURCE_CATEGORY("message_channel.dtls") | |||
There was a problem hiding this comment.
LOG_SOURCE_CATEGORY() (and all other macros which define log categories) cannot automatically make a category passed to it to be a subcategory of a category defined at module level ("comm" in this case). It's necessary to specify full name for all defined categories, e.g. "comm.message_channel.dtls".
There was a problem hiding this comment.
I was hoping to not have to hard code the parent category, but I do like the idea of knowing the logs are a sub category of the communication library. Is it fair to say all files within the communication/ folder will have a default parent category of "comm" ? That's probably a better way of keeping log messages filterable by "comm" as well ;-) I'll make this change in a couple places.
There was a problem hiding this comment.
Yes, if category name is not defined explicitly for a log statement's scope, upper level category name is used, up to category specified at module level (which is "comm" for communication/ directory). I didn't find a way to make categories easy to define and override, and also maintain nesting automatically at compile time.
| /* 22 */ IO_ERROR_LIGHTSSL_BLOCKING_RECEIVE, | ||
| /* 23 */ IO_ERROR_LIGHTSSL_RECEIVE, | ||
| /* 24 */ IO_ERROR_LIGHTSSL_HANDSHAKE_NONCE, | ||
| /* 25 */ IO_ERROR_LIGHTSSL_HANDSHAKE_RECV_KEY, |
There was a problem hiding this comment.
We now have generic system error codes which can be passed between system modules or exposed to application code if necessary. There's SYSTEM_ERROR_IO code and currently only protocol's IO_ERROR is mapped to it: https://github.com/spark/firmware/blob/develop/communication/src/protocol_defs.cpp#L27 – this function needs to be updated for newly added IO_ERROR_* codes.
There was a problem hiding this comment.
Thanks for this! I'll add that with a note that if the ProtocolError's get expanded they also need to be added to the system_error particle::protocol::toSystemError(ProtocolError error) { switch.
Conflicts: communication/src/spark_protocol.cpp system/src/system_cloud_internal.cpp
…errors in system error lookup [ci skip]
This reverts commit d42d8d9. We'll look to make this change for 0.7.0-rc.1
Conflicts: system/src/system_cloud_internal.cpp
Updated system communication logging with new logging API
Comments welcome on changes to logging categories. Some categories were used in a more local scope such as "handshake" to reduce the size of the log messages (each had "Handshake: " on them before).
Doneness:
Enhancement