Your definition of success ...

[jelledevleeschouwer]

Hi @ST-dot-com and @ALABSTM

Your enum definition of SUCCESS in Include/stm32l0xx.h makes including that file dependent on the order in which it is included with respect to another header file that potentially defines SUCCESS as well. An example of such a header file can be found in the Semtech LoRaMAC-Node stack, a reference implementation of a LoRa network node.

While they have pushed a workaround to this problem 9 months ago in commit f6282aa to their mainline branch, the issue still persists in (most, from the looks of it) STMicroelectronics' CMSIS device packages.

Commonly accepted solutions to these kind of problems is (1) to name-mangle the enum definitions with a prefix to give them a narrower scope or (2) to use the C preprocessor to prevent from redefining the object-like macro if it is already defined. The latter solution could be potentially dangerous however, if the expanded value is used in a comparison expression and the the previously defined definition is the opposite of what is defined locally. However, if the expanded values are never passed to the client code this risk could be limited to a minimum. However, I do notice that this happens in the Low Level HAL drivers. So that leaves us with solution 1. Hence, I propose you rename SUCCESS and ERROR to ERROR_STATUS_SUCCESS and ERROR_STATUS_ERROR, respectively.

Describe the set-up

• Board: B-L072Z-LRWAN1
• Toolchain: GNU Tools for Arm Embedded Processors 9-2019-q4-major

Let me know what you think, Best regards, Jelle

[ALABSTM]

Hi @jelledevleeschouwer,

Thank you for this summary and proposal. May I ask how using ERROR_STATUS_SUCCESS and ERROR_STATUS_ERROR as new redefinitions would solve the problem in case, by chance, a client code uses exactly the same? Thank you for the clarification.

PS: best wishes for the new year 2022.

[jelledevleeschouwer]

Hi @ALABSTM,

First of all, best wishes for 2022 as well!

To answer your question: to be completely fair, it does not solve the problem in that case. But what I was trying to get at is that you prefix the enum definitions themselves with some prefix that binds or confines the definitions to the enum that it is defined in. You create a kind of "scope" by prefixing it. For example, in this case, the name of the enum is ErrorStatus so you prefix the definitions SUCCESS and ERROR with ErrorStatus (if you prefer camel case) or ERROR_STATUS_ (if your prefer snake-case) which would yield ErrorStatusSUCCESS and ErrorStatusERROR, or ERROR_STATUS_SUCCESS and ERROR_STATUS_ERROR, respectively.

Another example, say I have an enum for car colors. I would define it like:

typedef enum car_color 
{
    CAR_COLOR_BLUE,
    CAR_COLOR_RED
}
car_color_t;

To not mix these up with some other module that defines colors.

So maybe in this case it would be preferable to maybe prefix those definitions with something that confines these definitions to the scope of the ST CMSIS module, for example ST_SUCCESS and ST_ERROR. The chance of a collision then is even less likely than using ERROR_STATUS_ as a prefix. But whatever, you may still find the pre-processor guard the better solution, if you do not want to break legacy code for example.

Anyway, I think something has to be done about this, since it may cause trouble to some customers using this module. Which consequences and trade-offs you can afford and hence, how this problem is resolved is not up to me to evaluate. I personally would feel comfortable with my both (1 & 2) suggested solutions, though.

Regards, Jelle

[ALABSTM]

Hi @jelledevleeschouwer,

Thank you again for your reply. I will submit the point to our development teams for deeper analysis.

With regards,

[hubpav]

@jelledevleeschouwer I fully agree with your suggestions on enum conventions - this is the scalable way how to keep things tidy and without conflicts.

But generally speaking of return codes, frankly, I would stick with the conventions proven over decades in other environments - such as POSIX (Linux, BSD, etc.), or Zephyr RTOS, and many others: Simply return the int type, where zero means success and negative means failure. For actual erroneous return codes, use #include <errno.h>, exactly as Zephyr does. There are many useful pre-defined error codes and this can help trace problems while printing return values when traversing call tree.

This is the usual error-handling skeleton that works:

#include <errno.h>

int bar(void)
{
        return -ENOTSUP;
}

int foo(void)
{
        int ret;

        ret = bar();
        if (ret) {
                LOG_ERR("Call `bar` failed: %d", ret);
                return ret;
        }

        return 0;
}

CC @ALABSTM

[jelledevleeschouwer]

@hubpav Amen! That has been my preferred solution for a long time now. Thanks for also putting it forward.

I did not bring that up since I assumed that ST is conservative about making changes in their API like this, since their customer base is quite vast and diverse. But that might just be an assumption, maybe (and hopefully) @ST-dot-com proves me wrong.

[ALABSTM]

Hi @jelledevleeschouwer and @hubpav,

Thank you for your comments and proposals. The point has been submitted to our development teams. We will get back to you as soon as we get their feedback.

With regards,

[ALABSTM]

ST Internal Reference: 123347

[ALABSTM]

Hi @jelledevleeschouwer and @hubpav ,

I hope you are fine. Back to this point you tackled some time ago. We finally have the feedback of our development teams. While they may understand the reasons behind such a request, they prefer to keep these definitions as they are for the moment. We will keep you informed, should there be any change.

Please allow me to close this issue. Thank you for your comprehension.

With regards,