Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Dec 19, 2018
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cpu/stm32_common/periph/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,12 @@ static inline void wait_for_tx_complete(uart_t uart)
void uart_write(uart_t uart, const uint8_t *data, size_t len)
{
assert(uart < UART_NUMOF);

#if DEVELHELP
/* If tx is not enabled don't try to send */
if (!(dev(uart)->CR1 & USART_CR1_TE)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@maribu maribu Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
#endif
#ifdef MODULE_PERIPH_DMA
if (!len) {
return;
Expand Down