Update UART driver with specs, documentaion, and read/2 #1508
Conversation
UncleGrumpy
force-pushed
the
uart_add_timeout
branch
from
1cd4e1b to
54b5195
Compare
|
I pushed changes that eliminate the use of any timer or task in the C code, and instead added a I believe this is much more efficient, and less resource intensive than creating a timer. |
libs/eavmlib/src/uart.erl
Outdated
| case port:call(Pid, read, Timeout) of | ||
| {error, timeout} -> | ||
| port:call(Pid, cancel_read), | ||
| timeout; |
There was a problem hiding this comment.
I think you mean {error, timeout} here.
There was a problem hiding this comment.
I was not convinced that the return should be {error, timeout} for read/2, since timeout is an expected result... we either want data or a timeout. I am not opposed to making this an error, but it seemed a bit wrong to me.
There was a problem hiding this comment.
true, I prefer the error tuple though.
having a single atom returns leads to more defensive code needed imho.
eg. there is an additional return type, that I need to handle.
And it might even catch me out by surprise, "shipped to prod" etc.
case :uart.read(port, 0) do
{:ok, read} ->
#
{:error, error} ->
#
other ->
#
endThere was a problem hiding this comment.
Sounds good to me, just looking for the return "of least surprise". I will update the return and documentation.
It does make the most sense to have read/1 and read/2 have the same returns.
This also lets me make the return specifically a tuple in the spec, to make for more accurate use of dialyzer.
There was a problem hiding this comment.
I was also reacting to the spec that doesn't allow timeout.
There was a problem hiding this comment.
Clearly I was on the fence about the proper return there. lol. It's fixed now.
| { | ||
| struct UARTData *uart_data = ctx->platform_data; | ||
|
|
||
| uart_data->reader_process_pid = term_invalid_term(); |
There was a problem hiding this comment.
uart_interrupt_callback can run on the other scheduler on dual core esp32s. So let's take advantage of this PR to protect uart_data accesses with a mutex or Atomic fields if it's sufficient.
There was a problem hiding this comment.
I think making the two fields uart_data->reader_process_pid, and uart_data->reader_ref_ticks atomic should mitigate the problem, since these are both touched during read, cancel_read, and the interrupt callback.
|
Works great still. nitpick: Tried |
Yes. I see what you mean now, I thought your comment about concurrency was related to the previous implementation, but I see now that you were talking about pre-existing concurrency problems. |
Exactly. And issues could be amplified by the new code. |
Add type definitions for input parameters. Add spec for all exported functions. Add documentation for functions. Signed-off-by: Winford <winford@object.stream>
Adds a new uart:read/2 that takes a timeout parameter for reads. If no data is received during the
timeout period `timeout` is returned, and the listener is removed allowing for another read without
getting the `{error, ealready}` error tuple.
Closes atomvm#1446
Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
uart_add_timeout
branch
from
54b5195 to
b1bc0b0
Compare
| term reader_process_pid; | ||
| uint64_t reader_ref_ticks; | ||
| _Atomic term reader_process_pid; | ||
| _Atomic uint64_t reader_ref_ticks; |
There was a problem hiding this comment.
Now I have some doubts about this fix, it seems like the reader_process_pid and reader_ref_ticks could get out of sync, if a process were to change the reader_process_id, while another running on a different core was waiting to change it, and the first process was interrupted by a higher priority task (before changing reader_ref_ticks), and the second then was allowed to change reader_process_id and then reader_ref_ticks, before the first process was scheduled to resume. When the first process is scheduled again it would continue on to change the reader_ref_ticks.
I think, instead, we need to enter a critical section before updating both values.
Updates the UART driver to include function specs and documentation.
Adds new
uart:read/2with a timeout parameter.Closes #1446
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later