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.

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

High-level stdio is not beginner friendly when they use asserts #20626

Closed
chrysn opened this issue Apr 26, 2024 · 2 comments · Fixed by #20627
Closed

High-level stdio is not beginner friendly when they use asserts #20626

chrysn opened this issue Apr 26, 2024 · 2 comments · Fixed by #20627

Comments

@chrysn
Copy link
Member

chrysn commented Apr 26, 2024

When novice users use boards with high-level IO (eg. nrf52840-feather), they don't get good crash feedback -- system is just gone. From @MrKevinWeiss's experience, many occurrences of this happens with asserts failing. (Other than that, RIOT is rather stable, it doesn't "just" run into a hard fault).

So what do?

  1. Use low level stdio? No thanks. It's not the 1990s any more, RS232 is not a prevalent connection any more, and why just add extra hardware onto devices just to compensate for software shortcomings?

  2. We could improve stdio to stay usable in crash situations. There is a viable path -- high-level stdio usually have some kind of buffer, flags that to not be cleared out at startup, and coordinates that with the bootloader. The latter may be tricky especially in the nrf52840-feather case because that uses the nrfutil-adafruit bootloader which we don't control.

  3. Change how an assert crashes. Compare to Rust: If a Rust panic happens in an ISR or while interrupts are off, it calls core_panic which halts or reboots the system. But in the common case of thread mode, it merely prints the error message and the sends the thread into an endless loop of thread_sleep. Unlike a full crash, this has a good chance of leaving the high-level stdio usable. (Sure, a crash in a CoAP handler will not be usable by a CoAP based stdio, but well with a USB based and vice versa).

    Doing this for asserts won't break deployments because the design intention with them is to be only used during development; we could even consider taking it further by looking at other places that call core_panic to see whether they'd also be eligible (i.e., whether the corruption they correspond to is sure to be limited to the thread they are in), but let's focus on asserts first.

I suggest we go with 3., while leaving 2 as an option for further enhancement.

CC'ing @MrKevinWeiss and @bergzand who supported this during a chat, and @mguetschow who was also there but I don't remember if he supported it :-)

@benpicco
Copy link
Contributor

I'm trying to reproduce this on saml21-xpro, with assert(0) added to the text command in tests/sys/usbus_cdc_acm_stdio I get

2024-04-26 13:51:52,583 # tests/sys/usbus_cdc_acm_stdio/main.c:45 => *** RIOT kernel panic:
2024-04-26 13:51:52,585 # FAILED ASSERTION.
2024-04-26 13:51:52,585 # 
2024-04-26 13:51:52,590 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2024-04-26 13:51:52,596 # 	  - | isr_stack            | -        - |   - |    512 (  192) (  320) | 0x20000000 | 0x200001c0
2024-04-26 13:51:52,599 # 	  1 | main                 | running  Q |   7 |   1536 (  808) (  728) | 0x20000834 | 0x20000b0c 
2024-04-26 13:51:52,603 # 	  2 | usbus                | bl anyfl _ |   1 |   1024 (  588) (  436) | 0x20000434 | 0x200006b4 
2024-04-26 13:51:52,605 # 	    | SUM                  |            |     |   3072 ( 1588) ( 1484)
2024-04-26 13:51:52,605 # 
2024-04-26 13:51:52,606 # *** halted.

@chrysn
Copy link
Member Author

chrysn commented Apr 26, 2024

Indeed, my test with particle-xenon (where usbus based stdio is default) confirm that. (And I admit to not having tested this after the video call discussion). @MrKevinWeiss, was there a particular situation in which this did not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants