feat: uart configurable `tx_idle_num` #2859

zpg6 · 2024-12-21T18:35:36Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Added tx_idle_num to the UART configuration. By default, this is 256 bits - and results in bytes waiting in the TX FIFO before being transmitted. For latency-sensitive operations that causes chaotic debugging of timing issues, so I added documentation of the behavior (set 0 to send immediately).

Testing

Ongoing

bugadani

I know this is a draft, but there are a few things that need clarification.

As UART is one of the stable candidate drivers, we will have to discuss whether this is how we want to support this feature. It would help if you could please state why this setting needs to be in the HAL, what potential use cases exist, and if you could document it in such a way that conforms to our and the Rust guidelines.

What does it exactly mean that the duration is based on the baud rate? Is incrementing this delay by one means 1 bit, one symbol, or some other time increment?

We'll also need to explore edge cases. Do you know what happens if the user configures this setting while the UART is in sending data?

bugadani · 2024-12-21T19:04:49Z

esp-hal/src/uart.rs

@@ -507,6 +526,7 @@ impl Default for Config {
            clock_source: Default::default(),
            rx_fifo_full_threshold: UART_FULL_THRESH_DEFAULT,
            rx_timeout: Some(UART_TOUT_THRESH_DEFAULT),
+            tx_idle_num: 256,


Why not default to 0?

256 is the default shown in the technical specification.

esp-hal/src/uart.rs

bugadani · 2024-12-21T19:05:54Z

esp-hal/src/uart.rs

@@ -2628,6 +2649,17 @@ impl Info {
            .modify(|_, w| unsafe { w.stop_bit_num().bits(stop_bits as u8) });
    }

+    fn change_tx_idle(&self, idle_num: u16) {
+        // Bits 10:19 => 10-bit register has max value of 1023.


Is this true for all devices?

Please see our API_GUIDELINES, and return an error instead of panicking.

Still need help to determine beyond than just esp32.

For error vs panic: fixed.

zpg6 · 2024-12-21T19:21:31Z

Sure, my main intention is to expose a setting that tripped me up for hours. I expected it to default to 0 and send UART bytes right out when you put them in TX FIFO.

I suspect the intention of the non-zero default is to group transmissions together and save interrupts on the receiving device.

But for time-sensitive operations it leaves pauses and the only mention of this grouping feature is one vague description in the technical specification's UART Registers summary.

zpg6 · 2024-12-21T19:23:59Z

I'm not positive of the unit for idle_num, I assume it is bits but I haven't measured it yet. It is not mentioned in the technical specification.

bugadani · 2024-12-21T19:26:34Z

Are you saying that by default we are waiting for ~25 full frames between transmitting each frame? Can we just set this to 0 when we create the driver, and let the user use the PAC to insert delays if they need them?

Ah no it's not that bad, it's a delay after the FIFO has been emptied. Hmm...

in the unit of bit time, i.e. the time it takes to transfer one bit

zpg6 · 2024-12-21T19:34:12Z

it can mean a few ms for me after a break:

With default 256:

With 0:

zpg6 · 2024-12-21T20:08:51Z

As UART is one of the stable candidate drivers, we will have to discuss whether this is how we want to support this feature. It would help if you could please state why this setting needs to be in the HAL, what potential use cases exist, and if you could document it in such a way that conforms to our and the Rust guidelines.

I think this default behavior is irregular and confusing enough to warrant a change. Either default to 0 and require other values be set with the PAC, or allow this configuration value that is quite visible to the user that way.

We'll also need to explore edge cases. Do you know what happens if the user configures this setting while the UART is in sending data?

@bugadani - how to test this? Help me with some pseudocode if you can

zpg6 · 2024-12-21T20:17:28Z

By the way, toggling a random IO pin on a secondary oscilloscope probe has been helpful for seeing when you try to write versus when the bytes are actually written.

How to replicate the delay issue:

Write some bytes
Delay a tiny bit, long enough for it to empty FIFO but not longer than 256 bits time
Write more bytes

You'll see it'll space your transmissions out and not preserve the timing of your instructions.

bugadani · 2024-12-21T20:35:51Z

Help me with some pseudocode if you can

You yourself gave something good :)

Write some bytes
Delay a tiny bit, long enough for it to empty FIFO but not longer than 256 bits time
~~Write more bytes~~ Reconfigure with a shorter/longer idle timeout, and see what happens.

Honestly, I think it would be a lot simpler to just zero out this register field, and not bother with making it configurable through the HAL. If we have a sensible default that isn't surprising, we are in a good spot. If this change would solve your problem, then I think we can leave figuring out how to introduce actual configurability to when we have a strong(er) need to tackle that part of the issue.

But we'll have to wait for the others to voice their opinions as well, and everyone is on vacation :)

feat: uart configurable tx_idle_num

5445771

bugadani requested changes Dec 21, 2024

View reviewed changes

zpg6 added 3 commits December 21, 2024 14:45

fix: remove extraneous setter

67e8d73

fix: use ConfigError instead of assert!; clarify unit of bit time

3205778

fix: remove unneeded assert

e064484

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: uart configurable `tx_idle_num` #2859

feat: uart configurable `tx_idle_num` #2859

zpg6 commented Dec 21, 2024

bugadani left a comment

bugadani Dec 21, 2024

zpg6 Dec 21, 2024

bugadani Dec 21, 2024

zpg6 Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024

zpg6 commented Dec 21, 2024

bugadani commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading

bugadani commented Dec 21, 2024

feat: uart configurable tx_idle_num #2859

Are you sure you want to change the base?

feat: uart configurable tx_idle_num #2859

Conversation

zpg6 commented Dec 21, 2024

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

bugadani left a comment

Choose a reason for hiding this comment

bugadani Dec 21, 2024

Choose a reason for hiding this comment

zpg6 Dec 21, 2024

Choose a reason for hiding this comment

bugadani Dec 21, 2024

Choose a reason for hiding this comment

zpg6 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

zpg6 commented Dec 21, 2024

zpg6 commented Dec 21, 2024

bugadani commented Dec 21, 2024 • edited Loading

zpg6 commented Dec 21, 2024 • edited Loading

With default 256:

With 0:

zpg6 commented Dec 21, 2024 • edited Loading

zpg6 commented Dec 21, 2024 • edited Loading

bugadani commented Dec 21, 2024

feat: uart configurable `tx_idle_num` #2859

feat: uart configurable `tx_idle_num` #2859

zpg6 Dec 21, 2024 •

edited

Loading

bugadani commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading

zpg6 commented Dec 21, 2024 •

edited

Loading