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.

Sign up for GitHub

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

Jump to bottom

drivers: uart: native tty: Split native tty in top and bottom #60270

Merged
merged 2 commits into from
Sep 13, 2023
Merged

drivers: uart: native tty: Split native tty in top and bottom #60270

merged 2 commits into from
Sep 13, 2023

Conversation

MarkoSagadin
Copy link
Contributor

@MarkoSagadin MarkoSagadin commented Jul 11, 2023 •
edited
Loading

Split the native tty serial driver in a top and bottom to enable using it with embedded libCs when building targeting the native_sim boards.

This was the functionally same change as in #60053.

I also added native_sim over in the native_tty sample and included it into twister tests.

Related: #60096

@zephyrbot zephyrbot added area: native port Host native arch port (native_sim) area: UART Universal Asynchronous Receiver-Transmitter labels Jul 11, 2023
dcpleung
dcpleung previously approved these changes Jul 11, 2023
View reviewed changes
@MarkoSagadin MarkoSagadin changed the title Split native tty in top and bottom drivers: uart: native tty: Split native tty in top and bottom Jul 12, 2023
@carlescufi carlescufi assigned aescolar and unassigned dcpleung Jul 12, 2023
@carlescufi
Copy link
Member

Assigned this to @aescolar. He's back next week.

@aescolar aescolar mentioned this pull request Jul 18, 2023
Open
13 tasks
aescolar
aescolar requested changes Jul 18, 2023
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Thank you very much @MarkoSagadin ,
A couple of things need to be fixed though.

samples/drivers/uart/native_tty/boards/native_sim.overlay Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
MarkoSagadin
MarkoSagadin commented Aug 4, 2023
View reviewed changes
Copy link
Contributor Author

@MarkoSagadin MarkoSagadin left a comment

Choose a reason for hiding this comment

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

I apologize that it took me so much time to come back to this, I was busy. I have rebased my changes to the current main and resolved requested changes. Please have a look when you can.

drivers/serial/uart_native_tty.c Show resolved Hide resolved
samples/drivers/uart/native_tty/boards/native_sim.overlay Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty.c Outdated Show resolved Hide resolved
@MarkoSagadin
Copy link
Contributor Author

native_posix build was failing for the native_tty sample due to a warning. I fixed it and force-pushed.

aescolar
aescolar previously requested changes Aug 7, 2023
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @MarkoSagadin
Just a couple of minor things left.
Did you try it with the sample and two back to back serial dongles?

drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.h Outdated Show resolved Hide resolved
MarkoSagadin
MarkoSagadin commented Aug 7, 2023
View reviewed changes
Copy link
Contributor Author

@MarkoSagadin MarkoSagadin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@aescolar I have fixed the mistakes. I currently could not test the sample as I do not have the dongles with me (I am on vacation). If is okay with you this PR can stay open until I return back (28th of august) to test this.

drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
drivers/serial/uart_native_tty_bottom.c Outdated Show resolved Hide resolved
@MarkoSagadin
drivers: serial: native tty: Split in top and bottom
cc2376c 
Split the native tty serial driver in a top and bottom to enable using it
with embedded libCs.

Signed-off-by: Marko Sagadin <marko.sagadin42@gmail.com>
@MarkoSagadin
samples: drivers: uart: native tty: Add native_sim overlay
d934456 
Also include it in the twister tests.

Signed-off-by: Marko Sagadin <marko.sagadin42@gmail.com>
@aescolar aescolar dismissed their stale review August 8, 2023 07:30

Addressed

@aescolar
Copy link
Member

aescolar commented Aug 8, 2023

Thanks @MarkoSagadin

@aescolar I have fixed the mistakes. I currently could not test the sample as I do not have the dongles with me (I am on vacation). If is okay with you this PR can stay open until I return back (28th of august) to test this.

It is more than ok, I thank you in advance for testing it :). I will leave it flagged as "dnm" until you (or somebody else with suitable HW) test it.

@aescolar aescolar added the DNM This PR should not be merged (Do Not Merge) label Aug 8, 2023
aescolar
aescolar previously approved these changes Aug 8, 2023
View reviewed changes
@MarkoSagadin
Copy link
Contributor Author

Hello @aescolar, I have built the samples/drivers/uart/native_tty sample for both the native_posix and native_sim boards and the output is correct, messages are correctly sent and received. You can merge this. I have rebased against current main and force-pushed.

@aescolar aescolar removed the DNM This PR should not be merged (Do Not Merge) label Aug 29, 2023
@aescolar
Copy link
Member

Thanks @MarkoSagadin
@dcpleung your reaproval would be welcome, so we can merge it.

@carlescufi carlescufi merged commit 5f0bb1d into zephyrproject-rtos:main Sep 13, 2023
37 of 39 checks passed
@MarkoSagadin MarkoSagadin deleted the native_tty_bottom branch September 14, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aescolar aescolar aescolar approved these changes

@carlescufi carlescufi carlescufi approved these changes

@nashif nashif Awaiting requested review from nashif

@dcpleung dcpleung Awaiting requested review from dcpleung

Assignees

@aescolar aescolar

Labels
area: native port Host native arch port (native_sim) area: UART Universal Asynchronous Receiver-Transmitter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MarkoSagadin @carlescufi @aescolar @dcpleung @zephyrbot