Skip to content

Overhaul UART to support single wire transmitters/receivers#164

Merged
ivmarkov merged 9 commits into
esp-rs:masterfrom
zRedShift:uart
Nov 19, 2022
Merged

Overhaul UART to support single wire transmitters/receivers#164
ivmarkov merged 9 commits into
esp-rs:masterfrom
zRedShift:uart

Conversation

@zRedShift

Copy link
Copy Markdown
Collaborator

No description provided.

@zRedShift zRedShift changed the title overhaul UART to support single wire trasimitters/receivers Overhaul UART to support single wire trasimitters/receivers Nov 14, 2022
Comment thread src/uart.rs
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated
Comment thread src/uart.rs Outdated

@ivmarkov ivmarkov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just one key question. The rest looks great.

Comment thread src/uart.rs
pub struct UartDriver<'d>(Port<'d, Owned>);
pub struct UartDriver<'d> {
port: u8,
_p: PhantomData<&'d mut ()>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't we need an Owner here as well? My rust knowledge falls short here: will drop still be called on UartDriver if I do into_split(self)?

  • if yes, we need Owner here
  • if not, we are good to go

@ivmarkov ivmarkov Nov 18, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think drop WILL be called, and the only reason Rust allows us to implement into_split in the presence of drop is because - in fact - we are not moving anything out of UartDriver? :) Everything that we push into RX/TX is Copy...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ivmarkov absolutely right, I stepped into this rake again.
pushed something to fix it, probably better than having an owner enum, what do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep, LGTM. Shall I merge or will you bang on it a bit more? How do you feel about it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'll probably look over it some more tomorrow, as I did this in quite a hurry today.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK. Will wait for a green-light from you.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ivmarkov yeah so I went over it and found some issues.
I accidentally removed the count function from UartRxDriver, which I promptly restored. Then I noticed that uart_get_buffered_data_len and uart_get_tx_buffer_free_size can return 256 (SOC_UART_FIFO_LEN * 2), when the rx buffer is completely filled up, or the tx buffer is empty, which won't fit in u8. So the solution is either to reduce the buffers to length of 255, or to increase the return word width. In the future I imagine we'll make the tx/rx ring buffer size user tweakable, so it's better to future proof than restrict, IMHO.
I also updated the example to not require mut, and removed pub from Owner, as it's an internal implementation detail.
And lastly, I made a very much useless "optimization" (I'm pretty sure the compiler should be able to infer this by itself), whereas the internal tx()/rx() return a ManuallyDrop instead, which won't run the destructor (which obviously does nothing). This is the change I feel the most weakly about.

… to be able to fit the maximum allowed value, don't run `drop` on temporary internal borrows, update example
@zRedShift zRedShift changed the title Overhaul UART to support single wire trasimitters/receivers Overhaul UART to support single wire transmitters/receivers Nov 19, 2022
@ivmarkov
ivmarkov merged commit 3f532d7 into esp-rs:master Nov 19, 2022
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 this pull request may close these issues.

2 participants