Fix panics for UART #434
Fix panics for UART #434
Conversation
- Default rx/tx_buffer_size in uart_driver_install - Make not covered ResetReason as Unknown
| esp_reset_reason_t_ESP_RST_BROWNOUT => Self::Brownout, | ||
| esp_reset_reason_t_ESP_RST_TASK_WDT => Self::TaskWatchdog, | ||
| esp_reset_reason_t_ESP_RST_DEEPSLEEP => Self::DeepSleep, | ||
| _ => unreachable!(), | ||
| _ => Self::Unknown, |
There was a problem hiding this comment.
I don't mind altering it so that Self::Unknown becomes a catch all.
But: you are not fixing the root cause why it panicked in the first place, isn't it?
- It didn't panic because of
esp_reset_reason_t_ESP_RST_UNKNOWNbecause we have that branch - It panicked because of something else
So... how about we map this something else (whatever it is) in the match statement? I mean in addition to the change you introduce here?
There was a problem hiding this comment.
OK. I'll look at the reset reason code and add it to the enum. @ivmarkov Did you mean this?
| } else { | ||
| 0 | ||
| }, | ||
| UART_FIFO_SIZE * 2 |
There was a problem hiding this comment.
However.... why do we need this? Are you having problems with TX as well, or are you doing it "just in case"? IMO this is not necessary, because the TX assert is slightly different: https://github.com/espressif/esp-idf/blob/a322e6bdad4b6675d4597fb2722eea2851ba88cb/components/driver/uart/uart.c#L1589
There was a problem hiding this comment.
Agree. Cancel changes
Default rx/tx_buffer_size in uart_driver_install
This is necessary because esp-idf requires a non-zero buffer size for the UART (see this issue)
Make not covered ResetReason as Unknown
I believe that this applies to the UART, since for me the only ResetReason that is not covered is a reset via the UART (for example, by pressing Ctrl+R when connected via espflash). I don’t see a problem making this reason as unknown. As for me, panicking when trying to get ResetReason is not the best idea