ESP32: support USB-Serial-JTAG in uart driver

[pguyot]

Adds `peripheral, "USB_SERIAL_JTAG"' to the ESP32 uart driver for chips with SOC_USB_SERIAL_JTAG_SUPPORTED (C3/C5/C6/C61/H2/H21/H4/P4/S3).

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]

usual pragmatic pick and choose:

PR Review — 88dcb9d "ESP32: support USB-Serial-JTAG in uart driver"

Author: Paul Guyot pguyot@kallisys.net
Date: Sat May 9 12:00:00 2026 +0200
Files changed:

• CHANGELOG.md (+2)
• doc/src/programmers-guide.md (+21/-1)
• libs/avm_esp32/src/uart.erl (+4)
• src/platforms/esp32/components/avm_builtins/uart_driver.c (+250/-43)

Summary

This adds USB-Serial-JTAG (USJ) support to the existing ESP32 UART port
driver by wrapping the IDF driver/usb_serial_jtag.h API. Because that
API is blocking-only, a dedicated FreeRTOS task is spawned to bridge it
into a uart_event_t queue / ringbuffer compatible with the existing
event-listener flow.

Two substantive issues should block merge:

1. USJ shutdown is unsafe — vTaskDelete() is called on the reader
   task while it is blocked inside
   usb_serial_jtag_read_bytes(portMAX_DELAY).
2. USJ async read trusts uart_event_t.size — but for this bridge
   the event is only a wake-up; relying on its size field can produce
   wrong-length binaries with bogus zero padding.

A handful of smaller correctness, docs and style issues are listed
below.

---

Findings overview

#	Severity	Location	Summary
1	Major	uart_driver.c:380-489	Listener registered before sender/rxqueue are set; uart_data not zero-initialized.
2	Major	uart_driver.c:134-152, 659-669	Reader task force-deleted while blocked in IDF call. Use cooperative shutdown.
3	Major	uart_driver.c:147-150, 193-201, 542-546	USJ async path treats event.size as authoritative; should drive sizing from ringbuffer availability.
4	Major	uart_driver.c:147-150	Event queue full drops notification but bytes remain in ringbuffer → size accounting drift.
5	Minor	programmers-guide.md:1954-1956 vs C code	Docs say USJ line-coding/pin options are "accepted but ignored", but C still validates and rejects invalid values.
6	Minor	uart_driver.c:272+ (pre-existing)	Context *ctx = context_new(global) is leaked on every return NULL.
7	Minor	uart_driver.c:428-429	Reader task priority tskIDLE_PRIORITY + 5 looks unjustified.
8	Nit	many sites	Repeated #if SOC_USB_SERIAL_JTAG_SUPPORTED if (use_usj) ... #endif pattern is noisy.
9	Nit	uart.erl:211-222	validate_peripheral catch branches return {bardarg, ...} instead of throw-ing it (inconsistent with the catch-all clause).
10	Nit	uart_driver.c:161-164	Dead if (item != NULL) after the outer `

Items checked but no issue:

• Ordering of sys_unregister_listener() vs xQueueRemoveFromSet() in
  uart_driver_do_close — safe (listener lock prevents racing
  invocation).
• New sys_unregister_listener() calls in failure paths — these are
  the right fix for a pre-existing cleanup bug.
• uart_data->use_usj cross-task visibility — fine, set once before
  xTaskCreate, then read-only.
• Keeping the validate_peripheral("USB_SERIAL_JTAG") clause
  unconditional in uart.erl — correct: validation runs on the device
  that opened the port, but capability checking belongs in the native
  layer, not in Erlang.

---

Detailed review

1) Listener registration ordering / uninitialized uart_data — Major

In uart_driver_create_port the listener is registered immediately
after malloc, while listener.sender, rxqueue, ringbuffer and the
reader task have not been set up yet:

struct UARTData *uart_data = malloc(alloc_size);  // not zeroed
...
uart_data->listener.handler = uart_interrupt_callback;
sys_register_listener(global, &uart_data->listener);   // registered HERE
...
ctx->platform_data = uart_data;
...
uart_data->listener.sender = uart_data->rxqueue;       // set MUCH later
if (xQueueAddToSet(uart_data->rxqueue, event_set) != pdPASS) { ... }

Event dispatch matches a listener by comparing
listener->sender == event_source. With uart_data from malloc,
sender is uninitialized garbage from the moment the listener is
visible until the assignment near the end of create_port. If garbage
happens to alias a live source, uart_interrupt_callback() runs
against half-initialized state.

It also makes every new failure path in this commit responsible for
calling sys_unregister_listener(), which is fragile.

Fix: zero-initialize and register the listener last.

- struct UARTData *uart_data = malloc(alloc_size);
+ struct UARTData *uart_data = calloc(1, alloc_size);
  if (IS_NULL_PTR(uart_data)) {
      ...
  }
  uart_data->listener.handler = uart_interrupt_callback;
- sys_register_listener(global, &uart_data->listener);
  uart_data->reader_process_pid = term_invalid_term();
  uart_data->reader_ref_ticks = 0;
  uart_data->uart_num = uart_num;
  ...
  uart_data->listener.sender = uart_data->rxqueue;
  if (xQueueAddToSet(uart_data->rxqueue, event_set) != pdPASS) {
      ...
  }
+ sys_register_listener(global, &uart_data->listener);

After this, most of the new sys_unregister_listener() calls in the
USJ install path can be dropped.

---

2) usj_reader_task shutdown is not robust — Major

The reader task blocks indefinitely inside the IDF driver:

while (true) {
    int n = usb_serial_jtag_read_bytes(buf, sizeof(buf), portMAX_DELAY);
    ...
}

…and shutdown calls vTaskDelete() on it from
uart_driver_do_close(). Force-deleting a task blocked inside a
peripheral driver is brittle: it relies on undocumented behaviour of
the IDF driver's internal locking and on FreeRTOS unwinding cleanly,
both of which can change. Even today there is no real guarantee of
clean release of internal driver state.

Fix: cooperative shutdown — finite read timeout + stop flag +
synchronous wait-for-exit before freeing backend resources.

 struct UARTData
 {
     ...
 #if SOC_USB_SERIAL_JTAG_SUPPORTED
     bool use_usj;
+    volatile bool usj_stop;
     RingbufHandle_t usj_rx_ringbuf;
     TaskHandle_t usj_reader_task;
+    SemaphoreHandle_t usj_reader_stopped;
 #endif
     ...
 };

 static void usj_reader_task(void *arg)
 {
     struct UARTData *uart_data = arg;
     uint8_t buf[AVM_USJ_READ_CHUNK];
-    while (true) {
-        int n = usb_serial_jtag_read_bytes(buf, sizeof(buf), portMAX_DELAY);
+    while (!uart_data->usj_stop) {
+        int n = usb_serial_jtag_read_bytes(buf, sizeof(buf), pdMS_TO_TICKS(50));
         if (n <= 0) {
             continue;
         }
         ...
     }
+    xSemaphoreGive(uart_data->usj_reader_stopped);
+    vTaskDelete(NULL);
 }

…and in uart_driver_do_close:

 if (uart_data->use_usj) {
-    if (uart_data->usj_reader_task != NULL) {
-        vTaskDelete(uart_data->usj_reader_task);
-    }
+    uart_data->usj_stop = true;
+    if (uart_data->usj_reader_stopped != NULL) {
+        xSemaphoreTake(uart_data->usj_reader_stopped, pdMS_TO_TICKS(200));
+        vSemaphoreDelete(uart_data->usj_reader_stopped);
+    }
     ...
 }

A force-vTaskDelete() fallback if the wait times out is acceptable
belt-and-braces.

---

3) USJ async read incorrectly trusts event.size — Major

For hardware UART, uart_event_t.size comes straight from the IDF
driver and matches what uart_read_bytes() will return.

For USJ, event.size is synthesized by usj_reader_task:

xRingbufferSend(..., buf, n, ...)
uart_event_t event = { .type = UART_DATA, .size = (size_t) n };
xQueueSend(uart_data->rxqueue, &event, 0)

The bytes live in a separate ringbuffer; queue and ringbuffer can
diverge (previous direct read drained ahead, queue events coalesced or
dropped, etc.). The current async path nevertheless does:

term bin = term_create_uninitialized_binary(event.size, &heap, glb);
...
size_t got = usj_drain_rx(uart_data, bin_buf, event.size);
if (got < event.size) {
    memset(bin_buf + got, 0, event.size - got);
}

That is wrong — when fewer bytes are available the binary should be
shorter, not zero-padded. Synthetic \0 bytes look like real data to
the reader process and silently corrupt protocol decoders.

uart_driver_do_read() is already much closer to correct, because it
reads count from vRingbufferGetInfo(). The async callback should
do the same.

Fix: treat the queue as a wake-up only; size from the ringbuffer.

+#if SOC_USB_SERIAL_JTAG_SUPPORTED
+static size_t usj_rx_available(struct UARTData *uart_data)
+{
+    UBaseType_t avail = 0;
+    vRingbufferGetInfo(uart_data->usj_rx_ringbuf, NULL, NULL, NULL, NULL, &avail);
+    return (size_t) avail;
+}
+#endif

 case UART_DATA_BREAK:
 case UART_DATA:
     if (uart_data->reader_process_pid != term_invalid_term()) {
-        int bin_size = term_binary_heap_size(event.size);
+        size_t count = event.size;
+#if SOC_USB_SERIAL_JTAG_SUPPORTED
+        if (uart_data->use_usj) {
+            count = usj_rx_available(uart_data);
+            if (count == 0) {
+                break;   // stale/coalesced wake-up
+            }
+        }
+#endif
+        int bin_size = term_binary_heap_size(count);
         ...
-        term bin = term_create_uninitialized_binary(event.size, &heap, glb);
+        term bin = term_create_uninitialized_binary(count, &heap, glb);
         uint8_t *bin_buf = (uint8_t *) term_binary_data(bin);
 #if SOC_USB_SERIAL_JTAG_SUPPORTED
         if (uart_data->use_usj) {
-            size_t got = usj_drain_rx(uart_data, bin_buf, event.size);
-            if (got < event.size) {
-                memset(bin_buf + got, 0, event.size - got);
-            }
+            size_t got = usj_drain_rx(uart_data, bin_buf, count);
+            if (UNLIKELY(got != count)) {
+                ESP_LOGW(TAG, "USB_SERIAL_JTAG drained %u/%u bytes",
+                    (unsigned) got, (unsigned) count);
+            }
         }
 #endif

Same change applies to the read-path zero-padding in
uart_driver_do_read() (lines 542–546): drop the memset() and
either trust the ringbuffer or shrink the binary.

---

4) Event-queue overflow drops notifications, keeps bytes — Major

if (xRingbufferSend(...) != pdTRUE) {
    ...dropping bytes...
    continue;
}
uart_event_t event = { .type = UART_DATA, .size = (size_t) n };
if (xQueueSend(uart_data->rxqueue, &event, 0) != pdTRUE) {
    ESP_LOGW(TAG, "USB_SERIAL_JTAG event queue full");
}

When the event queue is full the bytes stay in the ringbuffer but
the wake-up event is dropped. With the current event.size-based
consumer, later reads will then run with a stale size and again
mis-frame data. After fix #3 this degrades to a benign coalescing
case, but it is worth coalescing on purpose — one pending UART_DATA
event is enough.

Fix: only enqueue when the queue is empty; carry no size.

- uart_event_t event = { .type = UART_DATA, .size = (size_t) n };
- if (xQueueSend(uart_data->rxqueue, &event, 0) != pdTRUE) {
+ uart_event_t event = { .type = UART_DATA, .size = 0 };
+ if (uxQueueMessagesWaiting(uart_data->rxqueue) == 0
+         && xQueueSend(uart_data->rxqueue, &event, 0) != pdTRUE) {
     ESP_LOGW(TAG, "USB_SERIAL_JTAG event queue full");
 }

---

5) Docs vs behaviour mismatch for USJ options — Minor

doc/src/programmers-guide.md says:

Pin and line-coding options (rx, tx, speed, data_bits,
stop_bits, flow_control, parity) are accepted but ignored

But uart_driver_create_port() still validates and rejects these
options when use_usj is true (e.g. data_bits = 9 will fail before
it ever reaches the USJ path).

Two acceptable directions:

• Behaviour (preferred): skip UART-only validation when
  use_usj — i.e. move all UART-only parsing into the
  if (!use_usj) branch.
• Docs: clarify that the options are still parsed/validated for
  compatibility but not applied.

I prefer the behaviour fix; "ignored" naturally implies "not
validated".

---

6) Context *ctx leaked on every failure return — Minor (pre-existing)

uart_driver_create_port() starts with:

Context *ctx = context_new(global);

…then has many return NULL; paths (made worse by this commit) that
never destroy ctx. Fix is a single goto fail; cleanup label that
calls context_destroy(ctx) (and the new USJ teardown).

---

7) Reader task priority — Minor

xTaskCreate(usj_reader_task, "usj_reader", 2560, uart_data,
    tskIDLE_PRIORITY + 5, &uart_data->usj_reader_task);

idle + 5 looks arbitrarily high relative to AtomVM scheduler tasks
on ESP32. Suggest tskIDLE_PRIORITY + 1 (or
uxTaskPriorityGet(NULL)). The task is mostly blocked, so this is
unlikely to manifest, but it could preempt VM work under sustained USB
input.

Stack size (2560) looks fine.

---

8) Repetitive preprocessor pattern — Nit

In create/read/write/close the same construct is repeated:

#if SOC_USB_SERIAL_JTAG_SUPPORTED
if (uart_data->use_usj) {
    ...
}
#endif
#if SOC_USB_SERIAL_JTAG_SUPPORTED
if (!uart_data->use_usj)
#endif
{
    ...
}

Consider either a single if/else under one #if, or small helpers:
uart_rx_available(), uart_read_into(), uart_write_all(),
uart_backend_close(). Pure maintainability — not a correctness
issue.

---

9) validate_peripheral returns {bardarg, ...} instead of throwing — Nit

In libs/avm_esp32/src/uart.erl:

validate_peripheral([$U, $A, $R, $T | N] = Value) ->
    try list_to_integer(N) of
        _ -> Value
    catch
        error:_ -> {bardarg, {peripheral, Value}}    %% returns, doesn't throw
    end;
validate_peripheral(<<"UART", N/binary>> = Value) ->
    try binary_to_integer(N) of
        _ -> Value
    catch
        error:_ -> {bardarg, {peripheral, Value}}    %% returns, doesn't throw
    end;
...
validate_peripheral(Value) ->
    throw({bardarg, {peripheral, Value}}).           %% throws

So "UARTfoo" / <<"UARTfoo">> silently propagate {bardarg, ...}
into migrate_config/1 instead of failing the same way as other
invalid peripherals.

Suggested diff (preserves the existing bardarg spelling, which is
also used in spi.erl / i2c.erl):

 validate_peripheral([$U, $A, $R, $T | N] = Value) ->
     try list_to_integer(N) of
         _ -> Value
     catch
-        error:_ -> {bardarg, {peripheral, Value}}
+        error:_ -> throw({bardarg, {peripheral, Value}})
     end;
 validate_peripheral(<<"UART", N/binary>> = Value) ->
     try binary_to_integer(N) of
         _ -> Value
     catch
-        error:_ -> {bardarg, {peripheral, Value}}
+        error:_ -> throw({bardarg, {peripheral, Value}})
     end;

The new clauses for "USB_SERIAL_JTAG" are correctly written.

---

10) usj_drain_rx defensive branch — Nit

if (item == NULL || recv_size == 0) {
    if (item != NULL) {
        vRingbufferReturnItem(uart_data->usj_rx_ringbuf, item);
    }
    break;
}

If xRingbufferReceiveUpTo() on a RINGBUF_TYPE_BYTEBUF cannot
return a non-NULL item with recv_size == 0, the inner if (item != NULL) is dead code — simplify or add a comment.

---

Items checked, no issue

A) Ordering in uart_driver_do_close

sys_unregister_listener() before xQueueRemoveFromSet() is safe.
The dispatch path takes the listeners lock while finding/invoking a
handler, and sys_unregister_listener() takes the same lock. After it
returns, the listener cannot fire again. If the queue was already
selected from the set, the loop will fail to find a handler for that
sender, which is harmless.

B) New sys_unregister_listener() in error paths

Correct — they fix a real pre-existing cleanup bug for the case where
the listener was registered but a later step failed.

C) uart_data->use_usj visibility

Set once before xTaskCreate(), then read-only. No cross-task
synchronization needed.

D) validate_peripheral("USB_SERIAL_JTAG") unconditional in Erlang

Correct. Capability checking belongs in the native layer; gating the
Erlang clause on the SoC would push hardware checks to the wrong
layer.

---

Recommended minimum changes before merge

1. Cooperative USJ shutdown (Enhancement: Port to Darwin and FreeBSD #2).
2. Treat the USJ event queue as wake-up only and stop trusting
   event.size / drop the zero-padding (Link fails on CentOS6 #3, Darwin support #4).
3. Move listener registration to last and zero-initialize uart_data
   (bifs_hash.h not found #1).
4. Either align docs to current option behaviour or skip UART-only
   validation for USJ (Add support to OTP21 #5).

The remaining items are clean-up that can be addressed in a follow-up.

Merge recommendation

Request changes — the feature is welcome, but the unsafe forced
shutdown of the blocking USJ reader task and the reliance on
event.size for USJ reads need to be fixed before merge.

[petermm]

Posted wrong comment above, sorry..

AMP:

PR Review — 7d285e3 "ESP32: support USB-Serial-JTAG in uart driver" (round 2)

Author: Paul Guyot pguyot@kallisys.net
Files changed (round 2):

• CHANGELOG.md (+2)
• doc/src/programmers-guide.md (+22/-1)
• libs/avm_esp32/src/uart.erl (+8/-4)
• src/platforms/esp32/components/avm_builtins/uart_driver.c (+318/-48)

This is the updated version of the earlier 88dcb9d commit. The
contributor has addressed most of the previous round's findings — well
done. Two new follow-up issues remain (one likely runtime-broken
Erlang line, and a small allocation inefficiency), plus the still-open
pre-existing items.

---

Status of previous review items

#	Previous severity	Status in 7d285e3
1	Major — listener registered too early, uart_data not zero-init	Fixed — now calloc(1, ...) and sys_register_listener() is the last step of uart_driver_create_port
2	Major — vTaskDelete() of blocked reader task	Fixed — cooperative stop: usj_stop flag, finite read timeout (AVM_USJ_READ_TIMEOUT_MS = 50 ms), usj_reader_stopped semaphore, with vTaskDelete fallback only on timeout
3	Major — USJ async read trusts event.size, zero-pads	Fixed — count = usj_rx_available(); drains exact available bytes; binary is sized to actual drained length; no zero padding
4	Major — event queue full drops wake-up but keeps bytes	Fixed — uxQueueMessagesWaiting(rxqueue) == 0 coalescing pattern, .size = 0
5	Minor — docs say USJ options are "ignored" but C still validates	Fixed via docs — now states "parsed and validated like regular UART options, but otherwise ignored"
6	Minor — Context *ctx = context_new(global) leaked on return NULL	Not fixed (pre-existing, broader than this PR)
7	Minor — reader task priority idle+5	Fixed — now tskIDLE_PRIORITY + 1
8	Nit — repetitive #if ... if (use_usj) pattern	Not fixed (style; defer)
9	Nit — validate_peripheral returned {bardarg, ...} instead of throwing	Changed but BROKEN — see new issue N1 below
10	Nit — dead if (item != NULL) in usj_drain_rx	Fixed — simplified

---

New issues introduced in this revision

N1) validate_peripheral calls erlang:error/2 with wrong argument shape — Major (runtime crash)

libs/avm_esp32/src/uart.erl:

validate_peripheral([$U, $A, $R, $T | N] = Value) ->
    try list_to_integer(N) of
        _ -> Value
    catch
        error:_ -> error(bardarg, {peripheral, Value})   %% <-- wrong
    end;
validate_peripheral(<<"UART", N/binary>> = Value) ->
    try binary_to_integer(N) of
        _ -> Value
    catch
        error:_ -> error(bardarg, {peripheral, Value})   %% <-- wrong
    end;
...
validate_peripheral(Value) ->
    error(bardarg, {peripheral, Value}).                 %% <-- wrong

Two problems:

1. erlang:error/2 signature: the second argument must be a
   list of the original function's arguments (it becomes the
   "args" entry in the stack trace). Passing a {peripheral, Value}
   tuple raises badarg from error/2 itself, masking the intended
   error.

2. Typo bardarg is now the reason atom (not nested inside a
   tuple), so existing consumers matching the previous
   {bardarg, {peripheral, Value}} shape will silently break.

   bardarg is preserved elsewhere in spi.erl/i2c.erl, so changing
   only here also creates an inconsistency.

Suggested fix:

 validate_peripheral([$U, $A, $R, $T | N] = Value) ->
     try list_to_integer(N) of
         _ -> Value
     catch
-        error:_ -> error(bardarg, {peripheral, Value})
+        error:_ -> error(badarg, [{peripheral, Value}])
     end;
 validate_peripheral(<<"UART", N/binary>> = Value) ->
     try binary_to_integer(N) of
         _ -> Value
     catch
-        error:_ -> error(bardarg, {peripheral, Value})
+        error:_ -> error(badarg, [{peripheral, Value}])
     end;
 validate_peripheral("USB_SERIAL_JTAG" = Value) ->
     Value;
 validate_peripheral(<<"USB_SERIAL_JTAG">> = Value) ->
     Value;
 validate_peripheral(Value) ->
-    error(bardarg, {peripheral, Value}).
+    error(badarg, [{peripheral, Value}]).

Two callouts:

• Using the correct error/2 form (badarg reason + list of args) is
  the idiomatic way and matches what callers normally pattern-match.
• If preserving backward-compatibility with the (also wrong) old
  {bardarg, ...} shape is more important than fixing the spelling,
  then use error({bardarg, {peripheral, Value}}) (single-arg form)
  instead. Either way, the current code is broken.

---

N2) Async path double-allocates RX buffer — Minor

uart_interrupt_callback for the USJ branch now does:

usj_buf = malloc(count);
...
count = usj_drain_rx(uart_data, usj_buf, count);
...
term bin = term_create_uninitialized_binary(count, &heap, glb);
uint8_t *bin_buf = (uint8_t *) term_binary_data(bin);
if (uart_data->use_usj) {
    memcpy(bin_buf, usj_buf, count);
    free(usj_buf);
}

So we malloc a scratch buffer, drain into it, then allocate the
Erlang binary, then memcpy from scratch to binary. The temporary copy
is unnecessary: drain directly into bin_buf. The reason the code
currently can't is that the binary is allocated after count is
known.

A simple restructure avoids the malloc + memcpy entirely:

- usj_buf = malloc(count);
- if (IS_NULL_PTR(usj_buf)) { ... AVM_ABORT(); }
- count = usj_drain_rx(uart_data, usj_buf, count);
- if (count == 0) { free(usj_buf); break; }
- ...
- term bin = term_create_uninitialized_binary(count, &heap, glb);
- uint8_t *bin_buf = (uint8_t *) term_binary_data(bin);
- if (uart_data->use_usj) {
-     memcpy(bin_buf, usj_buf, count);
-     free(usj_buf);
- }
+ // ringbuffer can shrink between availability check and drain
+ size_t drain_max = count;
+ int bin_size = term_binary_heap_size(drain_max);
+ Heap heap;
+ if (UNLIKELY(memory_init_heap(&heap, bin_size + REF_SIZE + TUPLE_SIZE(2) * 2) != MEMORY_GC_OK)) {
+     ...
+ }
+ term bin = term_create_uninitialized_binary(drain_max, &heap, glb);
+ uint8_t *bin_buf = (uint8_t *) term_binary_data(bin);
+ size_t got = usj_drain_rx(uart_data, bin_buf, drain_max);
+ if (got == 0) {
+     memory_destroy_heap(&heap, glb);
+     break;
+ }
+ // If drained less than worst-case, shrink the binary. (If
+ // `term_binary_set_size`/equivalent isn't available, simply allocate
+ // enough heap and accept the conservative oversize on the rare
+ // shrink case.)

Same applies to the synchronous uart_driver_do_read path. The
double-buffering is harmless correctness-wise but wastes memory under
load.

---

N3) Small window where rxqueue is in event_set but no listener is registered — Minor

The new ordering in uart_driver_create_port is:

1. xTaskCreate(usj_reader_task, ...)         // task may immediately enqueue events
2. uart_data->listener.sender = rxqueue;
3. xQueueAddToSet(rxqueue, event_set);       // event_set can now select rxqueue
4. smp_mutex_create();                       // (SMP only)
5. sys_register_listener(global, &uart_data->listener);

Between step 3 and step 5, the platform event loop can pull rxqueue
out of the set and find no matching listener. Whether that drops the
event or leaves it in the queue depends on
sys_register_listener/dispatch semantics.

If the platform dispatch reads from the queue before finding the
listener (some implementations do this), early events can be silently
lost. Suggested mitigation: move xQueueAddToSet to after
sys_register_listener, or perform sys_register_listener first and
add to the set last under the assumption that the listener will be
matched.

Worth verifying behavior of the platform listener loop. Not blocking,
but worth a comment if it is intentionally OK.

---

Items still outstanding from round 1

O1) Context *ctx leaked on every return NULL — Minor (pre-existing)

The new code adds more failure return paths, none of which destroy
the Context *ctx allocated at the top of uart_driver_create_port.
A single goto fail; cleanup label would normalize this.

O2) Repetitive #if SOC_USB_SERIAL_JTAG_SUPPORTED ... if (use_usj) pattern — Nit

Still present in create_port, the listener callback,
uart_driver_do_read, uart_driver_do_write, and
uart_driver_do_close. Pure style. Could be cleaned up later via
small helpers or a single if/else under one #if.

---

Items checked, no issue

• Shutdown sequence (usj_stop_reader_task): stop flag + bounded
  xSemaphoreTake + vTaskDelete fallback + vSemaphoreDelete is
  the safe sequence. The reader task gives the semaphore before
  vTaskDelete(NULL), so the caller doesn't free the semaphore while
  the task is still using it.
• Coalesced wake-up events (.size = 0) match the new sizing
  logic (which uses usj_rx_available()).
• sys_register_listener ordering vs xQueueAddToSet: see N3 —
  worth a glance, but probably benign on the current platform
  implementation.
• Listener removed from set in close before unregister: the close
  path correctly calls xQueueRemoveFromSet(rxqueue, event_set) and
  sys_unregister_listener before deleting the queue.
• USJ option validation: docs now match behaviour (Add support to OTP21 #5).

---

Recommended minimum diff before merge

1. Fix validate_peripheral error/2 shape (issue N1) — this
   is currently runtime-broken on any non-UART/non-USJ peripheral.
2. (Optional) drop the scratch buffer in the async USJ read path
   (issue N2) — minor but easy.
3. (Optional) verify N3 with the listener event-set semantics on
   ESP32; either reorder or add a comment.

Merge recommendation

Approve with one required change (N1): the Erlang validator's
error/2 shape needs fixing before merge. The C-side concerns from
round 1 have been correctly resolved.

[petermm]

Status of all open items

#	Round-2 severity	Status
N1	Major (runtime crash) — error/2 shape	Fixed — now error(badarg, [{peripheral, Value}]). Spelling corrected (bardarg → badarg) and the second argument is now a proper list of args.
N2	Minor — async path double-allocates RX buffer (malloc + memcpy into binary)	Not addressed — still present in both the listener callback and uart_driver_do_read. Style/perf only; safe.
N3	Minor — small window where rxqueue is in event_set before sys_register_listener runs	Not addressed — same ordering as round 2. Worth a comment that it's intentional, or move xQueueAddToSet after sys_register_listener.
O1	Minor (pre-existing) — Context *ctx leaked on every return NULL	Fixed — context_new(global) is now the very last step. All earlier failure paths return NULL without allocating a context, so nothing leaks.
O2	Nit — repetitive #if SOC_USB_SERIAL_JTAG_SUPPORTED ... if (use_usj) pattern	Not addressed — pure style; defer.

Notes on the changes

validate_peripheral (N1) — verified correct

erlang:error/2 semantics: error(Reason, Args) — Reason is the
exception reason, Args is a list of the surrounding function's args
that surfaces in the stack trace. The new code:

error(badarg, [{peripheral, Value}])

is the standard idiom and now matches what other AtomVM modules do
when raising badarg for a single offending argument.

The original bardarg typo also disappears here. spi.erl /
i2c.erl still use it — out of scope for this PR.

Context *ctx lifecycle (O1) — verified correct

After the change:

• context_new(global) runs only after USJ/UART driver install,
  ringbuffer / queue / semaphore creation, reader-task spawn, listener
  add-to-set, mutex creation, and sys_register_listener have all
  succeeded.
• Every failure path before that frees its own resources and returns
  NULL without ever creating a context.
• After context_new, no further failure paths exist; the function
  goes straight to return ctx;.

grep-verified that the only ctx/ctx->* references in
uart_driver_create_port are the three lines at the very end.

Items still outstanding (all minor / style)

• N2: drain directly into bin_buf in the async path to avoid
  the temporary malloc/memcpy.
• N3: clarify or fix the xQueueAddToSet vs sys_register_listener
  ordering window.
• O2: consolidate the repeated #if SOC_USB_SERIAL_JTAG_SUPPORTED
  blocks (style only).

Merge recommendation

Approve. All previously identified blocking issues (the four C-side
Major items in round 1 plus the runtime-broken error/2 call in
round 2) have been correctly resolved. The remaining items are
cosmetic / micro-optimization and safe to defer.

[pguyot]

I disagree about N2 but didn't leave a comment about this.

Started to look into N3 but then got distracted and forgot about it. The risk is a lost message. I have yet to confirm if we should swap statements.

[pguyot]

Fixed N3 and also a neighbor bug that preexisted in uart_driver. xQueueAddToSet could fail if the queue was not empty, for example with uart activity between uart_driver_install and xQueueAddToSet.

[petermm]

I agree N2 is not worth pursuing, LGTM and to the LLM..

Status of all open items

#	Round-3 status	Round-4 status
N1 (Major) — error/2 shape	Fixed in round 3	Still fixed
N2 (Minor) — async path double-allocates RX buffer	Not addressed	Not addressed (acceptable; safe)
N3 (Minor) — xQueueAddToSet vs sys_register_listener ordering window	Not addressed	Fixed — same reorder as the local hand-edit, with an extra stale-event drain loop
O1 (Minor) — Context *ctx leak on return NULL	Fixed in round 3	Still fixed
O2 (Nit) — repetitive #if pattern	Not addressed	Not addressed (style; defer)

Notes on the N3 fix in this revision

New ordering inside uart_driver_create_port:

1. install driver / create ringbuf / queue / semaphore (USJ) — or
   uart_driver_install (UART)
2. listener.sender = rxqueue
3. smp_mutex_create()
4. sys_register_listener() — listener is now visible
5. while (xQueueAddToSet(...) != pdPASS) { drain one stale event; retry }
6. xTaskCreate(usj_reader_task) (USJ only) — reader task starts
   producing events only after the queue is in the event set and
   the listener is registered

Why this is sound:

• For the USJ branch, the only producer is usj_reader_task, which
  is spawned in step 6. Before step 6 the queue is empty, so the
  while loop adds the queue to the set on the first try.
• For the UART branch, the IDF UART RX ISR can enqueue events
  between uart_driver_install (step 1) and xQueueAddToSet (step
  5). The new while loop handles this by draining one stale event
  per failed AddToSet attempt. Dropping these is fine: no reader
  process is registered yet (reader_process_pid == term_invalid_term()),
  and the listener handler would have done nothing with them anyway.
• The listener handler is safe even if it fires immediately after
  step 5 (between AddToSet success and xTaskCreate): calloc
  initialises reader_process_pid to invalid, so the handler hits the
  early-out before touching the mutex.

Cleanup on xTaskCreate failure is complete and in correct reverse
order: xQueueRemoveFromSet → sys_unregister_listener →
smp_mutex_destroy → vSemaphoreDelete → vRingbufferDelete →
vQueueDelete → usb_serial_jtag_driver_uninstall → free(uart_data).

The usj_stop_reader_task() helper is correctly not called from
the AddToSet-failure cleanup — at that point the reader task hasn't
been spawned yet, so only the semaphore needs deleting.

Minor / theoretical observations on the stale-drain loop

• If the UART ISR sustains a high enough event rate to keep the queue
  non-empty between xQueueReceive and xQueueAddToSet indefinitely,
  the loop could spin. In practice this would require continuous
  inbound traffic during port creation, which is essentially never the
  case. Not worth defending against.
• The dropped stale events are logged nowhere. Probably fine; you
  could add a TRACE if useful for debugging, but it's not required.

Items still outstanding (all minor / style)

• N2: drain directly into bin_buf in the async path to avoid
  the temporary malloc/memcpy. Pure micro-opt.
• O2: consolidate the repeated #if SOC_USB_SERIAL_JTAG_SUPPORTED
  blocks. Pure style.

Merge recommendation

Approve. All previously identified blocking and minor-correctness
issues from rounds 1–3 are resolved. The remaining items are
style/micro-optimization and safe to defer.

[pguyot]

@bettio just added a small rot13 example that I tried on an ESP32-C5-DevKitC-1