nxp-mcuxpresso / mcux-sdk-examples

MCUXpresso SDK Examples
Other
57 stars 28 forks source link

Merge Freertos and fsl_assert.c assert approach #13

Closed Hadatko closed 7 months ago

Hadatko commented 2 years ago

Hello, i was thinking about asserts. It looks like freertos is using its approach and NXP its approach. I was thinking if these approaches shouldn't be merged.

For example: https://github.com/NXPmicro/mcux-sdk-examples/blob/ea04bc2a30f4dee05b14259727e4c57314093935/evkmcimx7ulp/demo_apps/power_mode_switch/FreeRTOSConfig.h#L88

Could be changed to:

#include <assert.h>
#define configASSERT(x)           \
    if ((x) == 0)                 \
    {                             \
        taskDISABLE_INTERRUPTS(); \
        assert(x);
    }

for casses when fsl_assert.c is used. Nices solution could be if this could be changed to #define configASSERT(x) assert(x); and in fsl_assert.c we could identified that we are inside Freertos task and call also taskDISABLE_INTERRUPTS();

What do you think? This is related to all Freertos examples in sdk i think.

Hadatko commented 2 years ago

To minimize assert implementations we did this in eRPC project: image

taskDISABLE_INTERRUPTS() can be handy in examples asserts

Hadatko commented 2 years ago

From experience i would say it could looks like (maybe not all cases covered): FreertosConfig.h:

#include <assert.h>
#define configASSERT(x) assert(x)

fsl_assert.c:

/*
 * Copyright (c) 2015-2016, Freescale Semiconductor, Inc.
 * Copyright 2016-2017 NXP
 * All rights reserved.
 *
 *
 * SPDX-License-Identifier: BSD-3-Clause
 */

#include "fsl_common.h"
#include "fsl_debug_console.h"

#if defined(__has_include)
    #if __has_include("FreeRTOSConfig.h")
        #undef ERPC_HAS_FREERTOSCONFIG_H
        #define ERPC_HAS_FREERTOSCONFIG_H (1)
    #endif
#endif

#if  ERPC_HAS_FREERTOSCONFIG_H
#include "FreeRTOS.h"
#include "task.h"
#endif

#ifndef NDEBUG
#if (defined(__CC_ARM)) || (defined(__ARMCC_VERSION)) || (defined(__ICCARM__))
void __aeabi_assert(const char *failedExpr, const char *file, int line)
{
#if  ERPC_HAS_FREERTOSCONFIG_H
    if (xPortIsInsideInterrupt() == pdFALSE)
    {
        taskDISABLE_INTERRUPTS();
    }
#endif
#if SDK_DEBUGCONSOLE == DEBUGCONSOLE_DISABLE
    PRINTF("ASSERT ERROR \" %s \": file \"%s\" Line \"%d\" \n", failedExpr, file, line);
#else
    (void)PRINTF("ASSERT ERROR \" %s \": file \"%s\" Line \"%d\" \n", failedExpr, file, line);
#endif

    for (;;)
    {
        __BKPT(0);
    }
}
#elif (defined(__GNUC__))
#if defined(__REDLIB__)
void __assertion_failed(char *failedExpr)
{
#if  ERPC_HAS_FREERTOSCONFIG_H
    if (xPortIsInsideInterrupt() == pdFALSE)
    {
        taskDISABLE_INTERRUPTS();
    }
#endif

    (void)PRINTF("ASSERT ERROR \" %s \n", failedExpr);
    for (;;)
    {
        __BKPT(0);
    }
}
#else
void __assert_func(const char *file, int line, const char *func, const char *failedExpr)
{
#if  ERPC_HAS_FREERTOSCONFIG_H
    if (xPortIsInsideInterrupt() == pdFALSE)
    {
        taskDISABLE_INTERRUPTS();
    }
#endif

    (void)PRINTF("ASSERT ERROR \" %s \": file \"%s\" Line \"%d\" function name \"%s\" \n", failedExpr, file, line,
                 func);
    for (;;)
    {
        __BKPT(0);
    }
}
#endif /* defined(__REDLIB__) */
#endif /* (defined(__CC_ARM) || (defined(__ICCARM__)) || (defined(__ARMCC_VERSION)) */
#endif /* NDEBUG */
Hadatko commented 1 year ago

@mcuxsusan Any inputs here?

DavidJurajdaNXP commented 7 months ago

Hi @Hadatko,

thank you for your comments. Generally the idea of centralized standard assertion mechanism is right. Technical limitation in case of FreeRTOSConfig.h is caused by header file inclusion into assembler port files like https://github.com/nxp-mcuxpresso/FreeRTOS-Kernel/blob/MCUX_2.15.000/portable/IAR/ARM_CM4F/portasm.s#L29

In IAR EWARM it results in error: ''' portasm.s
Error[2]: Failed to open #include file 'assert.h' '''

FreeRTOSConfig.h inclusion into assembler files is main reason for simplified configASSERT implementation: https://github.com/nxp-mcuxpresso/FreeRTOS-Kernel/blob/MCUX_2.15.000/template/ARM_CM4F_4_priority_bits/FreeRTOSConfig.h#L101

Hadatko commented 7 months ago

Thank you for explanation