Update UART driver with specs, documentaion, and read/2 #1508

UncleGrumpy · 2025-02-02T11:03:47Z

Updates the UART driver to include function specs and documentation.
Fixes possible concurrency problems, and insufficient sized memory checks.
Now cleans up and returns errors when initializing with badarg parameters.
Adds new uart:read/2 with 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

petermm

works, great! even when doing hundreds of uart:read per second!

libs/eavmlib/src/uart.erl

src/platforms/esp32/components/avm_builtins/uart_driver.c

UncleGrumpy · 2025-02-03T00:41:42Z

I pushed changes that eliminate the use of any timer or task in the C code, and instead added a cancel_read command, handling the timeout and sending the cancel command from erlang, following the suggestion @pguyot made.

I believe this is much more efficient, and less resource intensive than creating a timer.

pguyot

Thank you for this new version! Besides the little bug on timeout result, could you please fix potential concurrency issues?

libs/eavmlib/src/uart.erl

src/platforms/esp32/components/avm_builtins/uart_driver.c

petermm · 2025-02-03T09:28:29Z

Works great still.

nitpick:
Should it only take a positive integer as timeout value? pos_integer()

Tried :uart.read(port, 0) but it would never get a read, of course also a nonsensical value..

UncleGrumpy · 2025-02-03T18:36:14Z

Thank you for this new version! Besides the little bug on timeout result, could you please fix potential concurrency issues?

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.

pguyot · 2025-02-03T19:00:56Z

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.

src/platforms/esp32/components/avm_builtins/uart_driver.c

UncleGrumpy · 2025-02-06T00:14:34Z

This should be ready for another look. I made more bug fixes that should be checked.

CHANGELOG.md

libs/eavmlib/src/uart.erl

src/platforms/esp32/components/avm_builtins/uart_driver.c

CHANGELOG.md

src/platforms/esp32/components/avm_builtins/uart_driver.c

pguyot

Do you want to take advantage of this PR to change the if (xQueueRemove(...)) into a while? It would make queue overflow less likely.

pguyot · 2025-02-08T08:25:22Z

src/platforms/esp32/components/avm_builtins/uart_driver.c

+    int rx_pin = get_uart_pin_opt(opts, RX_ATOM);
+    int rts_pin = get_uart_pin_opt(opts, RTS_ATOM);
+    int cts_pin = get_uart_pin_opt(opts, CTS_ATOM);
+    if (UNLIKELY(tx_pin < UART_PIN_NO_CHANGE || rx_pin < UART_PIN_NO_CHANGE


It's better, but code is still inferring that -2 is less than UART_PIN_NO_CHANGE, which is a little bit hacky or ugly.

The function should take an additional parameter as suggested (int *ok) or we should define a new constant for -2 (UART_BADARG ?) and a static assert that it's less than UART_PIN_NO_CHANGE.

I went with the static assert option, since we only need to check the value of the returned integers. Using the *ok pointer would have required 4 individual checks, rather than one after all pins have been evaluated.

UncleGrumpy · 2025-02-09T23:22:00Z

Do you want to take advantage of this PR to change the if (xQueueRemove(...)) into a while? It would make queue overflow less likely.

I @almost overlooked this. Great idea, I added that change.

Add type definitions for input parameters. Add spec for all exported functions. Add documentation for functions. Signed-off-by: Winford <[email protected]>

Fixes possible concurrency problems by using a mutex for access to `reader_process_pid` and `reader_ref_ticks` in `uart_data`. Avoid possible overflow of the queue by handling all mesages in the interrupt callback. Now cleans up and returns errors, with improved log messages when initalization fails with a badarg in the configuration parameters, rather than aborting. Fixes incorrect pin integers being cast to terms. Pin numbers are validated for the chip the VM is compiled for before use to mitigate VM crashes from bad input parameters. Signed-off-by: Winford <[email protected]>

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 <[email protected]>

robinchew · 2025-02-10T13:29:55Z

httpd_example cannot load index.html if you open UART a certain way using this PR.

This issue does NOT occur in commit 4d38f90 of this PR.

Find my modified http_example code here robinchew/atomvm_lib@033a226 which is essentially the following code which spawns a process that opens UART a certain way before httpd:start:

    spawn(fun() ->
        Port = uart:open("UART2", [{rx, 16}, {tx, 17}]), % need to set rx and tx, and maybe UART2 as well
        io:format("port ~p~n", [Port]),
    end),
    
    httpd:start(....)

Trying to load index.html would result with:

~$ curl -v http://192.168.1.43:8080/index.html
*   Trying 192.168.1.43:8080...
* connect to 192.168.1.43 port 8080 from 192.168.1.4 port 37574 failed: Connection refused
* Failed to connect to 192.168.1.43 port 8080 after 306 ms: Couldn't connect to server
* Closing connection
curl: (7) Failed to connect to 192.168.1.43 port 8080 after 306 ms: Couldn't connect to server

petermm approved these changes Feb 2, 2025

View reviewed changes

pguyot requested changes Feb 2, 2025

View reviewed changes

UncleGrumpy force-pushed the uart_add_timeout branch from 1cd4e1b to 54b5195 Compare February 3, 2025 00:13

UncleGrumpy requested a review from pguyot February 3, 2025 01:02

pguyot requested changes Feb 3, 2025

View reviewed changes

libs/eavmlib/src/uart.erl Outdated Show resolved Hide resolved

src/platforms/esp32/components/avm_builtins/uart_driver.c Outdated Show resolved Hide resolved

UncleGrumpy force-pushed the uart_add_timeout branch from 54b5195 to b1bc0b0 Compare February 3, 2025 21:29

UncleGrumpy commented Feb 3, 2025

View reviewed changes

src/platforms/esp32/components/avm_builtins/uart_driver.c Outdated Show resolved Hide resolved

UncleGrumpy force-pushed the uart_add_timeout branch 8 times, most recently from abfdc3d to 6f19486 Compare February 5, 2025 23:52

arpunk reviewed Feb 6, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

libs/eavmlib/src/uart.erl Outdated Show resolved Hide resolved

UncleGrumpy force-pushed the uart_add_timeout branch from 6f19486 to c7fd896 Compare February 6, 2025 00:56

UncleGrumpy requested a review from pguyot February 6, 2025 02:29

pguyot requested changes Feb 7, 2025

View reviewed changes

UncleGrumpy force-pushed the uart_add_timeout branch 4 times, most recently from 0cdd473 to 38b751a Compare February 7, 2025 23:09

pguyot reviewed Feb 8, 2025

View reviewed changes

UncleGrumpy force-pushed the uart_add_timeout branch from 38b751a to f3886f3 Compare February 9, 2025 02:04

UncleGrumpy force-pushed the uart_add_timeout branch from f3886f3 to 5ac66d6 Compare February 9, 2025 23:17

Update uart driver with specs, types, and function documentation

4d38f90

Add type definitions for input parameters. Add spec for all exported functions. Add documentation for functions. Signed-off-by: Winford <[email protected]>

UncleGrumpy force-pushed the uart_add_timeout branch 3 times, most recently from dd22180 to 6baafa5 Compare February 9, 2025 23:41

UncleGrumpy added 2 commits February 9, 2025 23:41

UncleGrumpy force-pushed the uart_add_timeout branch from 6baafa5 to 56fd43b Compare February 9, 2025 23:43

UncleGrumpy requested a review from pguyot February 10, 2025 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update UART driver with specs, documentaion, and read/2 #1508

Update UART driver with specs, documentaion, and read/2 #1508

UncleGrumpy commented Feb 2, 2025 •

edited

Loading

petermm left a comment

UncleGrumpy commented Feb 3, 2025 •

edited

Loading

pguyot left a comment

petermm commented Feb 3, 2025

UncleGrumpy commented Feb 3, 2025

pguyot commented Feb 3, 2025

UncleGrumpy commented Feb 6, 2025

pguyot left a comment •

edited

Loading

pguyot Feb 8, 2025

UncleGrumpy Feb 9, 2025

UncleGrumpy commented Feb 9, 2025

robinchew commented Feb 10, 2025

Update UART driver with specs, documentaion, and read/2 #1508

Are you sure you want to change the base?

Update UART driver with specs, documentaion, and read/2 #1508

Conversation

UncleGrumpy commented Feb 2, 2025 • edited Loading

petermm left a comment

Choose a reason for hiding this comment

UncleGrumpy commented Feb 3, 2025 • edited Loading

pguyot left a comment

Choose a reason for hiding this comment

petermm commented Feb 3, 2025

UncleGrumpy commented Feb 3, 2025

pguyot commented Feb 3, 2025

UncleGrumpy commented Feb 6, 2025

pguyot left a comment • edited Loading

Choose a reason for hiding this comment

pguyot Feb 8, 2025

Choose a reason for hiding this comment

UncleGrumpy Feb 9, 2025

Choose a reason for hiding this comment

UncleGrumpy commented Feb 9, 2025

robinchew commented Feb 10, 2025

UncleGrumpy commented Feb 2, 2025 •

edited

Loading

UncleGrumpy commented Feb 3, 2025 •

edited

Loading

pguyot left a comment •

edited

Loading