NRF5x: redirect NRF logging to mbed stdout #6889

andrewleech · 2018-05-14T06:02:47Z

Description

The Nordic SDK libraries have a significant amount of logging available that can be very useful during development and debugging.
Unfortunately the provided logging engines expect the NRF5 SDK to be in direct control of the hardware which makes it unsuitable for most mbed use cases.

This PR replaces Nordic's logging backend/outputs with simple printf style log entries when the config entry softdevice.nrf_enable_logging is set to 1

As such the logs will go to the standard serial stdout provided by mbed.

This changeset was originally added in a slightly different form to #6860 but is complex enough to warrant its own PR

If/when #5965 is finished this nordic logging wrapper could be extended to take advantage of the log configuration / levels provided in that feature.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

marcuschangarm · 2018-05-16T15:48:13Z

I would like @donatieng 's opinion on whether this is a feature we want. From what I could glance it required adding some dummy files and mixing NRF51 macros with NRF52.

We finally got the NRF51 and NRF52 separated, it would be a shame to mix them up again.

andrewleech · 2018-05-16T20:37:58Z

The implementation for nrf51 (sdk11) is really very separate from nrf52 (sdk14) with no shared files.

The two take very different approaches, but that's just due to the massive changes nordic made to their logging subsystem.

The only shared element is the name of the define set by the config system to enable logging. If it's not enable (the default) there should be no change to existing functionality.

Modify the SDK11 softdevice_enable() logging to not dependent on Segger RTT being present & enabled. This change is not required in any later versions of SDK

… stdout (when enabled with softdevice.nrf_enable_logging config entry)

0xc0170 · 2018-05-22T12:36:22Z

@donatieng I removed the 5.9 label, please correct it if this is not the case.

This PR still needs review

donatieng

Hi @andrewleech, thanks for submitting this PR.

Let me start by saying that I like the intent being able to enable proper logging in the NRF SDK sounds great.

Couple points:

For the SDK14+, there's a 'log backend' feature that I think we should use
For SDK11 I guess things will have to be a bit patchy
I agree with @marcuschangarm that we shouldn't recouple implementations if not required
In both cases I don't think new files should live in the target's root directory and files need proper naming

donatieng · 2018-05-22T15:39:10Z

..._NORDIC/TARGET_NRF5x/TARGET_SDK_11/softdevice/common/softdevice_handler/softdevice_handler.c

    }
    else if (err_code != NRF_SUCCESS)
    {
        SD_HANDLER_LOG("sd_ble_enable: error 0x%x\r\n", err_code);
-        while(1);


I don't think we should remove this

Newer versions of the SDK don't hang here, but I guess it would also be more useful to redirect this to mbed_error()... though that would being in even more changes and dependencies so perhaps leaving it in place would be ok.

donatieng · 2018-05-22T15:40:05Z

targets/TARGET_NORDIC/TARGET_NRF5x/app_uart.h

@@ -0,0 +1,98 @@
+/* mbed Microcontroller Library


This seems to be a copy of the app_uart module from the Nordic SDK

Not quite. Much of it was cherry picked from the original Nordic SDK file to satisfy minimum requirements of the SDK logging interface, the rest is re-implemented to suit the redirection to stdin/stdout.

Sure, Nordic's license header should be kept though

donatieng · 2018-05-22T15:41:14Z

targets/TARGET_NORDIC/TARGET_NRF5x/nrf_log.h

+#define NRF_LOG_DEBUG(x, ...)                                        \
+    if (NRF_LOG_ENABLED && (NRF_LOG_LEVEL >= NRF_LOG_LEVEL_DEBUG))   \
+    {                                                                \
+            printf("[NRF_DBG] "x"\r\n", ##__VA_ARGS__);              \


I don't think this is portable (won't work if you do NRF_LOG_DEBUG("I don't have parameters");

Oh, don't the other compilers handle these kind of optional macro arguments? They really are substandard for the money you pay for them :-(

donatieng · 2018-05-22T15:41:44Z

targets/TARGET_NORDIC/TARGET_NRF5x/nrf_log.h

-#else
-#define NRF_LOG_MODULE_REGISTER() /*lint -save -e19*/ /*lint -restore*/
-#endif
+// #if NRF_LOG_ENABLED


That's a lot of commented-out code!

True, I forgot about them. Wasn't sure if they were actually needed/used in the SDK code we have.... it compiles without them though.

andrewleech · 2018-05-23T00:21:27Z

For the SDK14+, there's a 'log backend' feature that I think we should use

I started down the logging backend path and it was a massive amount of extra work for little visible gain. As soon as I started enabling the logging system as it stood I needed to being in a lot of extra fiels from the original SDK pack, including some of the third party code they bundle (the fprintf reimplementation) which I think started to conflict with builtin libraries.
There were more dependencies, more code size, and as far as I could tell would require modification to some actual SDK files to allow actually using a new backend implementation... and it seemed to enforce having only its built in ones actually compile and be linked in by default.

I switched to this path of replacing the logging from the top level interface (macros) down and it was a much simpler and smaller solution.

If there's a strong desire to keep the entire nordic logging framework though I can look at that again, although it will probably be a few weeks away before I'll be back on the nrf52 project here.

For SDK11 I guess things will have to be a bit patchy

Yeah there's more patches required, however if we ever upgrade nrf51 to SDK12 (the last with nrf51 support) these changes wouldn't be needed there as they changed the files used significantly.

I agree with @marcuschangarm that we shouldn't recouple implementations if not required

I'm pretty sure there's nothing coupled/shared between the two, I never intended to do this at all. They simply use the same name for the logging enable define for consistency, not dependency.

In both cases I don't think new files should live in the target's root directory and files need proper naming

Hah, I just looked at it again and it took me a while to find my files.... I never intended them to be in that sort of top level folder. When I first did this it was before the TARGET_NRF51 and TARGET_NRF52 folders were refactored under TARGET_NRF5x. My files were very much sitting alongside a bunch of NRF52 implementation files.
There's been a few rebases since then. I was always surprised they succeeded with no conflicts. Where they sit now is a result of one of them clearly, and yes it doesn't make sense. The three new headers are just for NRF52 and not used by NRF51.

The file naming was done to satisfy file includes in the SDK files, the alternative is modifying SDK files which I was attempting to avoid.

Would it be ok for app_uart.h, bsp.h and nrf_log.h to live in TARGET_NRF5x\TARGET_NRF52 or would they be better in a subfolder like logging. Nothing else is in a function subfolder there though.

cmonr · 2018-05-31T16:21:40Z

@marcuschangarm @donatieng Mind answering this?

donatieng

Hi @andrewleech, thanks for your response. It addresses most of might comments. The reason why I asked these files to be put in a separate directory is that the naming is not explicit. bsp.h is a bit of a hack (which might be required to keep the Nordic SDK happy) and app_uart.h does not convey that it's being used for a logging subsystem. If they can be renamed to something more explicit then that's fine having them here, however it'd be good to use a logging subfolder as you suggested.

donatieng · 2018-06-01T22:12:36Z

targets/TARGET_NORDIC/TARGET_NRF5x/app_uart.h

@@ -0,0 +1,98 @@
+/* mbed Microcontroller Library


Sure, Nordic's license header should be kept though

donatieng · 2018-06-01T22:12:51Z

targets/TARGET_NORDIC/TARGET_NRF5x/app_uart.h

+    } data;
+} app_uart_evt_t;
+
+//#define APP_IRQ_PRIORITY_LOW -1,


Please remove :)

donatieng · 2018-06-01T22:13:43Z

targets/TARGET_NORDIC/TARGET_NRF5x/bsp.h

+#ifndef DUMMY_BSP_H_
+#define DUMMY_BSP_H_
+
+const int RX_PIN_NUMBER = -1;


I'm afraid these dummy defines will clash with already declared variables, can we just get rid of this bsp.h file altogether?

cmonr · 2018-06-11T14:27:06Z

@andrewleech Any progress?

andrewleech · 2018-06-12T04:51:14Z

Sorry no I haven't progressed on this, running a bit behind here working on my nRF52810 support

cmonr · 2018-06-25T15:22:02Z

@andrewleech If this is still deprioritized in comparison to the nRF52910 PR, we'll be closing this PR within a couple of days. Once work is restarted on this PR, it can be reopened.

andrewleech · 2018-06-25T20:33:27Z

Sorry yes, that sounds fine. I'm jumping back and forth between a few projects at work, it will likely be a little while before I can finish this off

andrewleech changed the title ~~Nrf5x: redirect NRF logging to mbed stdout~~ NRF5x: redirect NRF logging to mbed stdout May 14, 2018

0xc0170 requested a review from marcuschangarm May 14, 2018 07:05

0xc0170 added the needs: review label May 14, 2018

cmonr added the release-version: 5.9.0-rc1 label May 14, 2018

0xc0170 requested a review from donatieng May 18, 2018 11:20

pi-anl added 2 commits May 22, 2018 11:41

Add target option to redirect NRF_LOG_* logging to mbed logging (stdout)

f8ec343

Modify the SDK11 softdevice_enable() logging to not dependent on Segger RTT being present & enabled. This change is not required in any later versions of SDK

Restructure NRF Logging for SDK 14.2 such that logging entries printf…

0b3df53

… stdout (when enabled with softdevice.nrf_enable_logging config entry)

andrewleech force-pushed the nrf5x_logging branch from 4c481f7 to 0b3df53 Compare May 22, 2018 01:47

0xc0170 removed the release-version: 5.9.0-rc1 label May 22, 2018

donatieng suggested changes May 22, 2018

View reviewed changes

donatieng suggested changes Jun 1, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jun 6, 2018

0xc0170 closed this Jul 2, 2018

0xc0170 removed the needs: work label Jul 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NRF5x: redirect NRF logging to mbed stdout #6889

NRF5x: redirect NRF logging to mbed stdout #6889

andrewleech commented May 14, 2018

marcuschangarm commented May 16, 2018

andrewleech commented May 16, 2018 •

edited

Loading

0xc0170 commented May 22, 2018

donatieng left a comment

donatieng May 22, 2018

andrewleech May 22, 2018

donatieng May 22, 2018

andrewleech May 22, 2018

donatieng Jun 1, 2018

donatieng May 22, 2018

andrewleech May 23, 2018

donatieng May 22, 2018

andrewleech May 23, 2018

andrewleech commented May 23, 2018

cmonr commented May 31, 2018

donatieng left a comment

donatieng Jun 1, 2018

donatieng Jun 1, 2018

donatieng Jun 1, 2018

cmonr commented Jun 11, 2018

andrewleech commented Jun 12, 2018

cmonr commented Jun 25, 2018

andrewleech commented Jun 25, 2018

NRF5x: redirect NRF logging to mbed stdout #6889

NRF5x: redirect NRF logging to mbed stdout #6889

Conversation

andrewleech commented May 14, 2018

Description

Pull request type

marcuschangarm commented May 16, 2018

andrewleech commented May 16, 2018 • edited Loading

0xc0170 commented May 22, 2018

donatieng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewleech commented May 23, 2018

cmonr commented May 31, 2018

donatieng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmonr commented Jun 11, 2018

andrewleech commented Jun 12, 2018

cmonr commented Jun 25, 2018

andrewleech commented Jun 25, 2018

andrewleech commented May 16, 2018 •

edited

Loading