luxonis / XLink

A cross-platform library for communicating with devices over various physical links.
Apache License 2.0
11 stars 17 forks source link

mvLog() is double-spacing; how use mvLog() in regards to trailing newline? #64

Open diablodale opened 1 year ago

diablodale commented 1 year ago

XLink double-spaces the mvLog output often. Nothing breaks, but when the log volume is large and multiple things are logging, the doublespacing doesn't help. 😉

While writing XLink code, I see inconsistencies of mvLog() format strings. Some have a trailing \n and some do not. On Windows and Linux, this trailing newline in the format string causes doublespaced log output on the console.

This can be resolved by choosing a policy, enforcing it, and then adjusting the mvLog calls to follow that policy. Bulk changes are easy with tools like vscode its regex search/replace.

Setup

Repo

Found by watching logs and then mvLog code review. Easy repro is to...

  1. add the following code at the top of the function XLinkPlatformFindDevices

    mvLog(MVLOG_FATAL, "Test line 1\n");
    mvLog(MVLOG_FATAL, "Test line 2\n");
    mvLog(MVLOG_FATAL, "Test line 3\n");
  2. config, build for Debug

  3. set env var XLINK_LEVEL=fatal the value must be lowercase

  4. run the test examples/list_devices

Result

Both Windows and Linux have double-spaced log entries

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:58  Test line 1

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:59  Test line 2

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:60  Test line 3

Expected

F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:58  Test line 1
F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:59  Test line 2
F: [global] [         0] [ThreadN] XLinkPlatformFindDevices:60  Test line 3

Notes

This is due to use of \n in the format string. When code calls mvLog(MVLOG_FATAL, "Test line 1\n"); with a trailing space, this causes double-spaced entries due to a trailing newline also added by mvLog() itself. There are 4 platform scenarios. 3/4 adds a newline. 1/4 (Android) doesn't.

https://github.com/luxonis/XLink/blob/457b23fb33e1146610e1a4e52defa7565046977a/src/shared/XLinkLog.c#L162-L186

themarpe commented 1 year ago

+1 on this - I'd go with mvLog adding the trailing newline itself and then removing the explicit \n in the calls, as this is how other libraries handle it usually as well

diablodale commented 1 year ago

The fix is in my fork, github linked to it above. Should be an easy cherry-pick; usb_host.cpp might not merge automatically.

I used vscode regex search/replace of:

mvLog\((.+)\\n"(.+)
mvLog($1"$2

then hand edited 4 in src\shared\XLinkDispatcherImpl.c due to their calls being multiline. then verified diff by eye. I found a few that had trailing spaces that I manually removed.

I also saw raw printf() and perror() calls in tcpip_host.cpp and XLinkDispatcher.c files, DEBUG() macro, functions like XLinkProfPrint(), etc. The OP issue and the linked fix do not address these. The printf output may be poorly interlaced with mvLog() output since they are on a different output buffer than mvLog().