Skip to content
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

update comms logs with new logging API #1169

Merged
merged 9 commits into from Dec 10, 2016

Conversation

@technobly
Copy link
Member

commented Nov 14, 2016

Updated system communication logging with new logging API

  • 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

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:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation (N/A)
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Enhancement

  • Updated system communication logging with new logging API
update comms logs with new logging API
- 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

@technobly technobly added this to the 0.7.x milestone Nov 14, 2016

@sergeuz sergeuz self-assigned this Nov 19, 2016

@technobly technobly referenced this pull request Nov 21, 2016
5 of 7 tasks complete

@technobly technobly modified the milestones: 0.7.x, 0.6.1 Nov 29, 2016

@@ -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 );

This comment has been minimized.

Copy link
@sergeuz

sergeuz Dec 1, 2016

Member

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.

This comment has been minimized.

Copy link
@technobly

technobly Dec 1, 2016

Author Member

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Dec 1, 2016

Member

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")

This comment has been minimized.

Copy link
@sergeuz

sergeuz Dec 1, 2016

Member

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".

This comment has been minimized.

Copy link
@technobly

technobly Dec 1, 2016

Author Member

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Dec 1, 2016

Member

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,

This comment has been minimized.

Copy link
@sergeuz

sergeuz Dec 1, 2016

Member

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.

This comment has been minimized.

Copy link
@technobly

technobly Dec 1, 2016

Author Member

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.

technobly added 7 commits Dec 1, 2016
Merge branch 'develop' into feature/comms-logging
  Conflicts:
	communication/src/spark_protocol.cpp
	system/src/system_cloud_internal.cpp
Revert "switch Travis CI to use gcc 5.3.1 20160307"
This reverts commit d42d8d9.

We'll look to make this change for 0.7.0-rc.1
@sergeuz
sergeuz approved these changes Dec 9, 2016
Merge branch 'develop' into feature/comms-logging
  Conflicts:
	system/src/system_cloud_internal.cpp

@technobly technobly merged commit dfc5e68 into develop Dec 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/comms-logging branch Dec 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.