Multiple API fixes in UART driver #2851
Conversation
esp-hal/src/uart.rs
Outdated
| @@ -528,25 +531,14 @@ pub struct AtCmdConfig { | |||
| pub char_num: Option<u8>, | |||
There was a problem hiding this comment.
I wonder if we should make this Option<core::num::NonZeroU8> - Some(0) doesn't make sense and if None we use 1
Would save us two bytes - not sure if it's worth it - makes things a bit less nice in user code
There was a problem hiding this comment.
This can also just be u8, can't it?
There was a problem hiding this comment.
(probably needs some explanation in the doc-comment)
There was a problem hiding this comment.
Then we need to validate AtCmdConfig if char_num is not 0 which is related to #2839 and probably should not be done in this PR (?)
There was a problem hiding this comment.
0 might be a perfectly reasonable value of "I don't want to detect AT commands"
There was a problem hiding this comment.
0 just means the same as currently None - shouldn't change anything?
BUT I just realized that some chips (e.g. ESP32 at least) offer more than 16 bits for the counts and the time-out.
Big question before stabilization would be if we are fine with artificially limiting this or not
There was a problem hiding this comment.
oh and I think it solves a problem: currently we cannot undo the at-cmd-config once set? After the change the user could set char_num to 0 and effectivly disable it
esp-hal/src/uart.rs
Outdated
| @@ -1582,7 +1575,15 @@ where | |||
| } | |||
|
|
|||
| fn flush(&mut self) -> Result<(), Self::Error> { | |||
| self.tx.flush() | |||
| loop { | |||
There was a problem hiding this comment.
embedded_io::Write::flush(&mut self.tx) would safe us from duplicating this code
playfulFence
force-pushed
the
upd/wbeta_uart
branch
from
b702b8a to
a0c4772
Compare
playfulFence
force-pushed
the
upd/wbeta_uart
branch
from
a0c4772 to
9f6a400
Compare
closes #2733
closes #2726
closes #2724
closes #2636 (public methods only, post your thoughts here if you want private functions to follow these conventions too)