nxp-mcuxpresso / mcux-sdk

MCUXpresso SDK
BSD 3-Clause "New" or "Revised" License
323 stars 142 forks source link

Add DbgConsole_Vprintf/DbgConsole_BlockingVprintf new API #24

Closed dmitrykos closed 3 years ago

dmitrykos commented 3 years ago

to make it possible to wrap SDK's logging into a higher level user-side logging API.

Prerequisites

Describe the pull request

Current SDK's logging API (DbgConsole_Printf) is sufficient in case user-side implementation is using only NXP's logging but in reality it is not always true and for better debugging (for example when MCUXpresso IDE can not be used) some additional logging can be required, for example the one offered by Segger's J-Link RTT Viewer (a standalone console logging utility) and its companion API (SEGGER_RTT_vprintf).

User would want to wrap logging into a single logging function or logging C++ class but absence of vprintf-like API makes it really troublesome.

This PR adds vprintf functionality to the SDK and makes it possible to wrap SDK's logging API easily into a user-side wrapper. For example (pseudocode):

#if defined(SDK_DEBUGCONSOLE) && (SDK_DEBUGCONSOLE > 0)
void NxpDebugConsoleLogger::Warn(const char *format, ...)
{
    va_list args;
    va_start(args, format);

    DbgConsole_Vprintf(format, args);

    va_end(args);
}
#else
void SeggerRttLogger::Warn(const char *format, ...)
{
    va_list args;
    va_start(args, format);

    SEGGER_RTT_vprintf(0, format, &args);

    va_end(args);
}
#endif

As you can see NXP SDK's co-exist with Segger's vprintf API in a harmony and a user-side implementation is easy and straight forward. This change does not break the API and therefore safe to add without breaking compatibility with the existing user code, it is operating in the production code for around 1 year and works as expected.

PR also corrects the parameter names of the corresponding functions to make them in-line with the parameters of other functions (self descriptive, camel style).

Type of change (please delete options that are not relevant):

Tests

mcuxsusan commented 3 years ago

Hi @dmitrykos, excuse for my late update. I have asked our developer to evaluate the change. According to latest discussion, they have agreed to populate the change since this is vprintf API is also part of standard library. But they need some time to test the change so thanks in advance for your patience. Previously they are thinking whether you could reference the alternative implementation in component log instead of changing the debug console because the impact could be quite broad so it takes some time. The alternative way in component log inserts the customized format in front of the full log input. It uses the __VA_ARGS__ to get the full arguments and appends to the customized format, the implementation is quite efficient. For an example:

define LOG_FATAL(format, ...) \

_LOG_PRINTF(&s_LogModuleLogger, kLOG_LevelFatal, "%s:%d:" format "\r\n", LOG_FILE_NAME, __LINE__, ##__VA_ARGS__);

define _LOG_PRINTF(logger, logLevel, format, ...) \

if (((logLevel > kLOG_LevelNone) && ((logger)->level >= logLevel)))       \
{                                                                         \
    LOG_Printf(logger, logLevel, LOG_TIMESTAMP_GET, format, __VA_ARGS__); \
}

endif

dmitrykos commented 3 years ago

The alternative way in component log inserts the customized format in front of the full log input. It uses the VA_ARGS to get the full arguments and appends to the customized format, the implementation is quite efficient.

@mcuxsusan thank you for reviewing my PR. Yes, your proposal is a working solution but still it does not solve the inability to encapsulate the logging inside the implementation class. For example top-level API has some virtual logging interface, in my implementation I have such interface which has implementation for NXP, STM and Segger RTT logging functions and can be easily extended to other logging types, such as UART for example:

//! Logging interface.
struct ILog
{
    //! Initialize.
    virtual void Initialize() pure;

    //! Log information message.
    virtual void Info(const char *format, ...) pure;

    //! Log warning message.
    virtual void Warn(const char *format, ...) pure;

    //! Log failure message.
    virtual void Fail(const char *format, ...) pure;

    //! Check if input key is available.
    virtual bool IsKeyAvailable() pure;

    //! Get input.
    virtual int32_t GetKey() pure;
};

So the proposed API change allows to implement this class for NXP standard logging capabilities (see my PR description and example) when using a new DbgConsole_VprintfDbgConsole_Vprintf function. The implementation code with such approach is much cleaner and modular while overhead is just an additional function call that is ok for the described benefits.

DbgConsole_Vprintf proposed implaementation is based on already existing DbgConsole_Printf which was split into DbgConsole_Vprintf and a refactored version of itself using `DbgConsole_Vprintf.

I hope this change can be accepted as it simplifies logging task considerably while bearing no incompatibilities with the existing code using DbgConsole_Printf.

lylezhu2014 commented 3 years ago

The alternative way in component log inserts the customized format in front of the full log input. It uses the VA_ARGS to get the full arguments and appends to the customized format, the implementation is quite efficient.

@mcuxsusan thank you for reviewing my PR. Yes, your proposal is a working solution but still it does not solve the inability to encapsulate the logging inside the implementation class. For example top-level API has some virtual logging interface, in my implementation I have such interface which has implementation for NXP, STM and Segger RTT logging functions and can be easily extended to other logging types, such as UART for example:

//! Logging interface.
struct ILog
{
    //! Initialize.
    virtual void Initialize() pure;

    //! Log information message.
    virtual void Info(const char *format, ...) pure;

    //! Log warning message.
    virtual void Warn(const char *format, ...) pure;

    //! Log failure message.
    virtual void Fail(const char *format, ...) pure;

    //! Check if input key is available.
    virtual bool IsKeyAvailable() pure;

    //! Get input.
    virtual int32_t GetKey() pure;
};

So the proposed API change allows to implement this class for NXP standard logging capabilities (see my PR description and example) when using a new DbgConsole_VprintfDbgConsole_Vprintf function. The implementation code with such approach is much cleaner and modular while overhead is just an additional function call that is ok for the described benefits.

DbgConsole_Vprintf proposed implaementation is based on already existing DbgConsole_Printf which was split into DbgConsole_Vprintf and a refactored version of itself using `DbgConsole_Vprintf.

I hope this change can be accepted as it simplifies logging task considerably while bearing no incompatibilities with the existing code using DbgConsole_Printf.

Thanks for your contribution. Yes, it is meaningful. Although it may not be called in a short time for mcux-sdk, I think we sould integrate it. Actually, we are working on your PR.

dmitrykos commented 3 years ago

@lylezhu2014 that's great, thank you for an update.