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

[BUG] Zephyr UART transport does not works with CDC ACM transport #409

Closed
2 tasks done
gabbla opened this issue Feb 22, 2024 · 4 comments
Closed
2 tasks done

[BUG] Zephyr UART transport does not works with CDC ACM transport #409

gabbla opened this issue Feb 22, 2024 · 4 comments
Labels

Comments

@gabbla
Copy link
Contributor

gabbla commented Feb 22, 2024

Describe the bug

Zephyr RTOS provides a transparent API to interact with physical UARTs and USB CDC ACM ones, so in theory one could create an UartTransport with either one.

Using a physical UART with the mentioned transport works fine, however, when switching to a CDC ACM one, the server always returns 16 (kErpcStatus_ReceiveFailed) after receiving an RPC call.
The issue resides in the UartTransport::underlyingReceive function: the logic handles the case in which the uart_receive_buf is not yet filled with the needed bytes, so it waits on a semaphore and only after the get the erpcStatus is marked as success. However, when the uart_receive_buf already has the needed bytes, the erpcStatus is not updated and the read wrongfully fails.

erpc_status_t UartTransport::underlyingReceive(uint8_t *data, uint32_t size)
{
erpc_status_t erpcStatus = kErpcStatus_ReceiveFailed;
if (ring_buf_size_get(&uart_receive_buf) < size)
{
s_transferReceiveRequireBytes = size;
/* wait until the receiving is finished */
#if !ERPC_THREADS_IS(NONE)
(void)m_rxSemaphore.get();
#else
s_isTransferReceiveCompleted = false;
while (!s_isTransferReceiveCompleted)
{
}
#endif
erpcStatus = kErpcStatus_Success;
}
/* read data from buffer */
if (ring_buf_get(&uart_receive_buf, data, size) != size)
{
/* reading error, should not happen */
erpcStatus = kErpcStatus_ReceiveFailed;
}
return erpcStatus;
}

I believe this may fail in other context even with physical UARTs, but I was not able to validate the such statement.

I'm about to open a PR with a proposed solution.

To Reproduce

Simply create an erpc::UartTransport using an USB CDC ACM node instead of a physical UART.

Expected behavior

The UartTransport should works with any kind of UART.

Desktop

The test were conducted with eRPC Version v1.12.0 using the python generated code for the client side and Zephyr v3.6.0-rc2 running on a NUCLEO-H563ZI.

Steps you didn't forgot to do

  • I checked if there is no related issue opened/closed.
  • I checked that there doesn't exist opened PR which is solving this issue.
@gabbla gabbla added the bug label Feb 22, 2024
Copy link

Hi eRPC user. Thank you for your interest and welcome. We hope you will enjoy this framework well.

@Hadatko
Copy link
Member

Hadatko commented Feb 22, 2024

Hi @gabbla , thank you for your inputs. I don't have skills with Zephyr, but i think your requested changes are not good. If i understand you correctly you mean that buffer in second case contains header+body. In first case header and body comes as two messages correct?
If i am correct for CDC you would need similar implementation that we have for erpc_rpmsg_tty_rtos_transport.cpp. That implementation count that it can receive two messages one header one body, or it can receive all at once.
In current implementation the header buffer has strict length and you cannot fill it with more data. But if you override another function instead, you will be able to get all data at once.

@gabbla
Copy link
Contributor Author

gabbla commented Feb 22, 2024

Hi @Hadatko, thank you for the quick replay. Yes, while using CDC the buffer is filled immediately with header + body, meanwhile using the UART the header and body came byte by byte (on my platform). Platforms with bigger UART FIFO (in hardware) may rise an interrupt when they have more than a single byte ready to be read (similar to what is happening with CDC).

My change should cover all cases, because if the condition

if (ring_buf_size_get(&uart_receive_buf) < size)

is false, that means we already have all the bytes we need, otherwise we need to wait. So it should not matter if we waited the semaphore or we already had all the bytes, erpcStatus should be kErpcStatus_Success in both cases. Or am I missing come cases?

I do not understand what do you mean with

In current implementation the header buffer has strict length and you cannot fill it with more data. But if you override another function instead, you will be able to get all data at once.

Can you please elaborate?

Gab

JanKomarekNXP added a commit that referenced this issue Mar 4, 2024
transport: fix zephyr uart transport. See #409
@JanKomarekNXP
Copy link
Contributor

Hi @gabbla, thank you for your contribution. Upon reviewing the bug, I think that the pull request from @gabbla is fine. In the interrupt, I am using the uart_fifo_read() to load everything in the uart_receive_buf buffer during the interupt. Enough data may be already loaded before calling UartTransport::underlyingReceive, which may prevent erpcStatus from changing to kErpcStatus_Success. I have already marged the PR to the develop branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants