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.

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

missing async uart.h system calls #21431

Closed
andrewboie opened this issue Dec 16, 2019 · 7 comments · Fixed by #21878
Closed

missing async uart.h system calls #21431

andrewboie opened this issue Dec 16, 2019 · 7 comments · Fixed by #21878
Assignees
Labels
area: Memory Protection area: UART Universal Asynchronous Receiver-Transmitter bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@andrewboie
Copy link
Contributor

andrewboie commented Dec 16, 2019

Describe the bug
The set of system calls for our serial API subsystem is incomplete. We are missing system calls for the following APIs, all of which are related to CONFIG_UART_ASYNC_API:

uart_tx()
uart_tx_abort()
uart_rx_enable()
uart_rx_disable()

uart_callback_set() installs a callback handler and shouldn't have a system call. uart_tx_buf_rsp() is only intended to be invoked from callbacks. The rest we should add unless we can specify a reason not to.

Impact
User mode threads can't use these APIs.

@andrewboie andrewboie added bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug labels Dec 16, 2019
@andrewboie
Copy link
Contributor Author

andrewboie commented Dec 16, 2019

Original PR discussion for async API is here:

#10820

There's a lot of discussion in here, but from skimming over all of it appears that the async UART APIs are intended for supervisor mode only, and these APIs are intended to be used by other drivers that are built on top of the UART. And that application code wouldn't use these APIs. @nordic-krch @pfalcon @Mierunski @carlescufi do I have that right?

@andrewboie andrewboie added question and removed bug The issue is a bug, or the PR is fixing a bug labels Dec 16, 2019
@andrewboie andrewboie changed the title missing async uart.h system calls missing async uart.h system calls -- intentional? Dec 16, 2019
@andrewboie andrewboie removed the priority: medium Medium impact/importance bug label Dec 16, 2019
@carlescufi
Copy link
Member

I don't think that's accurate

There's a lot of discussion in here, but from skimming over all of it appears that the async UART APIs are intended for supervisor mode only, and these APIs are intended to be used by other drivers that are built on top of the UART. And that application code wouldn't use these APIs. @nordic-krch @pfalcon @Mierunski @carlescufi do I have that right?

I am really not sure that is the case. I think those are actually also intended for application use. @nordic-krch and @Mierunski can you please confirm?

@nordic-krch
Copy link
Contributor

We had that discussion already.
See #10820 (comment) and #10820 (comment)

It's not intended to be used in userspace. Tty api is more suitable.

@andrewboie
Copy link
Contributor Author

We had that discussion already.
See #10820 (comment) and #10820 (comment)

Yeah, this discussion isn't very clear. I saw these comments.

We have lots of driver subsystems which have both system calls and callback registration. We don't allow callback registration from user mode, so the callback APIs are left supervisor only. But the rest of the APIs are exposed as system calls. The programming model is to set up callbacks at application init, before dropping privileges to user mode.

What's special about this set of async APIs that we cannot create system calls for everything except uart_callback_set()? What am I missing here?

It's not intended to be used in userspace. Tty api is more suitable.

That wasn't my question -- I was asking if this is an application API or only intended for use by drivers.

@Mierunski
Copy link
Collaborator

From #10820 by @pfalcon

To give additional input to your analysis, consider that Zephyr supports userspace/kernel split. You very well know that because you have __syscall in your patch. Something people keep missing is that callbacks from kernel to userspace is no-go for this mode.
So, even at the very start of your analysis you would have following conjecture:
This PR proposes API which is suitable only for kernel mode.

This was main argument for removing system calls from API (they were there at start of PR). There wasn't much interest in keeping system calls from other participants.
For me it wouldn't hurt to add system calls. If it makes sense to have API with callbacks and system calls. Only uart_rx_buf_rsp() is a function intended to be called from callback context.

That wasn't my question -- I was asking if this is an application API or only intended for use by drivers.

In my opinion this could also be application API, as (for me) it is easier to use than interrupt driven API.

@andrewboie
Copy link
Contributor Author

OK I think you guys got some bad advice, we can add syscalls for this API set and just omit it for the callback registration function. Callbacks get set up by supervisor threads just like other driver APIs.

@andrewboie andrewboie added bug The issue is a bug, or the PR is fixing a bug and removed question labels Dec 23, 2019
@andrewboie andrewboie changed the title missing async uart.h system calls -- intentional? missing async uart.h system calls Dec 23, 2019
@jhedberg jhedberg added the priority: medium Medium impact/importance bug label Jan 2, 2020
@andrewboie andrewboie added the area: UART Universal Asynchronous Receiver-Transmitter label Jan 13, 2020
@andrewboie
Copy link
Contributor Author

andrewboie commented Jan 13, 2020

I'm looking at the serial driver system calls today. Adding the system calls themselves does not seem too hard.

We have a small problem: async UART APIs are only implemented for two serial drivers: sam0 and nrfx. There are currently no platforms that are user-mode capable that also have the async UART APIs implemented in their serial driver, so I can't test whether this actually works even though we have a test case in tests/drivers/uart/uart_async_api

[EDIT] I got confused, we have several boards we can test with. I could use help from someone at Nordic to test #21878

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Memory Protection area: UART Universal Asynchronous Receiver-Transmitter bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants