cpu/stm32_common/uart: Prevent uart from sending if not initialized

[MrKevinWeiss]

Contribution description

Due to the stdio getting called after periph_init the uart may send before initialized.
This adds a simple check so the uart does not get into a locked-up state.

Testing procedure

Use an stm32F1, F2, F4, or L0
Enable debug in tests/periph_i2c/main.c
Write a DEBUG message in the init.
BOARD=<selected board from above> make flash term -C tests/periph_i2c/
help
you should see a result, on master you won't and it will get locked up.

Issues/PRs references

fixes #10614
discussed in #10608

[MrKevinWeiss]

Low impact fix that seems like it could help with other issues. It adds 12 bytes or so...

[leandrolanzieri]

Tested on nucleo-f103rb and fixes the described issue.

[maribu]

Tested on a blue pill. Works as expected :-) Please wait 10 more minutes to let me also check on an F4, just to be sure.

[MrKevinWeiss]

I will also note that if I do some sort of refactor in the future I will attempt to reduce the byte size for the uart.

[maribu]

I can confirm that it also solves the issue on the MSB-IoT (stm32f415rg) :-)

[maribu]

All good from my point of view :-)

[jcarrano]

I don't think this should be enabled always, but be an assertion.

[maribu]

Well, assert() uses stdio as well. I'm not sure what would happen then. In any case, the user will not be able to "see" that the assertion failed, as no output over UART is available.

[jcarrano]

mmm, I get the point. The problem is that having the UART fail silently will be very surprising for the user.

[maribu]

I think the best user experience would be to make sure stdio becomes available as soon as possible. This would allow us using DEBUG() even in the initialization code of other stuff.

Maybe a good compromise would be to perform this check only when DEVELHELP is enabled. And also set some global flag (only present with DEVELHELP) when this occurs. At the very end of the initialization of stdio a check for that flag could be added (again, only with DEVELHELP) that would print a warning about lost stdio output.

[MrKevinWeiss]

That's why I like error codes everywhere. As it stands the periphs get initialized before the uart does and attempting to write on certain boards locks it up in an infinite loop waiting for a flag. An assert would fail if added. Maybe that is the behavior we desire but then all periphs that want to be debugged and have something in the init would crash.

[jcarrano]

I think the best user experience would be to make sure stdio becomes available as soon as possible.

100%. This PR is kind of a work-around for me. The fix would be to either to split board initialization:

RIOT/cpu/cortexm_common/vectors_cortexm.c

Lines 124 to 133 in b709e63

	board_init();
	#if MODULE_NEWLIB
	/* initialize std-c library (this must be done after board_init) */
	extern void __libc_init_array(void);
	__libc_init_array();
	#endif
	/* startup the kernel */
	kernel_init();

into two functions that get called before and after the libc init, or to make board init responsible for initializing the C library.

In any case it is not an easy task, that's why I'm posting this as a comment and not a review, I don't want to block a PR that would prevent a lockup.

[kaspar030]

I think the best user experience would be to make sure stdio becomes available as soon as possible.

+1

[MrKevinWeiss]

Can we agree that, yes this is a workaround to the initialization problem, however, it is still better to have a check before sending blindly?

I can be convinced out of it with the, it costs bytes and if someone sends without initializing first they are doing it wrong/not guaranteed, argument. Just keep in mind that RIOT has been doing that for a while now.

[maribu]

I think that performing that check only when DEVELHELP is enabled is a good compromise. In production using stdio this soon during boot up is unlikely to happen. But in development it is nice for debugging. It would also be helpful to get a hint that UART output was discarded during development. Otherwise people soon will start to open issue that RIOT something silently discards stdio output. And in a separate PR we could try to enable stdio sooner in the boot process. (I could have a look into that.)

[MrKevinWeiss]

@maribu agreed, done!

IMHO we should avoid introducing #ifdefs but rather should get rid of them ... anyway, won't block this

[maribu]

I'm personally in favor of not having any white space in front of preprocessor directives, because:

1. C and C preprocessor macros are two completely distinct languages and keeping the indent of the C code surrounding it cannot be applied consistently. See example below
2. Most C code does this. I'm in favor for keeping conventions, unless there is a reason for not doing so
3. The rest of this file does not use white space in front of preprocessor directives
4. Preprocessor directives become more visible, if the have a zero indent compared to the C code. And you really don't want to overlook them :-)

Here an example with unclear level of indent.

int foo(enum bar, int blah)
{
    switch(bar) {
        #ifdef MODULE_FOO /* <-- two levels of indent */
        case some_enum_value:
            some_function(blah);
            break;
            #endif /* <-- two or three levels of indent? */
    }
}

However, in the RIOT code base there are both files that do not use whitespace before preprocessor directives, and files that do use whitespace. So there is apparently no rule on it. So treat this comment as my personal opinion and not as a review.

[maribu]

You could also use #ifdef DEVELHELP here...

[MrKevinWeiss]

Ya, I should have checked that. I also made it just #if DEVELHELP, though I am not sure if that is preferred.

[jcarrano]

I'm personally in favor of not having any white space in front of preprocessor directives, because:

I'm in favour of avoiding preprocessor directives if possible:

/* somewhere define a macro that is always defined */
#ifdef DEVELHELP
    #define DEVELHELP_ON 1
#else
    #define DEVELHELP_ON 0
#endif /* DEVELHELP */

/* then in the code you use a "real" if */

void uart_write(uart_t uart, const uint8_t *data, size_t len)
{
    assert(uart < UART_NUMOF);

    if (DEVELHELP_ON) {
        /* If tx is not enabled don't try to send */
         if (!(dev(uart)->CR1 & USART_CR1_TE)) {
             return;
         }
    }

/* rest of the function */
}

Isn't that much more readable? Also, you get the benefit of having the compiler SEE what is inside the if block always (even if it then gets thrown away.)

[maribu]

I'm in favour of avoiding preprocessor directives if possible

Let me point out that your code contains 5 preprocessor directives, which are 3 more than the original code. (It also adds 5 lines of code.)

I personally think the original code was more readable.

[MrKevinWeiss]

I also agree with @maribu, and for the sake of uniformity. I a bit think we are getting into a "lets delay this bugfix so we can be nit picky" territory.
...On that note I was thinking if it is a good idea to have something that would work if DEVELHELP==1 and crash if DEVELHELP==0. Can we assume if DEVELHELP==0 the uart will always be initialized first?

[maribu]

Just to make sure I did understand you correct: By "DEVELHELP==1" you mean DEVELHELP is defined, and "DEVELHELP==0" means DEVELHELP is not defined, right?

The only reason for a valid (assert(uart < UART_NUMOF) did not trigger) not to be initialized seems to be it is used early in the boot up process. The only use case to me seems debug output via stdio. In production there should be no debug output of the early boot process. (E.g. sys/auto_init will run only after the periph buses and stdio is available, so there is not to much code running before stdio is available.)

So I believe it is safe to assume that when DEVELHELP is not defined that UARTs are not used before they are initialized.

[maribu]

I will also post a follow-up PR for this

And also set some global flag (only present with DEVELHELP) when this occurs. At the very end of the initialization of stdio a check for that flag could be added (again, only with DEVELHELP) that would print a warning about lost stdio output.

I'm not 100% sure this is required, but it could be helpful.

[jcarrano]

I a bit think we are getting into a "lets delay this bugfix so we can be nit picky" territory.

No, because I have never blocked this PR.

[MrKevinWeiss]

@maribu DEVELHELP==0 or 1 to me means make DEVELHELP=0 or make DEVELHELP=1, I thought that they were defined in both cases but maybe something in the make system undef if it is 0 or something.

That's fine for me to assume production code has the initialization sorted out.

Also thanks for taking over the follow up PR.

@jcarrano True, but the conversation is still ongoing and it seems like nobody wants to click that shiny button. I think sometime you can't make every developer happy though.

[maribu]

+1 on merging

@jcarrano, @smlng: You have expressed concerns previously. Are they addressed? If so, I would hit merge now.

[MrKevinWeiss]

@leandrolanzieri Thanks for merging, now I can update the comment in the other PR!