-
Notifications
You must be signed in to change notification settings - Fork 7.3k
serial: uart_native_pty: ASYNC API support #87874
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
Conversation
d408832
to
95f5aab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is not an approach we can use, due to several issues, of which the main are:
- A signal is delivered fully asynchronously to the native_simulator execution
- The signal handler will by default be run by any native simulator thread
That means between other things that the signal handler will be run, for ex., while the CPU is sleeping corrupting the kernel state; for AMP targets, from any other CPU thread, causing mayhem in both CPUs kernels state, and other messy situations.
In general in native_sim we do not have really asynchronous behaviors. What we do is that we have a deterministic synchronous one that pretends to inject an asynchronous like event at a very deterministic time (see for example the TTY UART interrupt driven mechanism).
95f5aab
to
2dda4d3
Compare
Thanks for the feedback, I have reworked the PR in line with the TTY driver. |
8e9e255
to
a7ca231
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JordanYates
What I see now is much more minor.
And probably people who is more familiar with the UART API is better than me to provide good input for what remains (CC @dcpleung @nordic-krch )
|
||
static int np_uart_tx(const struct device *dev, const uint8_t *buf, size_t len, int32_t timeout) | ||
{ | ||
struct native_pty_status *data = dev->data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this check if the callback is set and return -ENOTSUP otherwise?
(Same for rx and np_uart_tx_abort)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't done in the other drivers that I have inspected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I based my comment on the API header comments. I leave it up to the driver class owner @dcpleung to decide if anything needs doing
Add a length parameter to the poll in read function so that data can be read in larger chunks. Signed-off-by: Jordan Yates <[email protected]>
3bbfee8
to
3f3d17f
Compare
@aescolar thanks for the feedback, halfway through the implementation I switched from a persistent thread to one that only runs while RX is enabled and missed some things. |
3f3d17f
to
0614be0
Compare
Add support for transmitting using the asynchronous API. The asynchronous portion is simulated through the system workqueue. Signed-off-by: Jordan Yates <[email protected]>
Add support for transmitting using the asynchronous API. The asynchronous portion is simulated through a dedicated polling thread. Signed-off-by: Jordan Yates <[email protected]>
Add a sample that utilises the ASYNC API to queue packets in bursts. Signed-off-by: Jordan Yates <[email protected]>
0614be0
to
e06e72e
Compare
data->async.tx_buf = NULL; | ||
|
||
if (data->async.user_callback) { | ||
data->async.user_callback(data->async.dev, &evt, data->async.user_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not critical for native but normally user callback should not be called with interrupts locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal here was to prevent the system workqueue item being preempted by some other thread while this function was running, since in a "normal" driver this logic would be running from an interrupt context. Happy to use a different solution if you have one.
} | ||
|
||
/* Generate TX_DONE event with number of bytes transmitted */ | ||
evt.type = UART_TX_DONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UART_TX_ABORTED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UART documentation contradicts itself on which event should be generated:
zephyr/include/zephyr/drivers/uart.h
Lines 195 to 196 in 6463c68
* Transmitting can be aborted using @ref uart_tx_abort, after calling that | |
* function #UART_TX_ABORTED event will be generated. |
zephyr/include/zephyr/drivers/uart.h
Line 800 in 6463c68
* #UART_TX_DONE event will be generated with amount of data sent. |
ARG_UNUSED(arg2); | ||
ARG_UNUSED(arg3); | ||
|
||
while (data->async.rx_len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the expected receiver behavior is that RX_RDY is reported when buffer is filled or when timeout occurs (and no new data arrives). When buffer is filled and new buffer is not provided then RX_BUF_RELEASED followed by RX_DISABLED events will be generated.
case UART_RX_BUF_REQUEST: | ||
/* Return the next buffer index */ | ||
LOG_DBG("Providing buffer index %d", async_rx_buffer_idx); | ||
rc = uart_rx_buf_rsp(dev, async_rx_buffer[async_rx_buffer_idx], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will assert with current implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't, because the native driver never issues UART_RX_BUF_REQUEST
, since it has no need for multiple buffers.
There could be an argument to be made to simulate the ping-pong buffers though to more closely behave like a real hardware device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't, because the native driver never issues UART_RX_BUF_REQUEST, since it has no need for multiple buffers.
But this is a generic sample for all UART drivers supporting the ASYNC API, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was in reference to what I thought @nordic-krch was referring to, which was that uart_rx_buf_rsp
would return -ENOTSUP
for the native driver, and thus trigger the assertion. If that is not the case I would need clarification on the "will assert" comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+2 for the native/host integration side
+1 for driver API/behaviour side
Add support for the ASYNC API to the native PTY uart driver.
Asynchronous transmission is performed from the system workqueue.
Asynchronous reception is performed from a dedicated thread.
I would love to add testing for testing this to
tests/drivers/uart/uart_async_api
, but it depends on loopback support.As far as I can tell the basic polling API is not tested either due to the lack of this support. Does anyone have ideas for enabling this in CI?