Open
Conversation
📝 WalkthroughWalkthroughAdds end-to-end ESP32 Wi‑Fi scanning: new Erlang APIs Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Erlang as Erlang
participant NetLib as libs/eavmlib\n(network.erl)
end
rect rgba(240,255,220,0.5)
participant Mailbox as Mailbox Loop
participant Driver as network_driver.c
end
participant ESP as ESP-IDF
Erlang->>NetLib: network:wifi_scan([Opts])
NetLib->>NetLib: validate/build scan config & ref
NetLib->>Mailbox: enqueue {scan, Config, pid, ref}
Mailbox->>Driver: NetworkScanCmd -> wifi_scan(ctx, pid, ref, config)
Driver->>ESP: esp_wifi_scan_start(scan_config)
ESP->>Driver: WIFI_EVENT_SCAN_DONE
Driver->>Driver: scan_done_handler -> build results
Driver->>NetLib: deliver {scan_results, {N, networks}} or {error, reason}
NetLib->>Erlang: reply or invoke callback
sequenceDiagram
participant Erlang as Erlang
participant NetLib as libs/eavmlib\n(network.erl)
participant Mailbox as Mailbox Loop
participant Driver as network_driver.c
participant ESP as ESP-IDF
participant Callback as UserCallback
Erlang->>NetLib: network:start([{sta, ...,{scan_done,Fun}}])
NetLib->>NetLib: store Fun as scan_receiver
Erlang->>NetLib: network:wifi_scan()
NetLib->>Mailbox: enqueue scan request
Mailbox->>Driver: NetworkScanCmd
Driver->>ESP: esp_wifi_scan_start(...)
ESP->>Driver: WIFI_EVENT_SCAN_DONE
Driver->>NetLib: deliver {N,Networks}
NetLib->>Callback: Fun({N,Networks})
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
libs/eavmlib/src/network.erl (2)
233-235: Incorrect type forscan_receiverfield.The field stores the
Fromargument fromgen_server:call, which is a{pid(), Tag}tuple, not a plainpid(). This should be reflected in the type for accuracy.Proposed fix
sta_state :: sta_status(), - scan_receiver :: pid() + scan_receiver :: {pid(), term()} | undefined }).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 233 - 235, The type for the record field scan_receiver is wrong: it stores the gen_server:call From value which is a {Pid, Tag} tuple, not a plain pid(); update the typespec to reflect this (e.g. change scan_receiver :: pid() to scan_receiver :: {pid(), reference()} or scan_receiver :: {pid(), tag()} and add a -type tag() :: reference() if you prefer a named alias) so the record and any callers reflect the actual {pid(), Tag} shape returned by gen_server:call.
516-522: Missing-specforwifi_scan/0.The function
wifi_scan/1has a spec (lines 501-503), butwifi_scan/0lacks one. Add a spec for consistency and documentation.Proposed fix
+%% `@doc` Equivalent to wifi_scan/1 with default options from sta_config(). +%% `@end` +-spec wifi_scan() -> + {ok, {NetworksDiscovered :: 0..20, [{SSID :: string(), [ApInfo :: network_properties()]}, ...]}} + | {error, Reason :: term()}. wifi_scan() -> Config = gen_server:call(?SERVER, get_config),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 516 - 522, Add a -spec for wifi_scan/0 to match the documented return type used by wifi_scan/1: place a -spec wifi_scan() -> <same_type_as_wifi_scan/1_spec> above the wifi_scan() function so the zero-arity clause is documented consistently with wifi_scan/1 (i.e., mirror the exact type expression used in the existing -spec for wifi_scan/1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 157-186: The docs contain typos and an incorrect API name and
tuple syntax: fix the malformed return tuple example to use {ok, {Num,
Networks}} semantics consistently, correct "discoverd" → "discovered" and other
typos, and replace any mention of network:disconnect/0 with the correct API
network:sta_disconnect/0; also ensure references to network:wifi_scan/0 and
network:wifi_scan/1, network:scan_options(), and the sta_config() option text
match the actual type names and phrasing used elsewhere (e.g., "FoundAPs may be
larger than length of Networks" → clearer wording) so the prose and examples are
consistent with the real function signatures and return shapes.
In `@examples/erlang/esp32/wifi_scan.erl`:
- Around line 41-47: The current anonymous function in wifi_scan.erl matches a
tuple/proplist result ({SSID, [{rssi, DBm}, ...]}) but the scan payload now uses
maps; update the pattern in the anonymous function (and the similar block at
lines 63-69) to match a map shape such as #{ssid := SSID, rssi := DBm, authmode
:= Mode, bssid := BSSID, channel := Number} and keep the same formatting call
(using binary:encode_hex(BSSID) if BSSID is a binary) so the output logic
(io:format(..., [SSID, binary:encode_hex(BSSID), DBm, Mode, Number])) continues
to work with the new map-based scan result.
- Line 55: The current assignment uses lists:flatten/1 unnecessarily: change the
construction of Opts so it prepends the atom passive directly to the existing
flat list Config (remove lists:flatten/1). Locate the expression building Opts
(the variable Opts in wifi_scan.erl) and replace the lists:flatten([passive |
Config]) usage with a direct list cons of passive onto Config (i.e., construct
Opts by prepending passive to Config).
In `@libs/eavmlib/src/network.erl`:
- Around line 45-48: The comment above the macros (e.g., ?DEVICE_TOTAL_CHANNELS,
?GEN_RESPONSE_MS, ?MAX_SHORT_DWELL) contains a typo: change "ate used to
calculate the gen_server:call/3 timeout." to "are used to calculate the
gen_server:call/3 timeout." so the comment reads correctly; update only the
comment text near those macro definitions in network.erl.
- Around line 222-225: The type network_properties() is incorrectly defined as
nested lists while the docstring describes a flat proplist; update the type spec
for network_properties() to be a list of property tuples (i.e., each element is
one of {rssi, dbm()}, {authmode, auth_type()}, {bssid, bssid_t()} or {channel,
wifi_channel()}) so it matches the documented proplist structure and functions
that consume it.
- Around line 576-585: The handler creates a fresh Ref with make_ref() and sends
it to the port, but handle_info later matches against the state's original ref
(State#state.ref), causing replies to be ignored; fix by using the state's
existing ref (e.g., Ref = State#state.ref) when sending the scan request to
network_port instead of make_ref(), and when cancelling an in-flight scan rename
the variable (e.g., From0 or PrevFrom) to reflect that scan_receiver holds a
gen_server From tuple and call gen_server:reply(PrevFrom, {error, canceled})
only when PrevFrom is a From-tuple; finally set scan_receiver = From in the
returned state. Ensure you update the symbols in handle_call({scan, ScanOpts},
From, `#state`{scan_receiver = Scanning} = State) and the send to network_port so
they use State#state.ref and the proper From tuple handling so handle_info's
pattern match on `#state`{ref = Ref} will succeed.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1255-1274: The handler reads and validates the results option
(interop_kv_get_value_default -> cfg_results -> num_results) but never enforces
it when assembling the scan reply, causing all APs to be returned; update the
scan result assembly/response code (where the list of found APs is collected and
port_send_reply is prepared) to truncate or only include up to num_results
(bounded by MAX_SCAN_RESULTS) before sending the reply so the response honors
the requested limit and memory usage is constrained.
- Around line 62-71: The heap-size calculation underestimates SSID capacity
because SSID_MAX_SIZE is defined as 16 while ESP32 SSIDs can be up to 32 bytes
(plus NUL), causing results_size and related allocations to be too small; update
SSID_MAX_SIZE to 33 (32 chars + NUL) and adjust any heap/result-size
calculations that use SSID_MAX_SIZE (e.g., the calculations that produce
results_size) so they account for the larger SSID buffer, and apply the same
change for the duplicated definitions referenced around lines 446-449 to keep
all allocations consistent.
- Around line 1310-1313: The code incorrectly calls
term_invalid_term(term_hidden) even though term_invalid_term takes no arguments;
change the check to compare term_hidden against the sentinel by using
term_invalid_term() (e.g., if ((term_hidden != term_invalid_term()) &&
(term_hidden == TRUE_ATOM)) {...}) so the logic around
interop_kv_get_value_default, the term_hidden variable, and the TRUE_ATOM
comparison remains correct and show_hidden is set appropriately; locate this in
the block using interop_kv_get_value_default, term_hidden, show_hidden, and
ctx->global.
- Around line 428-441: The error path after esp_wifi_scan_get_ap_records uses
sizeof(error) (pointer size) and leaks ap_records; change the call to
term_from_string to use strlen(error) (or strlen(error)+1 if null-termination is
required by term_from_string) instead of sizeof(error), and ensure ap_records
(allocated by calloc) is freed before returning from this error branch; update
the error-handling block around esp_err_to_name, esp_wifi_clear_ap_list,
term_from_string, send_term to free(ap_records) and then return.
- Around line 1325-1357: Check the return value of
esp_wifi_scan_start(scan_config, true): if it fails, free the allocated
wifi_scan_config_t (scan_config), log the error, build an error term with
port_create_error_tuple(ctx, <appropriate error atom>), and send it back with
port_send_reply(ctx, pid, ref, error); also ensure scan_config is freed after a
successful start (or otherwise not leaked) so the allocation is always released.
Use the existing symbols scan_config, esp_wifi_scan_start,
port_create_error_tuple, port_send_reply, pid, ref and ctx to locate and
implement the change.
---
Nitpick comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 233-235: The type for the record field scan_receiver is wrong: it
stores the gen_server:call From value which is a {Pid, Tag} tuple, not a plain
pid(); update the typespec to reflect this (e.g. change scan_receiver :: pid()
to scan_receiver :: {pid(), reference()} or scan_receiver :: {pid(), tag()} and
add a -type tag() :: reference() if you prefer a named alias) so the record and
any callers reflect the actual {pid(), Tag} shape returned by gen_server:call.
- Around line 516-522: Add a -spec for wifi_scan/0 to match the documented
return type used by wifi_scan/1: place a -spec wifi_scan() ->
<same_type_as_wifi_scan/1_spec> above the wifi_scan() function so the zero-arity
clause is documented consistently with wifi_scan/1 (i.e., mirror the exact type
expression used in the existing -spec for wifi_scan/1).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/esp32-build.yamlCHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
💤 Files with no reviewable changes (1)
- .github/workflows/esp32-build.yaml
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
3 times, most recently
from
e288558 to
c97b4f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/platforms/esp32/components/avm_builtins/network_driver.c (1)
439-457:⚠️ Potential issue | 🔴 Critical
ap_recordsleaks on scan result error path.
ap_recordsis allocated at Line 426, but in theesp_wifi_scan_get_ap_recordsfailure branch (Line 440-457) it is never freed before returning.♻️ Suggested fix
if (UNLIKELY_NOT_ESP_OK(err)) { // the ap_list must be cleared on failures to prevent a memory leak esp_wifi_clear_ap_list(); ESP_LOGE(TAG, "Failed to obtain scan results"); const char *error = esp_err_to_name(err); @@ port_send_message_from_task(data->global, term_from_local_process_id(data->owner_process_id), msg); END_WITH_STACK_HEAP(heap, data->global); + free(ap_records); free(data); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 439 - 457, The failure branch after calling esp_wifi_scan_get_ap_records leaks the dynamically allocated ap_records; before sending the error reply and returning in that branch (the block handling UNLIKELY_NOT_ESP_OK(err)), free ap_records (the buffer allocated earlier) immediately after calling esp_wifi_clear_ap_list() and before building/sending the error message and calling free(data). Ensure you reference and free the same pointer variable ap_records so no double-free occurs elsewhere and preserve existing calls to esp_wifi_clear_ap_list(), port_send_message_from_task, and free(data).doc/src/network-programming-guide.md (1)
157-157:⚠️ Potential issue | 🟡 Minor
wifi_scandocs still have correctness typos and a broken anchor.
Line 157 still linkswifi_scan/0to#wifi_scan1, and the tuple example text in that paragraph remains malformed/typoed (discoverd). Line 180 also still says “using and ESP32-C6”. These should be corrected to avoid misleading users.📝 Suggested doc fix
-After the network has been configured for STA mode and started, as long as no connection has been initiated or associated, you may scan for available access points using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan1) or [`network:wifi_scan/1`](./apidocs/erlang/eavmlib/network.md#wifi_scan1). Scanning for access points will temporarily inhibit other traffic on the access point network if it is in use, but should not cause any active connections to be dropped. With no options, a default 'active' scan, with a per-channel dwell time of 120ms will be used and will return network details for up to 6 access points. The return value for the scan takes the form of a tuple consisting of `{ok, Results}`, where `Results = {FoundAPs [NetworkList]}`. `FoundAPs` may be a number larger than the length of the NetworkList if more access points were discoverd than the number of results requested. +After the network has been configured for STA mode and started, as long as no connection has been initiated or associated, you may scan for available access points using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan0) or [`network:wifi_scan/1`](./apidocs/erlang/eavmlib/network.md#wifi_scan1). Scanning for access points will temporarily inhibit other traffic on the access point network if it is in use, but should not cause any active connections to be dropped. With no options, a default active scan with a per-channel dwell time of 120 ms will be used and will return network details for up to 6 access points. The return value for the scan takes the form `{ok, Results}`, where `Results = {FoundAPs, NetworkList}`. `FoundAPs` may be larger than the length of `NetworkList` if more access points were discovered than the number of results requested. @@ -For example to do a passive scan using and ESP32-C6, including hidden networks, using the longest allowed scan time and showing the maximum number of networks available use the following: +For example, to do a passive scan using an ESP32-C6, including hidden networks, using the longest allowed scan time and showing the maximum number of networks available, use the following:Also applies to: 180-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` at line 157, Update the wifi_scan docs to point each function link to its correct anchor (use the correct anchors for wifi_scan/0 and wifi_scan/1 instead of both linking to `#wifi_scan1`), fix the typo "discoverd" → "discovered" in the tuple description, and correct the phrasing "using and ESP32-C6" → "using an ESP32-C6"; locate and change the links and text around the wifi_scan references (wifi_scan/0, wifi_scan/1) and the tuple paragraph to apply these corrections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 224-226: The spec/types for wifi_scan and network_properties() are
inconsistent with the actual payload (code and tests expect maps like #{ssid :=
..., rssi := ...}), so update the typespecs to describe the canonical map shape
instead of tuple lists: modify network_properties() and the wifi_scan type/spec
to reflect a map-based entry with keys such as ssid(), rssi/dbm(),
authmode/auth_type(), bssid/bssid_t(), channel/wifi_channel() and ensure the
wifi_scan() return spec references a list of these maps (so functions like
wifi_scan/0 and consumers like test_wifi_scan.erl match the spec); also apply
the same change for the other duplicated spec locations mentioned (lines
~509-511).
- Around line 642-644: The handle_info clause for handle_info({Ref,
{scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} =
State) currently always replies gen_server:reply(From, {ok, Results}) which
wraps errors as success; change the logic in handle_info (or add
pattern-matching clauses) to detect when Results is an error tuple like {error,
Reason} and reply gen_server:reply(From, {error, Reason}) (otherwise reply {ok,
Results}), preserving the use of State#state{scan_receiver = undefined} and
matching on Ref and scan_receiver so callers receive the correct {ok, ...} or
{error, ...} per the declared API.
- Around line 531-536: The current wifi_scan/0 implementation chains
proplists:get_value(sta, Config) directly and will crash if the sta entry is
missing; first bind Sta = proplists:get_value(sta, Config, []) (or {}), then
derive Results = proplists:get_value(default_scan_results, Sta, 6), Dwell =
proplists:get_value(scan_dwell_ms, Sta, 120), Hidden =
proplists:get_value(scan_show_hidden, Sta, false), Passive =
proplists:get_value(scan_passive, Sta, false) and pass those into wifi_scan/1;
update the code around Config, Results, Dwell, Hidden, Passive in wifi_scan/0 to
use the safe Sta variable with defaults so missing sta does not cause a crash.
- Around line 590-598: In handle_call({scan, ScanOpts}, From, State) the code
mistakenly compares the stored scan_receiver against the incoming From by
binding Scanning to From, so previous callers aren't replied to; change the
logic to detect if State#state.scan_receiver is a previous caller (e.g.,
Scanning = {PrevPid, _Tag}) and, if Scanning =/= From, call
gen_server:reply(Scanning, {error, canceled}) to cancel the earlier requester
before sending the new scan to network_port and updating scan_receiver; update
references in handle_call, scan_receiver, Scanning, From and ensure you still
send network_port ! {self(), Ref, {scan, ScanOpts}} and set
State#state{scan_receiver = From}.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1462-1475: There is a duplicate call to esp_wifi_scan_start which
triggers the scan twice and overwrites the error handling; remove the first
esp_wifi_scan_start(scan_config, true) so that the function only calls
esp_wifi_scan_start once, store its return in err, then free(scan_config) and
run the existing error branch (using esp_err_to_name, port_ensure_available,
term_from_string, port_create_error_tuple, port_create_tuple2, port_send_reply)
to ensure callback/scan state is only cleaned up on the single failure path and
no scan callback state is leaked.
---
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Line 157: Update the wifi_scan docs to point each function link to its correct
anchor (use the correct anchors for wifi_scan/0 and wifi_scan/1 instead of both
linking to `#wifi_scan1`), fix the typo "discoverd" → "discovered" in the tuple
description, and correct the phrasing "using and ESP32-C6" → "using an
ESP32-C6"; locate and change the links and text around the wifi_scan references
(wifi_scan/0, wifi_scan/1) and the tuple paragraph to apply these corrections.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 439-457: The failure branch after calling
esp_wifi_scan_get_ap_records leaks the dynamically allocated ap_records; before
sending the error reply and returning in that branch (the block handling
UNLIKELY_NOT_ESP_OK(err)), free ap_records (the buffer allocated earlier)
immediately after calling esp_wifi_clear_ap_list() and before building/sending
the error message and calling free(data). Ensure you reference and free the same
pointer variable ap_records so no double-free occurs elsewhere and preserve
existing calls to esp_wifi_clear_ap_list(), port_send_message_from_task, and
free(data).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96d38fea-e6a3-436d-a3b6-95fef7c6de7d
📒 Files selected for processing (10)
.github/workflows/esp32-build.yamlCHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
💤 Files with no reviewable changes (1)
- .github/workflows/esp32-build.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/erlang/esp32/wifi_scan.erl
- examples/erlang/esp32/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
There was a problem hiding this comment.
♻️ Duplicate comments (7)
libs/eavmlib/src/network.erl (4)
642-644:⚠️ Potential issue | 🟠 MajorScan errors are returned as
{ok, {error, Reason}}.This clause always wraps result as success; if the driver sends
{scan_results, {error, Reason}}, API contract is violated.Proposed fix
+handle_info({Ref, {scan_results, {error, Reason}}}, `#state`{ref = Ref, scan_receiver = From} = State) -> + gen_server:reply(From, {error, Reason}), + {noreply, State#state{scan_receiver = undefined}}; handle_info({Ref, {scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} = State) -> gen_server:reply(From, {ok, Results}), {noreply, State#state{scan_receiver = undefined}};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 642 - 644, The handle_info clause for handle_info({Ref, {scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} = State) always replies gen_server:reply(From, {ok, Results}) which masks driver errors sent as {scan_results, {error, Reason}}; change the reply logic to inspect Results and reply either {ok, Results} for normal results or propagate the error tuple (e.g., {error, Reason}) unchanged — update the handle_info clause that references `#state.ref` and `#state.scan_receiver` and the gen_server:reply call so errors are not wrapped in {ok, ...} and scan_receiver is still cleared (scan_receiver = undefined).
224-226:⚠️ Potential issue | 🟠 Major
wifi_scantypespecs do not match the actual map-based payload.
network_properties()and bothwifi_scanspecs still describe tuple/proplist entries, but scan consumers and the driver use map entries (#{ssid := ..., rssi := ...}), causing incorrect contracts for callers and dialyzer.Proposed fix
--type network_properties() :: [ - {rssi, dbm()} | {authmode, auth_type()} | {bssid, bssid_t()} | {channel, wifi_channel()} -]. +-type scanned_network() :: #{ + ssid := string(), + rssi := dbm(), + authmode := auth_type(), + bssid := bssid_t(), + channel := wifi_channel() +}. --spec wifi_scan([Options :: scan_options(), ...]) -> - {ok, {NetworksDiscovered :: 0..20, [{SSID :: string(), [ApInfo :: network_properties()]}, ...]}} +-spec wifi_scan([Options :: scan_options(), ...]) -> + {ok, {NetworksDiscovered :: 0..20, [scanned_network()]}} | {error, Reason :: term()}. --spec wifi_scan() -> - {ok, {NetworksDiscovered :: 0..20, [{SSID :: string(), [ApInfo :: network_properties()]}, ...]}} +-spec wifi_scan() -> + {ok, {NetworksDiscovered :: 0..20, [scanned_network()]}} | {error, Reason :: term()}.Also applies to: 509-511, 527-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 224 - 226, The current typespecs network_properties() and the wifi_scan specs (used in functions handling wifi_scan payloads) describe list/tuple/proplist entries but the runtime payloads and driver use maps like #{ssid := ..., rssi := ...}; update the typespecs to reflect map-based structures (e.g., a map type with keys such as ssid, rssi, authmode, bssid, channel) and adjust any related wifi_scan type aliases to match map form so Dialyzer and callers see the correct contract (update all occurrences including the other instances noted around the file).
590-594:⚠️ Potential issue | 🟠 MajorPrevious scan requester is not being canceled.
The branch binds
Scanningto the incomingFrom(= From), so prior waiting callers are usually never replied{error, canceled}.Proposed fix
handle_call({scan, ScanOpts}, From, `#state`{scan_receiver = Scanning, ref = Ref} = State) -> case Scanning of - {Pid, _Tag} = From when is_pid(Pid) -> - gen_server:reply(From, {error, canceled}); + {Pid, _Tag} = PrevFrom when is_pid(Pid), PrevFrom =/= From -> + gen_server:reply(PrevFrom, {error, canceled}); _ -> ok end, network_port ! {self(), Ref, {scan, ScanOpts}}, {noreply, State#state{scan_receiver = From}};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 590 - 594, The branch in handle_call/3 erroneously binds Scanning to the incoming From, so the previous requester stored in the state never gets canceled; change the case to detect when scan_receiver (Scanning) contains a prior From (e.g., a tuple like {PrevFromPid,Tag} or simply PrevFrom) that is different from the incoming From and call gen_server:reply(PrevFrom, {error,canceled}) to notify the previous caller, leaving the other branch for no-op; update the pattern matching in handle_call to compare Scanning against From (use a variable like PrevFrom for the stored value) rather than using "= From".
531-536:⚠️ Potential issue | 🟠 Major
wifi_scan/0can crash when STA config is missing.
proplists:get_value(sta, Config)can beundefined; passing that into subsequentproplists:get_value/3calls can fail instead of returning a clean error/default flow.Proposed fix
wifi_scan() -> Config = gen_server:call(?SERVER, get_config), - Results = proplists:get_value(default_scan_results, proplists:get_value(sta, Config), 6), - Dwell = proplists:get_value(scan_dwell_ms, proplists:get_value(sta, Config), 120), - Hidden = proplists:get_value(scan_show_hidden, proplists:get_value(sta, Config), false), - Passive = proplists:get_value(scan_passive, proplists:get_value(sta, Config), false), + StaCfg = proplists:get_value(sta, Config, []), + Results = proplists:get_value(default_scan_results, StaCfg, 6), + Dwell = proplists:get_value(scan_dwell_ms, StaCfg, 120), + Hidden = proplists:get_value(scan_show_hidden, StaCfg, false), + Passive = proplists:get_value(scan_passive, StaCfg, false), wifi_scan([{results, Results}, {dwell, Dwell}, {show_hidden, Hidden}, {passive, Passive}]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 531 - 536, The current wifi_scan/0 uses proplists:get_value(sta, Config) without a default so StaConfig can be undefined and make subsequent proplists:get_value/3 calls crash; fix by first extracting a safe StaConfig (e.g., StaConfig = proplists:get_value(sta, Config, []) or an empty proplist) and then call proplists:get_value(default_scan_results, StaConfig, 6), proplists:get_value(scan_dwell_ms, StaConfig, 120), proplists:get_value(scan_show_hidden, StaConfig, false), and proplists:get_value(scan_passive, StaConfig, false) before invoking wifi_scan([{results, Results}, {dwell, Dwell}, {show_hidden, Hidden}, {passive, Passive}]).src/platforms/esp32/components/avm_builtins/network_driver.c (2)
1462-1475:⚠️ Potential issue | 🔴 CriticalDuplicate
esp_wifi_scan_startcall breaks scan lifecycle and error handling.The scan is started twice back-to-back; this can produce false error handling and inconsistent callback ownership/cleanup.
Proposed fix
- esp_wifi_scan_start(scan_config, true); - err = esp_wifi_scan_start(scan_config, true); + err = esp_wifi_scan_start(scan_config, true); free(scan_config); if (UNLIKELY(err != ESP_OK)) { const char *err_str = esp_err_to_name(err); size_t error_len = strlen(err_str); port_ensure_available(ctx, error_size + error_len); term reason = term_from_string((const uint8_t *) err_str, (uint16_t) error_len, &ctx->heap); term error = port_create_error_tuple(ctx, reason); term scan_results_atom = make_atom(ctx->global, ATOM_STR("\xc", "scan_results")); term ret = port_create_tuple2(ctx, scan_results_atom, error); port_send_reply(ctx, pid, ref, ret); + esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); + free(data); } return;#!/bin/bash # Verify duplicate start call and nearby failure cleanup in wifi_scan. rg -n -C3 'esp_wifi_scan_start\s*\(scan_config,\s*true\)' src/platforms/esp32/components/avm_builtins/network_driver.c rg -n -C8 'if \(UNLIKELY\(err != ESP_OK\)\)' src/platforms/esp32/components/avm_builtins/network_driver.c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 1462 - 1475, The code calls esp_wifi_scan_start(scan_config, true) twice which breaks the scan lifecycle; remove the first redundant call so esp_wifi_scan_start is invoked only once with its return stored in err, keep the subsequent free(scan_config) and the existing error branch that uses err/err_str/port_ensure_available/term_from_string/port_create_error_tuple/port_send_reply unchanged, and verify no additional cleanup is needed on success (ensure scan_config is freed and ownership/callbacks remain correct).
440-456:⚠️ Potential issue | 🟠 Major
ap_recordsleaks onesp_wifi_scan_get_ap_recordsfailure path.The branch frees
databut notap_records, so repeated scan failures leak heap memory.Proposed fix
if (UNLIKELY_NOT_ESP_OK(err)) { // the ap_list must be cleared on failures to prevent a memory leak esp_wifi_clear_ap_list(); ESP_LOGE(TAG, "Failed to obtain scan results"); const char *error = esp_err_to_name(err); size_t error_len = strlen(error); @@ port_send_message_from_task(data->global, term_from_local_process_id(data->owner_process_id), msg); END_WITH_STACK_HEAP(heap, data->global); + free(ap_records); free(data); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 440 - 456, The failure branch after esp_wifi_scan_get_ap_records leaks the ap_records buffer; before returning in the UNLIKELY_NOT_ESP_OK(err) block free the ap_records pointer (if non-NULL) in addition to freeing data and sending the error reply. Locate the error-handling block that calls esp_wifi_clear_ap_list(), builds the error_reply (using term_from_string, port_heap_create_tuple2, port_send_message_from_task) and add a safe free(ap_records) immediately prior to free(data) and return so the allocated AP list is released on failure.doc/src/network-programming-guide.md (1)
157-186:⚠️ Potential issue | 🟡 MinorWiFi scan docs still contain contract mismatches and broken references.
This section mixes tuple/proplist wording with map-based examples, has malformed return syntax, and links
wifi_scan/0to thewifi_scan/1anchor.Proposed fix
-... using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan1) or ... +... using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan0) or ... -... where `Results = {FoundAPs [NetworkList]}`. +... where `Results = {FoundAPs, NetworkList}`. -... if more access points were discoverd ... +... if more access points were discovered ... -... entries in the `NetworkList` take the form of `{SSID, [AP_Properties]}` ... +... entries in `NetworkList` are maps like `#{ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number}` ... -For example to do a passive scan using and ESP32-C6 ... +For example, to do a passive scan using an ESP32-C6 ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 157 - 186, The docs mix proplist wording with map-based examples, have malformed Erlang syntax, and a wrong anchor for wifi_scan; update the text to consistently describe the scan return as {ok, {Num, NetworkList}} where Num is integer and NetworkList is a list of maps (each map like #{ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number}), fix the example Erlang to valid pattern-matching and iteration (use {ok, {Num, Networks}} = network:wifi_scan(Config) and lists:foreach(fun(Network) -> ... end, Networks) or pattern-match inside the fun), replace any "proplist" wording with "options proplist" only when referring to scan options, correct the link anchors so network:wifi_scan/0 links to the /wifi_scan0 anchor and network:wifi_scan/1 links to the /wifi_scan1 anchor, and ensure references to network:scan_options(), sta_config(), and network:sta_disconnect/0 remain accurate and consistent with the map-based result description.
🧹 Nitpick comments (1)
libs/eavmlib/src/network.erl (1)
917-919: Remove debug print fromget_device_total_channels/0default path.The
io:format/2call introduces noisy stdout side effects during normal scans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 917 - 919, Remove the noisy debug printing in the default branch of get_device_total_channels/0: locate the clause that binds Debug and currently calls io:format("DEBUG: chip info -> ~p", [Debug]) and delete that io:format call (or guard it behind an explicit debug flag if logging is required), leaving the return value {?DEVICE_2_4GHZ_CHANNELS, 5000} unchanged; ensure no other side-effecting prints remain in get_device_total_channels/0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 157-186: The docs mix proplist wording with map-based examples,
have malformed Erlang syntax, and a wrong anchor for wifi_scan; update the text
to consistently describe the scan return as {ok, {Num, NetworkList}} where Num
is integer and NetworkList is a list of maps (each map like #{ssid := SSID, rssi
:= DBm, authmode := Mode, bssid := BSSID, channel := Number}), fix the example
Erlang to valid pattern-matching and iteration (use {ok, {Num, Networks}} =
network:wifi_scan(Config) and lists:foreach(fun(Network) -> ... end, Networks)
or pattern-match inside the fun), replace any "proplist" wording with "options
proplist" only when referring to scan options, correct the link anchors so
network:wifi_scan/0 links to the /wifi_scan0 anchor and network:wifi_scan/1
links to the /wifi_scan1 anchor, and ensure references to
network:scan_options(), sta_config(), and network:sta_disconnect/0 remain
accurate and consistent with the map-based result description.
In `@libs/eavmlib/src/network.erl`:
- Around line 642-644: The handle_info clause for handle_info({Ref,
{scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} =
State) always replies gen_server:reply(From, {ok, Results}) which masks driver
errors sent as {scan_results, {error, Reason}}; change the reply logic to
inspect Results and reply either {ok, Results} for normal results or propagate
the error tuple (e.g., {error, Reason}) unchanged — update the handle_info
clause that references `#state.ref` and `#state.scan_receiver` and the
gen_server:reply call so errors are not wrapped in {ok, ...} and scan_receiver
is still cleared (scan_receiver = undefined).
- Around line 224-226: The current typespecs network_properties() and the
wifi_scan specs (used in functions handling wifi_scan payloads) describe
list/tuple/proplist entries but the runtime payloads and driver use maps like
#{ssid := ..., rssi := ...}; update the typespecs to reflect map-based
structures (e.g., a map type with keys such as ssid, rssi, authmode, bssid,
channel) and adjust any related wifi_scan type aliases to match map form so
Dialyzer and callers see the correct contract (update all occurrences including
the other instances noted around the file).
- Around line 590-594: The branch in handle_call/3 erroneously binds Scanning to
the incoming From, so the previous requester stored in the state never gets
canceled; change the case to detect when scan_receiver (Scanning) contains a
prior From (e.g., a tuple like {PrevFromPid,Tag} or simply PrevFrom) that is
different from the incoming From and call gen_server:reply(PrevFrom,
{error,canceled}) to notify the previous caller, leaving the other branch for
no-op; update the pattern matching in handle_call to compare Scanning against
From (use a variable like PrevFrom for the stored value) rather than using "=
From".
- Around line 531-536: The current wifi_scan/0 uses proplists:get_value(sta,
Config) without a default so StaConfig can be undefined and make subsequent
proplists:get_value/3 calls crash; fix by first extracting a safe StaConfig
(e.g., StaConfig = proplists:get_value(sta, Config, []) or an empty proplist)
and then call proplists:get_value(default_scan_results, StaConfig, 6),
proplists:get_value(scan_dwell_ms, StaConfig, 120),
proplists:get_value(scan_show_hidden, StaConfig, false), and
proplists:get_value(scan_passive, StaConfig, false) before invoking
wifi_scan([{results, Results}, {dwell, Dwell}, {show_hidden, Hidden}, {passive,
Passive}]).
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1462-1475: The code calls esp_wifi_scan_start(scan_config, true)
twice which breaks the scan lifecycle; remove the first redundant call so
esp_wifi_scan_start is invoked only once with its return stored in err, keep the
subsequent free(scan_config) and the existing error branch that uses
err/err_str/port_ensure_available/term_from_string/port_create_error_tuple/port_send_reply
unchanged, and verify no additional cleanup is needed on success (ensure
scan_config is freed and ownership/callbacks remain correct).
- Around line 440-456: The failure branch after esp_wifi_scan_get_ap_records
leaks the ap_records buffer; before returning in the UNLIKELY_NOT_ESP_OK(err)
block free the ap_records pointer (if non-NULL) in addition to freeing data and
sending the error reply. Locate the error-handling block that calls
esp_wifi_clear_ap_list(), builds the error_reply (using term_from_string,
port_heap_create_tuple2, port_send_message_from_task) and add a safe
free(ap_records) immediately prior to free(data) and return so the allocated AP
list is released on failure.
---
Nitpick comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 917-919: Remove the noisy debug printing in the default branch of
get_device_total_channels/0: locate the clause that binds Debug and currently
calls io:format("DEBUG: chip info -> ~p", [Debug]) and delete that io:format
call (or guard it behind an explicit debug flag if logging is required), leaving
the return value {?DEVICE_2_4GHZ_CHANNELS, 5000} unchanged; ensure no other
side-effecting prints remain in get_device_total_channels/0.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec93b3ee-e4cd-425d-ae10-bf3c540ab85e
📒 Files selected for processing (9)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan.erl
- src/platforms/esp32/test/main/test_main.c
- src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
2 times, most recently
from
592914c to
cc88432
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
doc/src/network-programming-guide.md (1)
157-158:⚠️ Potential issue | 🟡 MinorWiFi scan docs are still inconsistent with the implemented API shape.
This section still mixes old tuple/proplist wording with the new map payload, includes a malformed result tuple example, has a wrong
wifi_scan/0anchor, and includes typos (discoverd,using and ESP32-C6).Suggested doc updates
-... [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan1) ... +... [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan0) ... @@ -... `Results = {FoundAPs [NetworkList]}` ... +... `Results = {FoundAPs, NetworkList}` ... @@ -... discoverd ... +... discovered ... @@ -For example to do a passive scan using and ESP32-C6 ... +For example, to do a passive scan using an ESP32-C6 ...Also align the prose with map entries like:
#{ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number}.Also applies to: 180-186
libs/eavmlib/src/network.erl (1)
224-226:⚠️ Potential issue | 🟠 Major
network_properties()typespec does not match the actual scan map entries.Current type wraps entries in an extra list, omits
ssid, and usesauth_type(atom) instead ofauth_type(). This makes thewifi_scan/*specs inaccurate.In Erlang typespecs for map fields, should a local type alias be written as `auth_type()` (with parentheses), and does writing `auth_type` mean only the literal atom `auth_type`?Suggested fix
--type network_properties() :: [ - #{rssi := dbm(), authmode := auth_type, bssid := bssid_t(), channel := wifi_channel()} -]. +-type network_properties() :: + #{ssid := ssid(), rssi := dbm(), authmode := auth_type(), bssid := bssid_t(), channel := wifi_channel()}.Also applies to: 509-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 224 - 226, The typespec network_properties() is wrong: remove the extra list wrapper so it describes a single map (not a list-of-maps), add the missing ssid key, and use the local type alias with parentheses (auth_type() not auth_type) — i.e. change the typedef to a map like #{rssi := dbm(), authmode := auth_type(), bssid := bssid_t(), channel := wifi_channel(), ssid := ssid()} and update any wifi_scan/* specs and the duplicate occurrence (lines around 509-511) to reference this corrected network_properties() type.
🧹 Nitpick comments (1)
libs/eavmlib/src/network.erl (1)
926-932: Remove the unconditional debug print fromget_device_total_channels/0.
io:format("DEBUG: chip info -> ~p", [Debug])is noisy in normal operation and leaks internal runtime details on every scan timeout computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 926 - 932, get_device_total_channels/0 currently prints chip info unconditionally; remove the io:format("DEBUG: chip info -> ~p", [Debug]) call and return the 2.4GHz tuple directly in that clause. Locate the second clause in get_device_total_channels/0 (the one binding Debug) and delete the debug print, optionally rename the variable to _Other or _ to avoid an unused-variable warning, ensuring the clause still returns {?DEVICE_2_4GHZ_CHANNELS, 5000}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 597-605: handle_call/3's current scan cancellation allows
overlapping in-flight scans to share a single Ref and scan_receiver, which can
cause replies to be delivered to the wrong caller
(gen_server:reply(undefined,...)). Fix by serializing scan requests or tying
replies to the originating ref: when accepting a new {scan, ScanOpts} in
handle_call, do not overwrite scan_receiver with just From; instead store the
tuple {From, Ref} (or maintain a queue of pending {From,ScanOpts}) and ensure
incoming results from network_port include the Ref and are only delivered when
Ref matches the stored {From,Ref}; only start the next scan from the queue after
the current scan’s Ref has been replied to (or explicitly increment/rotate Ref
on cancellation so stale in-flight results are ignored). Update all code paths
that set/compare scan_receiver and Ref (including the network_port result
handler and places referenced around handle_call and the code at lines ~649-657)
to use the new {From,Ref} pairing or queue semantics so replies cannot be
misrouted.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 427-437: The early-return error paths inside send_scan_results
currently free data and return without unregistering the scan_done_handler,
leaving a stale callback to freed memory; before freeing data (in the
IS_NULL_PTR(ap_records) path and the other error-paths around lines 439–457)
call the appropriate unregister function to remove the registered
scan_done_handler (e.g., wlan_unregister_scan_done_handler or the module's
unregister_scan_done_handler) using the stored handler reference on data
(scan_done_handler or equivalent) or data->scan_done_handler, then proceed to
free(data) and return.
---
Duplicate comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 224-226: The typespec network_properties() is wrong: remove the
extra list wrapper so it describes a single map (not a list-of-maps), add the
missing ssid key, and use the local type alias with parentheses (auth_type() not
auth_type) — i.e. change the typedef to a map like #{rssi := dbm(), authmode :=
auth_type(), bssid := bssid_t(), channel := wifi_channel(), ssid := ssid()} and
update any wifi_scan/* specs and the duplicate occurrence (lines around 509-511)
to reference this corrected network_properties() type.
---
Nitpick comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 926-932: get_device_total_channels/0 currently prints chip info
unconditionally; remove the io:format("DEBUG: chip info -> ~p", [Debug]) call
and return the 2.4GHz tuple directly in that clause. Locate the second clause in
get_device_total_channels/0 (the one binding Debug) and delete the debug print,
optionally rename the variable to _Other or _ to avoid an unused-variable
warning, ensuring the clause still returns {?DEVICE_2_4GHZ_CHANNELS, 5000}.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a0057b3-3a7b-4372-aee4-acf026e60264
📒 Files selected for processing (9)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl
- examples/erlang/esp32/CMakeLists.txt
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
There was a problem hiding this comment.
♻️ Duplicate comments (4)
libs/eavmlib/src/network.erl (2)
597-605:⚠️ Potential issue | 🔴 CriticalScan cancellation logic can misroute replies and leave unsafe reply targets.
Line 597 cancels the previous caller but still overwrites
scan_receiverbefore the prior in-flight scan result is consumed. With a sharedRef, results can go to the wrong caller, and subsequent messages may hitgen_server:reply/2withundefined.Suggested stabilization (serialize scans)
-handle_call({scan, ScanOpts}, From, `#state`{scan_receiver = Scanning, ref = Ref} = State) -> - case Scanning of - {Pid, _Tag} = Previous when is_pid(Pid) -> - gen_server:reply(Previous, {error, canceled}); - _ -> - ok - end, - network_port ! {self(), Ref, {scan, ScanOpts}}, - {noreply, State#state{scan_receiver = From}}; +handle_call({scan, _ScanOpts}, _From, `#state`{scan_receiver = Scanning} = State) when is_tuple(Scanning) -> + {reply, {error, busy}, State}; +handle_call({scan, ScanOpts}, From, `#state`{scan_receiver = undefined, ref = Ref} = State) -> + network_port ! {self(), Ref, {scan, ScanOpts}}, + {noreply, State#state{scan_receiver = From}}; @@ -handle_info({Ref, {scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} = State) -> +handle_info({Ref, {scan_results, Results}} = _Msg, `#state`{ref = Ref, scan_receiver = From} = State) when is_tuple(From) -> gen_server:reply(From, {ok, Results}), {noreply, State#state{scan_receiver = undefined}}; +handle_info({Ref, {scan_results, _Results}} = _Msg, `#state`{ref = Ref, scan_receiver = undefined} = State) -> + {noreply, State};Also applies to: 649-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 597 - 605, The scan cancellation currently replies to the previous caller and immediately overwrites scan_receiver, risking misrouted in-flight results (shared Ref) and gen_server:reply/2 on undefined; fix by serializing scan requests: generate/attach a fresh unique Ref per scan, send the cancel reply and ensure the previous scan result is consumed or ignored before setting State#state{scan_receiver = From} (e.g. wait for acknowledgement or clear Ref comparison), or alternatively stash incoming scan responses until the new scan_receiver is set and Ref matches; apply the same pattern to the other scan handling block referenced (lines 649-657) to avoid races on scan_receiver, Ref, and gen_server:reply calls.
224-227:⚠️ Potential issue | 🟠 Major
wifi_scantypes/docs are still out of sync with runtime payload.
network_properties()and the function docs/spec text still describe tuple/proplist-shaped entries, but scan results are map entries includingssid. This weakens type guarantees and API clarity.Suggested alignment
--type network_properties() :: [ - #{rssi := dbm(), authmode := auth_type, bssid := bssid_t(), channel := wifi_channel()} -]. -%% A proplist of network properties with the keys: `rssi', `authmode' and `channel' +-type network_properties() :: #{ + ssid := string() | binary(), + rssi := dbm(), + authmode := auth_type(), + bssid := bssid_t(), + channel := wifi_channel() +}. +%% A map describing one discovered network.-%% `{ok, {NumberResults, [{SSID, [{rssi, DBm}, {authmode, Mode}, {bssid, BSSID}, {channel, Number}]}, ...]}}' +%% `{ok, {NumberResults, [#{ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number}, ...]}}'Also applies to: 460-476, 509-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 224 - 227, The declared network_properties() type and related `@spec/docs` describe proplist/tuple entries but runtime scan results are maps containing an ssid key; update network_properties() to a list of maps (e.g. list(map())) with explicit map keys rssi :: dbm(), authmode :: auth_type, bssid :: bssid_t(), channel :: wifi_channel(), ssid :: binary() (or appropriate ssid type), and then update any function specs/docs that reference wifi_scan/network_properties (including functions named wifi_scan, parse_scan_result, or similar at the other affected locations) to use the new map-based type; also change code that pattern-matches or treats entries as tuples/proplists to match maps (e.g., #{ssid := S, rssi := R, ...}) so runtime and types/docs are consistent.src/platforms/esp32/components/avm_builtins/network_driver.c (1)
427-437:⚠️ Potential issue | 🔴 CriticalUnregister
scan_done_handleron early scan-result error exits before freeing callback data.Line 435 and Line 456 free
datawhile the scan-done handler may remain registered on these branches. This can leave a stale callback argument and trigger use-after-free on a laterWIFI_EVENT_SCAN_DONE.Suggested fix
if (IS_NULL_PTR(ap_records)) { BEGIN_WITH_STACK_HEAP(PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2), heap); term error = port_heap_create_tuple2(&heap, ERROR_ATOM, OUT_OF_MEMORY_ATOM); term error_reply = port_heap_create_tuple2(&heap, make_atom(data->global, ATOM_STR("\xc", "scan_results")), error); @@ port_send_message_from_task(data->global, term_from_local_process_id(data->owner_process_id), msg); END_WITH_STACK_HEAP(heap, data->global); + esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); free(data); return; } @@ if (UNLIKELY_NOT_ESP_OK(err)) { // the ap_list must be cleared on failures to prevent a memory leak esp_wifi_clear_ap_list(); @@ port_send_message_from_task(data->global, term_from_local_process_id(data->owner_process_id), msg); END_WITH_STACK_HEAP(heap, data->global); + esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); free(ap_records); free(data); return; }Also applies to: 440-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 427 - 437, The error-handling branches that send an OUT_OF_MEMORY reply (the block that creates error_reply and calls port_send_message_from_task) free the callback data without unregistering the scan_done_handler, which can leave a stale callback and cause use-after-free on WIFI_EVENT_SCAN_DONE; before calling free(data) in these early-exit branches, call the codepath that unregisters the handler (e.g., the same logic used to remove scan_done_handler or deregister the callback that was registered when scheduling the scan) so the handler no longer holds the freed pointer, then proceed to free(data) and return.doc/src/network-programming-guide.md (1)
157-158:⚠️ Potential issue | 🟡 MinorFix wifi_scan docs to match the actual API and payload shape.
Line 157 still has multiple mismatches:
wifi_scan/0points to#wifi_scan1, tuple syntax is malformed, and entries are described as tuple/proplist while current scan results are map-based. This section should be updated to avoid incorrect usage.Suggested doc patch
-After the network has been configured for STA mode and started, as long as no connection has been initiated or associated, you may scan for available access points using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan1) or [`network:wifi_scan/1`](./apidocs/erlang/eavmlib/network.md#wifi_scan1). Scanning for access points will temporarily inhibit other traffic on the access point network if it is in use, but should not cause any active connections to be dropped. With no options, a default 'active' scan, with a per-channel dwell time of 120ms will be used and will return network details for up to 6 access points. The return value for the scan takes the form of a tuple consisting of `{ok, Results}`, where `Results = {FoundAPs [NetworkList]}`. `FoundAPs` may be a number larger than the length of the NetworkList if more access points were discoverd than the number of results requested. The entries in the `NetworkList` take the form of `{SSID, [AP_Properties]}`. `SSID` is the name of the network, and the `AP_Properties` is a proplist with the keys `rssi` for the dBm signal strength of the access point, `authmode` value is the authentication method used by the network, `bssid` (a.k.a MAC address) of the access point, and the `channel` key for obtaining the primary channel for the network. +After the network has been configured for STA mode and started, as long as no connection has been initiated or associated, you may scan for available access points using [`network:wifi_scan/0`](./apidocs/erlang/eavmlib/network.md#wifi_scan0) or [`network:wifi_scan/1`](./apidocs/erlang/eavmlib/network.md#wifi_scan1). Scanning for access points will temporarily inhibit other traffic on the access point network if it is in use, but should not cause active connections to be dropped. With no options, a default active scan with a per-channel dwell time of 120 ms is used and returns details for up to 6 access points. The return value is `{ok, Results}`, where `Results = {FoundAPs, NetworkList}`. `FoundAPs` may be larger than the length of `NetworkList` if more access points were discovered than requested. Entries in `NetworkList` are maps like `#{ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number}`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 157 - 158, The wifi_scan docs are inconsistent with the actual API and payload: fix the broken apidoc anchor references (ensure wifi_scan/0 links to the `#wifi_scan0` anchor and wifi_scan/1 to `#wifi_scan1`), correct the malformed tuple syntax around the {ok, Results} description, and update the payload description to reflect that scan results are map-based (not tuple/proplist). Specifically, describe the return as {ok, Results} where Results includes a total count and a list of AP maps (each map containing keys like ssid, rssi, authmode, bssid, channel), and remove references to tuple/proplist entry forms such as `{SSID, [AP_Properties]}` in favor of the actual map shape used by wifi_scan.
🧹 Nitpick comments (1)
libs/eavmlib/src/network.erl (1)
926-932: Use_for the fallback channel-case variable.Line 930 binds
Debugbut never uses it; replace with_(or_ChipInfo) to avoid compiler warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 926 - 932, The function get_device_total_channels uses a case binding named Debug that is not used; update the fallback clause in the case on erlang:system_info(esp32_chip_info) to replace the unused variable Debug with an underscore (or a named underscore like _ChipInfo) so the pattern becomes #{model := esp32_c5} -> ...; _ -> ... and remove the unused binding to eliminate the compiler warning while keeping the same return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 157-158: The wifi_scan docs are inconsistent with the actual API
and payload: fix the broken apidoc anchor references (ensure wifi_scan/0 links
to the `#wifi_scan0` anchor and wifi_scan/1 to `#wifi_scan1`), correct the malformed
tuple syntax around the {ok, Results} description, and update the payload
description to reflect that scan results are map-based (not tuple/proplist).
Specifically, describe the return as {ok, Results} where Results includes a
total count and a list of AP maps (each map containing keys like ssid, rssi,
authmode, bssid, channel), and remove references to tuple/proplist entry forms
such as `{SSID, [AP_Properties]}` in favor of the actual map shape used by
wifi_scan.
In `@libs/eavmlib/src/network.erl`:
- Around line 597-605: The scan cancellation currently replies to the previous
caller and immediately overwrites scan_receiver, risking misrouted in-flight
results (shared Ref) and gen_server:reply/2 on undefined; fix by serializing
scan requests: generate/attach a fresh unique Ref per scan, send the cancel
reply and ensure the previous scan result is consumed or ignored before setting
State#state{scan_receiver = From} (e.g. wait for acknowledgement or clear Ref
comparison), or alternatively stash incoming scan responses until the new
scan_receiver is set and Ref matches; apply the same pattern to the other scan
handling block referenced (lines 649-657) to avoid races on scan_receiver, Ref,
and gen_server:reply calls.
- Around line 224-227: The declared network_properties() type and related
`@spec/docs` describe proplist/tuple entries but runtime scan results are maps
containing an ssid key; update network_properties() to a list of maps (e.g.
list(map())) with explicit map keys rssi :: dbm(), authmode :: auth_type, bssid
:: bssid_t(), channel :: wifi_channel(), ssid :: binary() (or appropriate ssid
type), and then update any function specs/docs that reference
wifi_scan/network_properties (including functions named wifi_scan,
parse_scan_result, or similar at the other affected locations) to use the new
map-based type; also change code that pattern-matches or treats entries as
tuples/proplists to match maps (e.g., #{ssid := S, rssi := R, ...}) so runtime
and types/docs are consistent.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 427-437: The error-handling branches that send an OUT_OF_MEMORY
reply (the block that creates error_reply and calls port_send_message_from_task)
free the callback data without unregistering the scan_done_handler, which can
leave a stale callback and cause use-after-free on WIFI_EVENT_SCAN_DONE; before
calling free(data) in these early-exit branches, call the codepath that
unregisters the handler (e.g., the same logic used to remove scan_done_handler
or deregister the callback that was registered when scheduling the scan) so the
handler no longer holds the freed pointer, then proceed to free(data) and
return.
---
Nitpick comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 926-932: The function get_device_total_channels uses a case
binding named Debug that is not used; update the fallback clause in the case on
erlang:system_info(esp32_chip_info) to replace the unused variable Debug with an
underscore (or a named underscore like _ChipInfo) so the pattern becomes #{model
:= esp32_c5} -> ...; _ -> ... and remove the unused binding to eliminate the
compiler warning while keeping the same return values.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4de74472-916f-485a-8a90-26adb50538c6
📒 Files selected for processing (9)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/erlang/esp32/CMakeLists.txt
UncleGrumpy
commented
Mar 4, 2026
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
4 times, most recently
from
fc07f45 to
706f93d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
doc/src/network-programming-guide.md (1)
157-158:⚠️ Potential issue | 🟡 MinorA few doc typos remain in the updated scan section.
Examples:
- Line 157:
equivalent toa→equivalent to a- Line 206:
per channel→per-channel- Line 218:
using and ESP32-C6→using an ESP32-C6Also applies to: 206-207, 218-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 157 - 158, Fix the remaining typos in the scan section of the doc: change "equivalent toa" to "equivalent to a" (near the sentence about 10 dBm decrease), change "per channel" to "per-channel" in the occurrence covering lines around 206-207, and change "using and ESP32-C6" to "using an ESP32-C6" (and verify similar phrasing in the nearby 218-220 range); update those exact phrases in the document text to correct spelling and hyphenation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 79-81: Update the documented callback signature for scan_done to
show a single-argument function (it currently lists fun(() -> term())); change
it to fun((term) -> term()) or fun((Result) -> term()) to reflect that the
callback receives the scan results as one argument, and adjust any nearby
example callback code to match the single-argument form; ensure the symbol
referenced is scan_done so readers can find the corrected signature.
In `@examples/erlang/esp32/wifi_scan_callback.erl`:
- Around line 28-34: Check and handle the result of network:start(Config) before
proceeding to scanning: call network:start(Config) and pattern-match its return
(e.g., {ok, State} | {error, Reason}), log/return/exit on {error, Reason}
instead of ignoring it, then only call network:wifi_scan/0 or
network:wifi_scan(ScanOptions) with an explicit scan options map/list (not the
full startup Config) to avoid ambiguity; update the code around the
network:start/1 and network:wifi_scan/1 calls (references: network:start/1,
network:wifi_scan/1, network:wifi_scan/0, Config, Reason) so failures are
handled deterministically and scan arguments are explicit.
In `@libs/eavmlib/src/network.erl`:
- Around line 618-626: The call handler currently always stores From into
scan_receiver causing gen_server:call to block when a scan_done callback
(callback-mode) is configured and the reply is never sent; modify
handle_call({scan, ScanOpts}, From, State) so that when the
configuration/ScanOpts indicate callback-mode (a scan_done pid/fun is set) you
immediately gen_server:reply(From, ok) (or another simple acknowledgement)
instead of leaving From waiting, and do not overwrite scan_receiver with that
blocking From; keep the existing cancellation behavior that
gen_server:reply(Previous, {error, canceled}) for any previous {Pid,_Tag} stored
in scan_receiver, and ensure the network_port ! {self(), Ref, {scan, ScanOpts}}
send still occurs so callback routing (scan_done) continues to deliver results
to the configured callback.
- Around line 83-86: The type scan_done_config() is declared as {scan_done,
fun(() -> term())} but callbacks are invoked with one argument; update the type
to {scan_done, fun((term()) -> term())} (or fun((term()) -> any())) so the spec
matches runtime usage, and ensure references such as scan_done, wifi_scan and
sta_connect use the revised type in their specs/docs.
- Around line 865-867: There are two adjacent EDoc `@private` tags before
get_port/0 causing a "multiple `@private` tags" error; remove the duplicate
comment so only a single "%% `@private`" remains immediately associated with the
get_port/0 function (look for the duplicate tags surrounding get_port/0 in
network.erl and delete the extra one).
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1465-1467: The code calls esp_wifi_scan_start(scan_config, true),
which blocks and prevents the WIFI_EVENT_SCAN_DONE from being posted to your
registered scan_done_handler; change the call to
esp_wifi_scan_start(scan_config, false) so the scan is non-blocking and the
scan_done_handler will receive WIFI_EVENT_SCAN_DONE and deliver results to
Erlang; ensure the existing scan_done_handler registration remains intact and
only free or NULL scan_config if/when it is safe after starting a non-blocking
scan (or after the scan_done_handler has processed results) to avoid freeing
memory still needed by the driver.
---
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 157-158: Fix the remaining typos in the scan section of the doc:
change "equivalent toa" to "equivalent to a" (near the sentence about 10 dBm
decrease), change "per channel" to "per-channel" in the occurrence covering
lines around 206-207, and change "using and ESP32-C6" to "using an ESP32-C6"
(and verify similar phrasing in the nearby 218-220 range); update those exact
phrases in the document text to correct spelling and hyphenation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7bbc217-b222-4e17-b293-ae2b7c0b034b
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl
- src/platforms/esp32/test/main/test_main.c
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan.erl
libs/eavmlib/src/network.erl
Outdated
Comment on lines
865
to
867
| %% @private | ||
|
|
||
| %% @private |
There was a problem hiding this comment.
Remove duplicate @private tag to fix docs pipeline failure.
Line 865 introduces an extra %% @private`` immediately before get_port/0, which causes EDoc failure (`multiple `@private` tags found for get_port/0`).
Proposed patch
-%% `@private`
-
%% `@private`
-spec get_port() -> port().🧰 Tools
🪛 GitHub Actions: Build Docs
[error] 867-867: edoc failed: multiple @private tags found for get_port/0. Resolve by correcting documentation tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/eavmlib/src/network.erl` around lines 865 - 867, There are two adjacent
EDoc `@private` tags before get_port/0 causing a "multiple `@private` tags" error;
remove the duplicate comment so only a single "%% `@private`" remains immediately
associated with the get_port/0 function (look for the duplicate tags surrounding
get_port/0 in network.erl and delete the extra one).
706f93d to
afecd18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
doc/src/network-programming-guide.md (2)
79-81:⚠️ Potential issue | 🟠 Major
scan_donecallback signature is documented with the wrong arity.Line 79 should document a one-argument callback; scan callbacks receive results/error payload.
📝 Suggested doc fix
-* `{scan_done, fun(() -> term())}` A callback function which will be called once a network scan is +* `{scan_done, fun((Result) -> term())}` A callback function which will be called once a network scan is completed, this allows for event driven connection management, and prevents blocking the caller when requesting a scan of available wifi networks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 79 - 81, The documented scan_done callback currently shows a zero-argument fun; update the signature for scan_done to a one-argument callback that accepts the scan result/error payload and adjust the explanatory text accordingly so it states that scan callbacks receive a single argument containing results or an error payload (refer to the scan_done callback symbol when making the change).
157-158:⚠️ Potential issue | 🟡 MinorFix remaining typos in the new scan section.
There are a few text issues that should be cleaned up for clarity.
📝 Suggested doc fix
- power of 1 milliwatt. A 10 dBm decrease in level is equivalent toa ten-fold decrease in signal + power of 1 milliwatt. A 10 dBm decrease in level is equivalent to a ten-fold decrease in signal - a proplist. The per channel scan time can be changed with the `dwell` key, the channel dwell time + a proplist. The per-channel scan time can be changed with the `dwell` key, the channel dwell time - For example, to do a passive scan using and ESP32-C6, including hidden networks, using the longest - allowed scan time and showing the maximum number of networks available use the following: + For example, to do a passive scan using an ESP32-C6, including hidden networks, using the longest + allowed scan time and showing the maximum number of networks available, use the following:Also applies to: 206-206, 218-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 157 - 158, Fix the typos in the "new scan" section text: replace "toa" with "to a" and ensure spacing and sentence flow around "power of 1 milliwatt. A 10 dBm decrease in level is equivalent toa ten-fold decrease in signal power." so it reads cleanly (e.g., "power of 1 milliwatt. A 10 dBm decrease in level is equivalent to a ten-fold decrease in signal power."). Also scan and correct the similar wording issues referenced at the other occurrences (lines noted around 206 and 218–220) to keep consistent phrasing and spacing.src/platforms/esp32/components/avm_builtins/network_driver.c (1)
1465-1467:⚠️ Potential issue | 🔴 CriticalStart scans in non-blocking mode to preserve event-driven completion.
Line 1465 currently starts scans with
block=truewhile this implementation depends onWIFI_EVENT_SCAN_DONE+scan_done_handlerfor result delivery. This can break or stall the reply lifecycle.🔧 Suggested fix
- err = esp_wifi_scan_start(scan_config, true); + err = esp_wifi_scan_start(scan_config, false);ESP-IDF `esp_wifi_scan_start` behavior: when `block=true`, is `WIFI_EVENT_SCAN_DONE` emitted? Compare with `block=false` and confirm recommended mode for event-handler-driven result delivery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 1465 - 1467, The scan is started in blocking mode which prevents the implementation from relying on the WIFI_EVENT_SCAN_DONE + scan_done_handler flow; change the esp_wifi_scan_start call to use non-blocking mode by passing false (esp_wifi_scan_start(scan_config, false)) so results are delivered via WIFI_EVENT_SCAN_DONE and handled by scan_done_handler, and keep the existing free(scan_config); scan_config = NULL pattern after the call to avoid leaks.examples/erlang/esp32/wifi_scan_callback.erl (1)
27-38:⚠️ Potential issue | 🟠 MajorCheck startup result and call
wifi_scanwith scan options (orwifi_scan/0).This sample ignores
network:start/1errors and passes full startup config intowifi_scan/1, which makes the example ambiguous.🔧 Suggested fix
start() -> Config = [{sta, [managed, {scan_done, fun display_scan_results/1}]}], - network:start(Config), - io:format( - "~nStarting active scan with configuration ~p, this may take some time depending on dwell ms used.~n~n", - [Config] - ), - case network:wifi_scan(Config) of - {error, Reason} -> - io:format("wifi_scan failed for reason ~p", [Reason]); - ok -> - timer:sleep(infinity) - end. + case network:start(Config) of + {ok, _Pid} -> + io:format( + "~nStarting active scan with configuration ~p, this may take some time depending on dwell ms used.~n~n", + [Config] + ), + case network:wifi_scan() of + {error, Reason} -> + io:format("wifi_scan failed for reason ~p~n", [Reason]); + ok -> + timer:sleep(infinity) + end; + {error, Reason} -> + io:format("network:start failed for reason ~p~n", [Reason]) + end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/erlang/esp32/wifi_scan_callback.erl` around lines 27 - 38, Check the result of network:start(Config) and handle {ok, _} vs {error, Reason} before calling network:wifi_scan; do not pass the full startup Config into network:wifi_scan/1—call network:wifi_scan/0 or call network:wifi_scan(ScanOptions) with explicit scan options instead. Specifically, update the flow around network:start/1 to pattern-match its return, log or return the error on failure, and on success invoke either network:wifi_scan() or network:wifi_scan(ScanOptions) (keeping display_scan_results/1 as the scan_done callback) so the example is unambiguous and startup failures are not ignored.libs/eavmlib/src/network.erl (2)
83-86:⚠️ Potential issue | 🟠 Major
scan_done_config()type is wrong for actual callback usage.The callback is invoked with one argument (
Results/Error) but declared as arity-0.🔧 Suggested fix
--type scan_done_config() :: {scan_done, fun(() -> term())}. +-type scan_done_config() :: {scan_done, fun((term()) -> term())}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 83 - 86, The declared type scan_done_config() currently specifies a zero-arity fun but the actual callback is called with one argument (Results/Error); update the type to a one-arity function (e.g., {scan_done, fun((term()) -> term())} or fun((term()) -> any())) so it matches how the callback is invoked, and ensure references like sta_connect/1 and wifi_scan/0,1 use the updated scan_done_config() type.
618-626:⚠️ Potential issue | 🔴 Critical
wifi_scan/0,1can timeout whenscan_donecallback is configured.
handle_call({scan, ...})always storesFrom, but callback-enabled branches route results to callback and skipgen_server:reply(From, ...), leaving callers waiting until timeout.🔧 Suggested fix (preserves preemption semantics)
-handle_call({scan, ScanOpts}, From, `#state`{scan_receiver = Scanning, ref = Ref} = State) -> +handle_call({scan, ScanOpts}, From, `#state`{ + scan_receiver = Scanning, ref = Ref, config = Config +} = State) -> case Scanning of {Pid, _Tag} = Previous when is_pid(Pid) -> gen_server:reply(Previous, {error, canceled}); _ -> ok end, network_port ! {self(), Ref, {scan, ScanOpts}}, - {noreply, State#state{scan_receiver = From}}; + case proplists:get_value(scan_done, proplists:get_value(sta, Config, [])) of + undefined -> + {noreply, State#state{scan_receiver = From}}; + _ -> + {reply, ok, State#state{scan_receiver = undefined}} + end; @@ scan_reply_or_callback(Results, `#state`{scan_receiver = From, config = Config} = State) -> case proplists:get_value(scan_done, proplists:get_value(sta, Config, [])) of undefined -> gen_server:reply(From, {ok, Results}); Pid when is_pid(Pid) -> - Pid ! {ok, Results}; + Pid ! {scan_done, Results}; Fun when is_function(Fun) -> spawn(fun() -> Fun(Results) end) end, State#state{scan_receiver = undefined}.Based on learnings: ESP-IDF does not support concurrent scans; initiating a second scan cancels the first and overwriting
scan_receiveris intentional, so this fix should preserve that preemption behavior.Also applies to: 734-753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 618 - 626, The handler handle_call({scan, ScanOpts}, From, `#state`{scan_receiver = Scanning, ref = Ref} = State) stores From into scan_receiver but if a scan_done callback is configured the result is routed to the callback and no gen_server:reply(From, ...) is ever sent, leaving the caller to timeout; fix by detecting when a callback is configured (the scan_done callback in ScanOpts or the module/state that indicates callback usage), and in that branch do not leave the caller waiting — either reply immediately to From (e.g. gen_server:reply(From, {ok, Ref})) and avoid storing scan_receiver, or ensure the code that invokes the callback also calls gen_server:reply(From, ...); update the logic around scan_receiver, network_port ! {self(), Ref, {scan, ScanOpts}} and the subsequent callback/result dispatch so callers are always replied to even when a callback is used, preserving the preemption semantics that overwrite previous scan_receiver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/erlang/esp32/wifi_scan.erl`:
- Around line 26-29: The start/0 function calls network:start([{sta,
[managed]}]) but ignores its return value so scans run even if startup fails;
update start/0 to pattern-match or case on the result of network:start/1 (e.g.,
{ok, Pid} or {error, Reason}) and only call scan_passive/1 and scan_active/1
when network:start/1 succeeds, otherwise log or return the error and abort
further scanning (referencing network:start/1, start/0, scan_passive/1, and
scan_active/1 to locate the change).
In `@libs/eavmlib/src/network.erl`:
- Around line 534-543: The current case on Dwell can crash if Dwell is
non-integer; update the clauses to guard with is_integer/1 for the numeric
comparisons and arithmetic: use a clause like Millis when is_integer(Millis) and
Millis =< ?MAX_SHORT_DWELL to handle short dwells, a clause like ChanDwellMs
when is_integer(ChanDwellMs) to compute Timeout = (ChanDwellMs * NumChannels) +
?GEN_RESPONSE_MS and call gen_server:call(?SERVER, {scan, Options}, Timeout),
and add a fallback clause for any non-integer Dwell (e.g., else ->
gen_server:call(?SERVER, {scan, Options}, DefaultTimeout)) so badarith is
avoided; reference symbols: Dwell, proplists:get_value(dwell, Options),
get_device_total_channels(), ?MAX_SHORT_DWELL, ?GEN_RESPONSE_MS,
gen_server:call.
---
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 79-81: The documented scan_done callback currently shows a
zero-argument fun; update the signature for scan_done to a one-argument callback
that accepts the scan result/error payload and adjust the explanatory text
accordingly so it states that scan callbacks receive a single argument
containing results or an error payload (refer to the scan_done callback symbol
when making the change).
- Around line 157-158: Fix the typos in the "new scan" section text: replace
"toa" with "to a" and ensure spacing and sentence flow around "power of 1
milliwatt. A 10 dBm decrease in level is equivalent toa ten-fold decrease in
signal power." so it reads cleanly (e.g., "power of 1 milliwatt. A 10 dBm
decrease in level is equivalent to a ten-fold decrease in signal power."). Also
scan and correct the similar wording issues referenced at the other occurrences
(lines noted around 206 and 218–220) to keep consistent phrasing and spacing.
In `@examples/erlang/esp32/wifi_scan_callback.erl`:
- Around line 27-38: Check the result of network:start(Config) and handle {ok,
_} vs {error, Reason} before calling network:wifi_scan; do not pass the full
startup Config into network:wifi_scan/1—call network:wifi_scan/0 or call
network:wifi_scan(ScanOptions) with explicit scan options instead. Specifically,
update the flow around network:start/1 to pattern-match its return, log or
return the error on failure, and on success invoke either network:wifi_scan() or
network:wifi_scan(ScanOptions) (keeping display_scan_results/1 as the scan_done
callback) so the example is unambiguous and startup failures are not ignored.
In `@libs/eavmlib/src/network.erl`:
- Around line 83-86: The declared type scan_done_config() currently specifies a
zero-arity fun but the actual callback is called with one argument
(Results/Error); update the type to a one-arity function (e.g., {scan_done,
fun((term()) -> term())} or fun((term()) -> any())) so it matches how the
callback is invoked, and ensure references like sta_connect/1 and wifi_scan/0,1
use the updated scan_done_config() type.
- Around line 618-626: The handler handle_call({scan, ScanOpts}, From,
`#state`{scan_receiver = Scanning, ref = Ref} = State) stores From into
scan_receiver but if a scan_done callback is configured the result is routed to
the callback and no gen_server:reply(From, ...) is ever sent, leaving the caller
to timeout; fix by detecting when a callback is configured (the scan_done
callback in ScanOpts or the module/state that indicates callback usage), and in
that branch do not leave the caller waiting — either reply immediately to From
(e.g. gen_server:reply(From, {ok, Ref})) and avoid storing scan_receiver, or
ensure the code that invokes the callback also calls gen_server:reply(From,
...); update the logic around scan_receiver, network_port ! {self(), Ref, {scan,
ScanOpts}} and the subsequent callback/result dispatch so callers are always
replied to even when a callback is used, preserving the preemption semantics
that overwrite previous scan_receiver.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1465-1467: The scan is started in blocking mode which prevents the
implementation from relying on the WIFI_EVENT_SCAN_DONE + scan_done_handler
flow; change the esp_wifi_scan_start call to use non-blocking mode by passing
false (esp_wifi_scan_start(scan_config, false)) so results are delivered via
WIFI_EVENT_SCAN_DONE and handled by scan_done_handler, and keep the existing
free(scan_config); scan_config = NULL pattern after the call to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57d1a69b-be8f-4a28-8985-9c7d268effb6
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
afecd18 to
6aa0090
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
examples/erlang/esp32/wifi_scan_callback.erl (1)
33-33:⚠️ Potential issue | 🟡 MinorUse scan options (or
wifi_scan/0) instead of startup config.Line 33 passes
Config([{sta, ...}]) towifi_scan/1, but that argument is intended for scan options. This makes the example API usage ambiguous.Proposed fix
- case network:wifi_scan(Config) of + case network:wifi_scan() of🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/erlang/esp32/wifi_scan_callback.erl` at line 33, The example is calling network:wifi_scan(Config) where Config is the startup [{sta,...}] list but wifi_scan/1 expects scan options; change the call to use wifi_scan/0 or build a proper ScanOptions term instead of reusing the startup Config. Locate the network:wifi_scan(Config) invocation and either replace it with network:wifi_scan() or construct a dedicated ScanOptions variable (e.g., ScanOpts) and pass that to network:wifi_scan(ScanOpts), leaving the original Config untouched.doc/src/network-programming-guide.md (1)
79-81:⚠️ Potential issue | 🟠 Major
scan_donecallback signature is still documented with wrong arity.Line 79 documents
fun(() -> term()), but callback is invoked with one argument (scan result or error). This will mislead users.Proposed fix
-* `{scan_done, fun(() -> term())}` A callback function which will be called once a network scan is +* `{scan_done, fun((Result) -> term())}` A callback function which will be called once a network scan is completed, this allows for event-driven connection management, and prevents blocking the caller when requesting a scan of available wifi networks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 79 - 81, The documentation for the scan_done callback incorrectly shows the arity as fun(() -> term()) even though the callback is invoked with one argument; update the description of scan_done to show a unary function type (e.g., fun((ScanResult | Error) -> term())) and mention that the single argument will contain the scan result or an error, so readers looking at the scan_done callback (symbol: scan_done) understand it receives one parameter rather than none.
🧹 Nitpick comments (1)
libs/eavmlib/src/network.erl (1)
217-230:auth_type()is narrower than values emitted by scan results.The driver mapping includes additional auth atoms (e.g., enterprise/DPP variants), but
auth_type()does not include them. This can make documented/typespec return values diverge from runtime payloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 217 - 230, The typespec auth_type() in network.erl is too narrow compared to runtime scan results (it omits additional auth atoms like enterprise/DPP variants emitted by the driver mapping); update the auth_type() type definition to include the missing atoms used by the mapping (e.g., enterprise, dpp, dpp_sa, dpp_proto or whatever exact atoms the driver emits) so the documented return values match runtime payloads (ensure you edit the auth_type() union in libs/eavmlib/src/network.erl to include those symbols and re-run Dialyzer/tests to confirm no mismatches remain).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 204-208: The typespec scan_options() is too narrow: code in
wifi_scan/0 passes tuple boolean forms like {show_hidden, Hidden} and {passive,
Passive}; update scan_options() to include those tuple forms (e.g.,
{show_hidden, boolean()} and {passive, boolean()}) alongside the existing
{results, ...} and {dwell, ...} clauses so the declared type matches actual
usage; locate and modify the scan_options() type declaration in
libs/eavmlib/src/network.erl to add these tuple entries.
- Around line 531-533: The specs for wifi_scan need to allow the atom ok for
callback mode responses; update the -spec declarations for wifi_scan/0 and
wifi_scan/1 (the spec using scan_options() and returning {ok, scan_results()} |
{error, term()}) to also accept ok as a valid return (i.e., return type should
include ok | {ok, scan_results()} | {error, term()}); apply the same change to
the other wifi_scan specs in this module that mirror the same signatures so
callback-mode returns are properly typed.
- Around line 83-86: The public type scan_done_config currently defines the
callback as fun((scan_results()) -> term()), but the scan callback flow can pass
error tuples like {error, Reason}; update the type to accept either successful
scan_results() or error tuples (e.g. fun((scan_results() | {error, term()}) ->
term())) so callers and dialyzer see the correct contract; adjust any related
specs/comments for scan_done_config, and ensure references such as scan_done,
scan_results(), sta_connect/1 and wifi_scan/0,1 reflect the widened input type.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Line 1322: Update the error message emitted by ESP_LOGE(TAG, ...) so it
references the correct exposed API arities: replace "network:wifi_scan/2" with
"network:wifi_scan/0" and "network:wifi_scan/1" (e.g. "network:wifi_scan/0 or
network:wifi_scan/1") so the log accurately reflects the available functions;
locate the ESP_LOGE call near the WiFi configuration check in network_driver.c
and change the string accordingly.
---
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 79-81: The documentation for the scan_done callback incorrectly
shows the arity as fun(() -> term()) even though the callback is invoked with
one argument; update the description of scan_done to show a unary function type
(e.g., fun((ScanResult | Error) -> term())) and mention that the single argument
will contain the scan result or an error, so readers looking at the scan_done
callback (symbol: scan_done) understand it receives one parameter rather than
none.
In `@examples/erlang/esp32/wifi_scan_callback.erl`:
- Line 33: The example is calling network:wifi_scan(Config) where Config is the
startup [{sta,...}] list but wifi_scan/1 expects scan options; change the call
to use wifi_scan/0 or build a proper ScanOptions term instead of reusing the
startup Config. Locate the network:wifi_scan(Config) invocation and either
replace it with network:wifi_scan() or construct a dedicated ScanOptions
variable (e.g., ScanOpts) and pass that to network:wifi_scan(ScanOpts), leaving
the original Config untouched.
---
Nitpick comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 217-230: The typespec auth_type() in network.erl is too narrow
compared to runtime scan results (it omits additional auth atoms like
enterprise/DPP variants emitted by the driver mapping); update the auth_type()
type definition to include the missing atoms used by the mapping (e.g.,
enterprise, dpp, dpp_sa, dpp_proto or whatever exact atoms the driver emits) so
the documented return values match runtime payloads (ensure you edit the
auth_type() union in libs/eavmlib/src/network.erl to include those symbols and
re-run Dialyzer/tests to confirm no mismatches remain).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c95d1cbd-933b-44fc-9af2-2862568ade61
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
- examples/erlang/esp32/CMakeLists.txt
- CHANGELOG.md
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
6aa0090 to
7f4bec5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/platforms/esp32/components/avm_builtins/network_driver.c (1)
1322-1322:⚠️ Potential issue | 🟡 MinorFix incorrect API arity in the scan-mode error log.
Line 1322 references
network:wifi_scan/2, but the exposed API isnetwork:wifi_scan/0andnetwork:wifi_scan/1. This can mislead debugging and support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` at line 1322, The error log message using "network:wifi_scan/2" is incorrect; update the ESP_LOGE call (the one using TAG in network driver error path) to reference the correct API arities "network:wifi_scan/0" and "network:wifi_scan/1" (or a neutral message listing both valid forms) so debugging reflects the actual exposed functions.libs/eavmlib/src/network.erl (2)
204-208:⚠️ Potential issue | 🟡 Minor
scan_options()is narrower than actual call sites.
wifi_scan/0passes{show_hidden, Hidden}and{passive, Passive}(Lines 566-568), butscan_options()only declares atom forms (show_hidden,passive). This mismatch should be aligned to avoid inaccurate specs.Also applies to: 566-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 204 - 208, The spec for scan_options() is too narrow compared to how wifi_scan/0 calls it: update the type definition in network.erl (scan_options()) to accept the tuple forms used at call sites—e.g., include {show_hidden, boolean()} and {passive, boolean()} in addition to the existing {results, ...} and {dwell, ...} forms—so the type matches the actual arguments passed by wifi_scan/0.
83-86:⚠️ Potential issue | 🟡 MinorWiden
scan_donecallback type to include error tuples.The
scan_done_config()type only allowsscan_results(), but runtime callback paths can pass{error, Reason}(e.g., Line 746 onward). The type contract is currently too narrow.Also applies to: 746-753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 83 - 86, The type scan_done_config() is too narrow because the scan_done callback can receive either scan_results() or an error tuple; update the typedef for scan_done_config() (and any related specs) to accept a callback of type fun((scan_results() | {error, term()}) -> term()), and adjust any specs/usages around scan_done, wifi_scan/0, wifi_scan/1 and sta_connect/1 to reflect the broadened callback type so callbacks handling {error, Reason} validate against the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 736-765: The code currently re-reads the routing target from
Config inside scan_reply_or_callback/2 which allows a changing config to divert
or drop replies; instead capture the routing target at the time the scan is
requested and store that snapshot in the state (the existing scan_receiver field
or a new scan_callback field) so delivery uses the saved value rather than
proplists:get_value(scan_done, proplists:get_value(sta, Config, [])) at reply
time. Update the scan initiation path (where scan_receiver is set) to compute
and store the actual callback target (Pid | Fun | {reply, From}) and then change
all clauses of scan_reply_or_callback/2 to dispatch to that stored target and
then clear scan_receiver/scan_callback when finished.
---
Duplicate comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 204-208: The spec for scan_options() is too narrow compared to how
wifi_scan/0 calls it: update the type definition in network.erl (scan_options())
to accept the tuple forms used at call sites—e.g., include {show_hidden,
boolean()} and {passive, boolean()} in addition to the existing {results, ...}
and {dwell, ...} forms—so the type matches the actual arguments passed by
wifi_scan/0.
- Around line 83-86: The type scan_done_config() is too narrow because the
scan_done callback can receive either scan_results() or an error tuple; update
the typedef for scan_done_config() (and any related specs) to accept a callback
of type fun((scan_results() | {error, term()}) -> term()), and adjust any
specs/usages around scan_done, wifi_scan/0, wifi_scan/1 and sta_connect/1 to
reflect the broadened callback type so callbacks handling {error, Reason}
validate against the contract.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Line 1322: The error log message using "network:wifi_scan/2" is incorrect;
update the ESP_LOGE call (the one using TAG in network driver error path) to
reference the correct API arities "network:wifi_scan/0" and
"network:wifi_scan/1" (or a neutral message listing both valid forms) so
debugging reflects the actual exposed functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4ce8b2d-46b0-432a-8495-c4740c0cd5ce
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- examples/erlang/esp32/wifi_scan_callback.erl
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
7f4bec5 to
b037f0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
doc/src/network-programming-guide.md (1)
146-147: Section heading is duplicated later; consider renaming this one.Using a distinct title here will make the document structure easier to scan.
📝 Suggested tweak
-### STA (or AP+STA) mode functions +### STA (or AP+STA) scan-related functions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 146 - 147, The heading "STA (or AP+STA) mode functions" is duplicated later; rename this instance to a distinct title (e.g., "STA (or AP+STA) mode — overview" or "STA (or AP+STA) mode (intro)") to avoid duplicate section headings and improve scanability, and update any internal anchors or references that rely on this heading text if present.examples/erlang/esp32/wifi_scan.erl (1)
41-53: Optional cleanup: extract duplicated network-print logic.Both scan paths format networks identically; a small helper would reduce repetition without changing behavior.
♻️ Proposed refactor
+format_networks(Networks) -> + lists:foreach( + fun( + _Network = #{ + ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number + } + ) -> + io:format( + "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", + [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] + ) + end, + Networks + ). scan_active(Config) -> @@ - lists:foreach( - fun( - _Network = #{ - ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number - } - ) -> - io:format( - "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", - [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] - ) - end, - Networks - ). + format_networks(Networks). scan_passive(Config) -> @@ - lists:foreach( - fun( - _Network = #{ - ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number - } - ) -> - io:format( - "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", - [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] - ) - end, - Networks - ). + format_networks(Networks).Also applies to: 67-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/erlang/esp32/wifi_scan.erl` around lines 41 - 53, Extract the duplicated network-printing anonymous function into a named helper (e.g., print_network/1 or format_and_print_network/1) that accepts a Network map and performs the existing io:format with binary:encode_hex(BSSID) and the same field order; then replace the two lists:foreach anonymous fun usages (the one shown and the similar block at the other occurrence) with lists:foreach(fun print_network/1, Networks) (or equivalent calls) so behavior is unchanged but duplication removed.src/platforms/esp32/components/avm_builtins/network_driver.c (1)
1333-1353: Refactor repeated scan error-reply assembly into one helper.The same
{scan_results, {error, Reason}}construction is repeated in multiple branches; extracting a helper will reduce drift and simplify future changes.♻️ Proposed refactor sketch
+static void send_scan_error_reply(Context *ctx, term pid, term ref, term reason) +{ + term error = port_create_error_tuple(ctx, reason); + term scan_results_atom = make_atom(ctx->global, ATOM_STR("\xc", "scan_results")); + term ret = port_create_tuple2(ctx, scan_results_atom, error); + port_send_reply(ctx, pid, ref, ret); +} @@ - term error = port_create_error_tuple(ctx, BADARG_ATOM); - term scan_results_atom = make_atom(ctx->global, ATOM_STR("\xc", "scan_results")); - term ret = port_create_tuple2(ctx, scan_results_atom, error); - port_send_reply(ctx, pid, ref, ret); + send_scan_error_reply(ctx, pid, ref, BADARG_ATOM); return;Also applies to: 1373-1390, 1412-1418, 1456-1462, 1468-1476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 1333 - 1353, Extract the repeated construction and send of the `{scan_results, {error, Reason}}` reply into a single helper (e.g., send_scan_results_error(ctx, pid, ref, term reason_atom) or send_scan_results_error_badarg(ctx, pid, ref) depending on whether you want generic/reason-specific helpers) and replace every repeated block that calls port_ensure_available, port_create_error_tuple, make_atom("scan_results"), port_create_tuple2 and port_send_reply with a single call to that helper; the helper should take the context, pid and ref, call port_ensure_available(ctx, error_size) as needed, build the error tuple with port_create_error_tuple(ctx, Reason) and make the scan_results atom via make_atom(ctx->global, ATOM_STR("\xc","scan_results")), assemble the final tuple using port_create_tuple2, and call port_send_reply—then update all occurrences around term validation (uses of cfg_results/num_results and the other listed ranges) to use this new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 685-691: handle_info/2 spawns scan_reply_or_callback/2 with a
message tagged {scan_results, ...} but scan_reply_or_callback/2 only matches
{scan_done, Results}, causing a function_clause and timeouts; change
scan_reply_or_callback/2 (or its clause) to accept the {scan_results, Results}
form (or accept both {scan_results, Results} and {scan_done, Results}) so the
spawned process can handle the message and reply/callback correctly; ensure you
update any pattern matches or clauses inside scan_reply_or_callback/2 (and any
callers/guards that assume {scan_done, ...}) to use the {scan_results, ...} tag
consistent with handle_info/2 and the driver.
- Line 251: The scan cancellation/invocation logic is inconsistent: code
sometimes records scan_receiver as the tuple {callback, Fun} but later stores
only FunOrPid (raw fun or pid), so cancellation misses callback cases and
handle_call({scan, ...}) may directly call a fun and crash the server; update
the logic so that scan_receiver is consistently stored as the tuple form
({callback, Fun} or {pid, Pid} or undefined), adjust the
preemption/cancel-on-second-scan path in handle_call({scan, ...}) to match and
compare against the full {callback, Fun} tuple, and when invoking a callback in
handle_call use a protected call (wrap the fun invocation in try...catch or
spawn a monitor/safe apply) so any exception from the callback is caught and
converted to an error reply instead of crashing the server; ensure the
intentional preemption behavior is preserved (do not serialize/queue cancelled
scans).
- Around line 542-546: The short-dwell branch uses a fixed DefaultTimeout which
can be too small for multi-channel scans; change the logic so that even when
Millis =< ?MAX_SHORT_DWELL you compute a channel-based timeout (Timeout =
(Millis * NumChannels) + ?GEN_RESPONSE_MS) and use max(Timeout, DefaultTimeout)
for the gen_server:call instead of always passing DefaultTimeout; update the
clause handling Millis and the clause using ChanDwellMs to both compute and use
this channel-aware timeout (referencing ?MAX_SHORT_DWELL, Millis/ChanDwellMs,
NumChannels, ?GEN_RESPONSE_MS, and the gen_server:call(?SERVER, {scan, Options},
...)).
- Line 757: Replace the invalid erlang:spawn(?MODULE, Fun, Results) call (which
passes a fun where an atom is expected) with the same pattern used later in the
file: wrap the fun invocation in an anonymous zero-arg function and call spawn/1
(or erlang:spawn/1) with fun() -> Fun(Results) end; locate the offending call by
the symbol Fun at the spawn site and mirror the approach used in the block
referenced around lines 833–862.
---
Nitpick comments:
In `@doc/src/network-programming-guide.md`:
- Around line 146-147: The heading "STA (or AP+STA) mode functions" is
duplicated later; rename this instance to a distinct title (e.g., "STA (or
AP+STA) mode — overview" or "STA (or AP+STA) mode (intro)") to avoid duplicate
section headings and improve scanability, and update any internal anchors or
references that rely on this heading text if present.
In `@examples/erlang/esp32/wifi_scan.erl`:
- Around line 41-53: Extract the duplicated network-printing anonymous function
into a named helper (e.g., print_network/1 or format_and_print_network/1) that
accepts a Network map and performs the existing io:format with
binary:encode_hex(BSSID) and the same field order; then replace the two
lists:foreach anonymous fun usages (the one shown and the similar block at the
other occurrence) with lists:foreach(fun print_network/1, Networks) (or
equivalent calls) so behavior is unchanged but duplication removed.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1333-1353: Extract the repeated construction and send of the
`{scan_results, {error, Reason}}` reply into a single helper (e.g.,
send_scan_results_error(ctx, pid, ref, term reason_atom) or
send_scan_results_error_badarg(ctx, pid, ref) depending on whether you want
generic/reason-specific helpers) and replace every repeated block that calls
port_ensure_available, port_create_error_tuple, make_atom("scan_results"),
port_create_tuple2 and port_send_reply with a single call to that helper; the
helper should take the context, pid and ref, call port_ensure_available(ctx,
error_size) as needed, build the error tuple with port_create_error_tuple(ctx,
Reason) and make the scan_results atom via make_atom(ctx->global,
ATOM_STR("\xc","scan_results")), assemble the final tuple using
port_create_tuple2, and call port_send_reply—then update all occurrences around
term validation (uses of cfg_results/num_results and the other listed ranges) to
use this new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 227b0d5c-5c26-4d0b-93c5-2d24cedf78c7
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platforms/esp32/test/main/test_main.c
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
3 times, most recently
from
fbf0d4d to
44e29c1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
doc/src/network-programming-guide.md (1)
79-81:⚠️ Potential issue | 🟡 MinorMinor grammar fix: hyphenate "event-driven".
Suggested fix
-* `{scan_done, fun((scan_results()) -> term())}` A callback function which will be called once a network scan is -completed, this allows for event driven connection management, and prevents blocking the caller +* `{scan_done, fun((scan_results()) -> term())}` A callback function which will be called once a network scan is +completed, this allows for event-driven connection management, and prevents blocking the caller🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/src/network-programming-guide.md` around lines 79 - 81, The phrase describing the callback for `{scan_done, fun((scan_results()) -> term())}` should hyphenate "event-driven" for correct grammar; update the sentence to read "...this allows for event-driven connection management..." so the documentation for the scan callback uses the hyphenated form.
🧹 Nitpick comments (1)
src/platforms/esp32/components/avm_builtins/network_driver.c (1)
232-234: Missingbreakstatement inWIFI_AUTH_MAXcase.The
WIFI_AUTH_MAXcase setsauthmode = ERROR_ATOMbut lacks abreakstatement. While there's no subsequent case to fall through to, adding an explicitbreakmaintains consistency and prevents future issues if cases are reordered.Suggested fix
case WIFI_AUTH_MAX: authmode = ERROR_ATOM; + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 232 - 234, The switch case for WIFI_AUTH_MAX sets authmode = ERROR_ATOM but lacks an explicit break; update the switch in network_driver.c to add a break after the WIFI_AUTH_MAX case (i.e., after the assignment to authmode) so the case uses an explicit "break" to prevent unintended fall-through and future reorder errors; reference symbols: WIFI_AUTH_MAX, authmode, ERROR_ATOM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/src/network-programming-guide.md`:
- Around line 79-81: The phrase describing the callback for `{scan_done,
fun((scan_results()) -> term())}` should hyphenate "event-driven" for correct
grammar; update the sentence to read "...this allows for event-driven connection
management..." so the documentation for the scan callback uses the hyphenated
form.
---
Nitpick comments:
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 232-234: The switch case for WIFI_AUTH_MAX sets authmode =
ERROR_ATOM but lacks an explicit break; update the switch in network_driver.c to
add a break after the WIFI_AUTH_MAX case (i.e., after the assignment to
authmode) so the case uses an explicit "break" to prevent unintended
fall-through and future reorder errors; reference symbols: WIFI_AUTH_MAX,
authmode, ERROR_ATOM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71a3a552-5d1e-4429-b818-dc70a63a12bb
📒 Files selected for processing (11)
.github/workflows/esp32-build.yamlCHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
💤 Files with no reviewable changes (1)
- .github/workflows/esp32-build.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- CHANGELOG.md
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan.erl
- src/platforms/esp32/test/main/test_main.c
Convert aarch64, armv6m, riscv32 and x86_64 backends from list-based register tracking to bitmask-based operations. The main gain is not by replacing O(n) list operations with O(1) bitwise operations (bor, band, bnot) for register allocation/freeing, as n is small, but on the lower impact on gc. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
5b74c1c to
dea08bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl (1)
26-45:⚠️ Potential issue | 🟠 MajorAlways stop the network in an
afterblock.A failed scan or assertion here skips
network:stop/0, which can leave WiFi state behind for later ESP32 tests.Suggested fix
start() -> {ok, _Pid} = network:start([{sta, [managed]}]), - {ok, {Num, Networks}} = network:wifi_scan(), - io:format("network:wifi_scan found ~p networks.~n", [Num]), - lists:foreach( - fun( - _Network = #{ - ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number - } - ) -> - io:format( - "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", - [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] - ) - end, - Networks - ), - true = lists:any(fun(#{ssid := SSID}) -> SSID =:= <<"Wokwi-GUEST">> end, Networks), - ok = network:stop(), - ok. + try + {ok, {Num, Networks}} = network:wifi_scan(), + io:format("network:wifi_scan found ~p networks.~n", [Num]), + lists:foreach( + fun( + _Network = #{ + ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number + } + ) -> + io:format( + "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", + [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] + ) + end, + Networks + ), + true = lists:any(fun(#{ssid := SSID}) -> SSID =:= <<"Wokwi-GUEST">> end, Networks), + ok + after + ok = network:stop() + end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl` around lines 26 - 45, The test's start/0 function currently calls network:start(), runs wifi_scan() and assertions but may skip network:stop() on failures; wrap the scan and assertion logic in a try ... after ... end (or equivalent) so network:stop() is always invoked, i.e., call network:start() first, then use try to perform network:wifi_scan(), the lists:foreach block and the lists:any assertion, and place ok = network:stop() in the after clause to guarantee cleanup even on errors.src/platforms/esp32/components/avm_builtins/network_driver.c (1)
429-437:⚠️ Potential issue | 🟠 MajorRemove
TERM_BINARY_HEAP_SIZE(reason)from line 431.The macro
TERM_BINARY_HEAP_SIZE()expects a byte-count parameter, not a materializedterm. Whenreasonis an atom, small integer, or binary term, passing it directly produces an incorrect heap size calculation. For binary reasons, the caller (e.g., line 1472) pre-allocates the required heap space before creating the binary and passing it here.Suggested fix
static void send_scan_error_reason(Context *ctx, term pid, term ref, term reason) { - size_t error_size = PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2) + TERM_BINARY_HEAP_SIZE(reason); + size_t error_size = PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2); port_ensure_available(ctx, error_size); term error = port_create_error_tuple(ctx, reason); term scan_results_atom = make_atom(ctx->global, ATOM_STR("\xc", "scan_results"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 429 - 437, In send_scan_error_reason remove the incorrect TERM_BINARY_HEAP_SIZE(reason) from the port_ensure_available sizing calculation: the macro expects a byte count not a term, and the caller pre-allocates any binary heap needed, so change the error_size computation in send_scan_error_reason to only include PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2) before calling port_ensure_available; keep the rest of the function (creation of error via port_create_error_tuple, making scan_results_atom, creating ret and port_send_reply) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 228-238: The paragraph incorrectly states scans can be performed
"without starting the network"; update the text to state that
network:wifi_scan/0 and network:wifi_scan/1 require the network gen_server to be
running (network:wifi_scan/1 is a gen_server:call/3 and network:wifi_scan/0 will
return {error, no_sta_configured} if no STA is configured), so instruct readers
to start the driver first (for example using a configuration like [{sta,
[managed]}]) and remove the claim that scans bootstrap the driver; also adjust
the parts about network:sta_disconnect/0 and the disconnected callback to
reflect that scans require an active network server and recommend using a custom
disconnected callback when starting via sta_config().
In `@libs/eavmlib/src/network.erl`:
- Around line 562-574: The code calls get_num_channels_timeout() (which probes
erlang:system_info(esp32_chip_info)) unconditionally from wifi_scan/1, causing
crashes on non-ESP32 targets; change wifi_scan/1 to first guard or safely probe
for esp32_chip_info (e.g., catch or case on erlang:system_info(esp32_chip_info))
and only call get_num_channels_timeout() when that key exists, otherwise fall
back to a safe default timeout/num-channels path so the call proceeds to the
driver and returns "not supported" instead of raising; update references in
wifi_scan/1 and/or get_num_channels_timeout/0 accordingly (ensure
gen_server:call(?SERVER, {scan, Options}, Timeout) still gets a valid Timeout).
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1035-1039: The assignment in the UNLIKELY check is missing
parentheses so err receives the boolean result instead of the actual esp_err_t;
wrap the call in parentheses so err = (esp_event_handler_unregister(WIFI_EVENT,
WIFI_EVENT_SCAN_DONE, &event_handler)) is evaluated first, then compared to
ESP_OK, i.e. change the condition around the esp_event_handler_unregister call
in the UNLIKELY(...) expression (referencing err, esp_event_handler_unregister,
WIFI_EVENT, WIFI_EVENT_SCAN_DONE, event_handler, esp_err_to_name) to match the
other checks in this file.
---
Duplicate comments:
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 429-437: In send_scan_error_reason remove the incorrect
TERM_BINARY_HEAP_SIZE(reason) from the port_ensure_available sizing calculation:
the macro expects a byte count not a term, and the caller pre-allocates any
binary heap needed, so change the error_size computation in
send_scan_error_reason to only include PORT_REPLY_SIZE + TUPLE_SIZE(2) +
TUPLE_SIZE(2) before calling port_ensure_available; keep the rest of the
function (creation of error via port_create_error_tuple, making
scan_results_atom, creating ret and port_send_reply) unchanged.
In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl`:
- Around line 26-45: The test's start/0 function currently calls
network:start(), runs wifi_scan() and assertions but may skip network:stop() on
failures; wrap the scan and assertion logic in a try ... after ... end (or
equivalent) so network:stop() is always invoked, i.e., call network:start()
first, then use try to perform network:wifi_scan(), the lists:foreach block and
the lists:any assertion, and place ok = network:stop() in the after clause to
guarantee cleanup even on errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a0fb681-c73d-4431-98b0-2eccc848f649
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan.erl
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
dea08bf to
a10eddd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 559-567: The function wifi_scan/1 currently raises an exception on
non-ESP32 platforms because the case branch returns the atom
error(unsupported_platform) instead of the tuple {error, unsupported_platform},
which contradicts the declared return spec; update wifi_scan/1 (and the similar
branch at lines referenced around 583-586) so the non-esp32 branch returns
{error, unsupported_platform} (or otherwise return a matching {error, Reason}
tuple) rather than throwing, and ensure the function returns ok or {ok,
scan_results()} on the esp32 path so the implementation matches the advertised
spec.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1373-1377: The driver only treats the scan flags as enabled when
the config value equals TRUE_ATOM, so atom-form flags like the standalone atom
passive or show_hidden are ignored; update the checks around term_passive (and
the analogous term_show_hidden branch) to treat either a boolean true
(TRUE_ATOM) or the corresponding atom name as enabling the option — i.e., if
term_passive is not invalid and (term_passive == TRUE_ATOM || term_passive is
the atom "passive") then set active_scan=false, and similarly treat
term_show_hidden being TRUE_ATOM or the atom "show_hidden" as enabling
show-hidden behavior. Ensure you use the existing term helpers (e.g.,
term_is_atom / term comparison helpers already used elsewhere) and the same
symbol names term_passive, term_show_hidden, active_scan to locate and modify
the code.
- Around line 1446-1463: The scan handler currently allocates a new
ScanClientData and registers scan_done_handler with esp_event_handler_register
each request, leaking previous handlers; change to using
esp_event_handler_instance_register (or
esp_event_handler_instance_register_with) to obtain an
esp_event_handler_instance_t and store that instance handle on the
request/context (add a field on the context that tracks the active scan instance
and its ScanClientData). Before registering a new instance in the function that
creates ScanClientData, check for an existing instance handle and if present
call esp_event_handler_instance_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE,
existing_instance) and free the prior ScanClientData, then overwrite the
instance handle with the new one; ensure you also unregister and free the
instance and ScanClientData on error paths and when scans complete inside
scan_done_handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b662e24-8554-4e2d-b56b-c137d7d76e70
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
- examples/erlang/esp32/CMakeLists.txt
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
3 times, most recently
from
8f395ec to
958387f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/platforms/esp32/components/avm_builtins/network_driver.c (1)
1459-1465:⚠️ Potential issue | 🔴 Critical
ctx->platform_datacan be double-freed here, and it is freed before the old handler is detached.
send_scan_results()frees the activeScanClientData, but none of those completion paths clearsctx->platform_data. On the next scan request this block sees a non-NULL stale pointer, frees it again, and does so before unregisteringscan_done_handler, which leaves a use-after-free window if the old callback is still attached. Please make the scan completion paths clear the tracked pointer beforefree(data), and only free any replacedctx->platform_dataafter the old handler has been detached.Based on learnings: the unregister-before-register replacement flow is intentional, so the fix should preserve that behavior rather than queueing scans or switching to handler instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 1459 - 1465, The code can double-free ctx->platform_data and free it before the old scan_done_handler is unregistered; modify send_scan_results() and any scan-completion paths to clear ctx->platform_data (set to NULL) before calling free(data) so the shared pointer isn't left stale, and change the replacement sequence in the scan-start path so you first call esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler) to detach the old handler, then free the old ctx->platform_data and only after that assign and register the new platform_data/handler; update references to ScanClientData, send_scan_results, ctx->platform_data, scan_done_handler, and esp_event_handler_unregister accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 568-579: The code uses DefaultTimeout when Dwell is undefined,
causing passive scans to time out too early; change the logic in the scan branch
so that when Dwell = undefined and Options specify a passive scan you treat
dwell as the driver's passive default (360 ms/channel), then compute
ComputedTimeout = (ChanDwellMs * NumChannels) + ?GEN_RESPONSE_MS, set Timeout =
erlang:max(DefaultTimeout, ComputedTimeout) and call gen_server:call(?SERVER,
{scan, Options}, Timeout); use get_num_channels_timeout(), the Dwell/Options
value and gen_server:call to locate where to apply this change.
In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl`:
- Around line 26-47: In start() call network:start/1 before invoking
network:wifi_scan/0 and ensure the after clause only calls network:stop/0 if the
server was successfully started; specifically, invoke network:start/1 at the top
of start(), then run network:wifi_scan/0, and adjust the after block to guard
the network:stop/0 call (or track a started flag) so we don't call stop on a
non-existent gen_server.
---
Duplicate comments:
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1459-1465: The code can double-free ctx->platform_data and free it
before the old scan_done_handler is unregistered; modify send_scan_results() and
any scan-completion paths to clear ctx->platform_data (set to NULL) before
calling free(data) so the shared pointer isn't left stale, and change the
replacement sequence in the scan-start path so you first call
esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE,
&scan_done_handler) to detach the old handler, then free the old
ctx->platform_data and only after that assign and register the new
platform_data/handler; update references to ScanClientData, send_scan_results,
ctx->platform_data, scan_done_handler, and esp_event_handler_unregister
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b24cd5f9-3df0-4090-b3a1-266a901a30a8
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- CHANGELOG.md
- examples/erlang/esp32/wifi_scan_callback.erl
- examples/erlang/esp32/wifi_scan.erl
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl (1)
26-47:⚠️ Potential issue | 🔴 CriticalStart the network server before calling
wifi_scan/0.This test never starts
network, sonetwork:wifi_scan/0will fail withnoproc, and theafterclause then callsnetwork:stop/0on the same missing server.Suggested fix
start() -> + {ok, _Pid} = network:start([{sta, [managed]}]), try {ok, {Num, Networks}} = network:wifi_scan(), io:format("network:wifi_scan found ~p networks.~n", [Num]), lists:foreach( fun(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl` around lines 26 - 47, The test's start() calls network:wifi_scan() without starting the network server, causing noproc and then network:stop() in the after block to fail; modify start() to call and await network:start() (or network:start_link()) before invoking network:wifi_scan(), ensure the start result is validated (e.g., {ok, Pid} or ok) and only then call network:wifi_scan(), and keep the existing after clause to call network:stop() so the server is cleanly stopped even on failure.src/platforms/esp32/components/avm_builtins/network_driver.c (1)
483-530:⚠️ Potential issue | 🔴 CriticalClear
ctx->platform_databefore freeing the active scan state.Both the
esp_wifi_scan_get_ap_recordserror path and the success path freedatabut leavectx->platform_datapointing at it. The next scan reaches Line 1463, sees a non-NULLpointer, and frees the same block again.Suggested fix
err = esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); if (UNLIKELY(err != ESP_OK)) { ESP_LOGE(TAG, "Failed to unregister event handler for reason %s, future scans may fail", esp_err_to_name(err)); } else { + data->ctx->platform_data = NULL; free(data); } return; @@ err = esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); if (UNLIKELY_NOT_ESP_OK(err)) { ESP_LOGE(TAG, "Failed to unregister event handler for reason %s, future scans may fail", esp_err_to_name(err)); const char *error = esp_err_to_name(err); @@ port_send_message_from_task(data->global, term_from_local_process_id(data->owner_process_id), msg); END_WITH_STACK_HEAP(heap, data->global); } else { + data->ctx->platform_data = NULL; free(data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 483 - 530, The active-scan state pointer ctx->platform_data is left pointing at freed memory in both the esp_wifi_scan_get_ap_records error path and the successful path where free(data) is called; before any free(data) (references: variable data and ctx->platform_data, and the esp_event_handler_unregister/scan_done_handler blocks), set ctx->platform_data = NULL to avoid a double-free on subsequent scans and then proceed to free(data).libs/eavmlib/src/network.erl (1)
568-599:⚠️ Potential issue | 🟠 MajorPreserve the passive-scan default dwell when callers do not set one.
wifi_scan([passive])still falls back toDefaultTimeout, even though the driver defaults passive scans to360ms/channel, andwifi_scan/0hardcodes120ms even whenscan_passiveis enabled. That makes passive scans either time out too early or run with a much shorter dwell than documented.Suggested fix
wifi_scan(Options) -> case atomvm:platform() of esp32 -> ok; _ -> error(unsupported_platform) end, - Dwell = proplists:get_value(dwell, Options), + Passive = proplists:get_value(passive, Options, false), + Dwell = + case {proplists:get_value(dwell, Options), Passive} of + {undefined, false} -> undefined; + {undefined, _} -> 360; + {Value, _} -> Value + end, {NumChannels, DefaultTimeout} = get_num_channels_timeout(), case Dwell of undefined -> gen_server:call(?SERVER, {scan, Options}, DefaultTimeout); ChanDwellMs when is_integer(ChanDwellMs) -> @@ undefined -> {error, no_sta_configured}; StaCfg -> Results = proplists:get_value(default_scan_results, StaCfg, 6), - Dwell = proplists:get_value(scan_dwell_ms, StaCfg, 120), Hidden = proplists:get_value(scan_show_hidden, StaCfg, false), Passive = proplists:get_value(scan_passive, StaCfg, false), + Dwell = + case proplists:get_value(scan_dwell_ms, StaCfg) of + undefined when Passive -> 360; + undefined -> 120; + Value -> Value + end, wifi_scan([ {results, Results}, {dwell, Dwell}, {show_hidden, Hidden}, {passive, Passive} ]) end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 568 - 599, The passive-scan dwell is not preserved: wifi_scan/0 builds Options with scan_passive but still sets a hardcoded Dwell (120) and wifi_scan/1 treats undefined Dwell by using DefaultTimeout only, causing passive scans to time out or use the wrong per-channel dwell. Update wifi_scan/0 to set {dwell, 360} when scan_passive (scan_passive in StaCfg) is true and no explicit dwell is present, and update the Dwell-handling branch in wifi_scan/1 (the Dwell variable and the case that matches undefined) so that when passive scanning is requested you compute Timeout using the passive per-channel dwell (360 ms) before calling gen_server:call; use get_num_channels_timeout(), the Dwell variable, and the scan_passive option to decide between DefaultTimeout and a computed Timeout so passive scans get the driver-default 360 ms/channel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1455-1459: The ScanClientData instance is not initializing its ctx
field before result paths may dereference it in send_scan_results; set data->ctx
= ctx (the current context pointer) when populating the struct (alongside
data->global, data->owner_process_id, data->ref_ticks, data->num_results) so
that send_scan_results and any cleanup always see a valid ctx; update the
assignment site that constructs ScanClientData to initialize data->ctx before
the struct escapes.
---
Duplicate comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 568-599: The passive-scan dwell is not preserved: wifi_scan/0
builds Options with scan_passive but still sets a hardcoded Dwell (120) and
wifi_scan/1 treats undefined Dwell by using DefaultTimeout only, causing passive
scans to time out or use the wrong per-channel dwell. Update wifi_scan/0 to set
{dwell, 360} when scan_passive (scan_passive in StaCfg) is true and no explicit
dwell is present, and update the Dwell-handling branch in wifi_scan/1 (the Dwell
variable and the case that matches undefined) so that when passive scanning is
requested you compute Timeout using the passive per-channel dwell (360 ms)
before calling gen_server:call; use get_num_channels_timeout(), the Dwell
variable, and the scan_passive option to decide between DefaultTimeout and a
computed Timeout so passive scans get the driver-default 360 ms/channel.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 483-530: The active-scan state pointer ctx->platform_data is left
pointing at freed memory in both the esp_wifi_scan_get_ap_records error path and
the successful path where free(data) is called; before any free(data)
(references: variable data and ctx->platform_data, and the
esp_event_handler_unregister/scan_done_handler blocks), set ctx->platform_data =
NULL to avoid a double-free on subsequent scans and then proceed to free(data).
In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl`:
- Around line 26-47: The test's start() calls network:wifi_scan() without
starting the network server, causing noproc and then network:stop() in the after
block to fail; modify start() to call and await network:start() (or
network:start_link()) before invoking network:wifi_scan(), ensure the start
result is validated (e.g., {ok, Pid} or ok) and only then call
network:wifi_scan(), and keep the existing after clause to call network:stop()
so the server is cleanly stopped even on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59798747-dd42-449f-9cf0-3ba4b80d8654
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/erlang/esp32/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
- examples/erlang/esp32/wifi_scan.erl
- examples/erlang/esp32/wifi_scan_callback.erl
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
958387f to
2662076
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/platforms/esp32/components/avm_builtins/network_driver.c (1)
1464-1468:⚠️ Potential issue | 🟠 Major
ctx->platform_datais cleared before the previous scan context can be reclaimed.Line 1466 guarantees Line 1467 becomes
free(NULL), so every superseded scan leaks itsScanClientData. Please detach the previous pointer before clearing the field; otherwise repeated preempted scans will ratchet heap usage upward. Based on learnings: the unregister-before-register pattern is intentional here; the remaining bug is that the oldScanClientDatais never reclaimed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 1464 - 1468, The code clears ctx->platform_data before freeing the previous ScanClientData, causing free(NULL) and leaking the old scan context; change the sequence to stash the old pointer (e.g., ScanClientData *old = ctx->platform_data), then if old != NULL call esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler), set ctx->platform_data = NULL, and finally free(old) so the previous ScanClientData is reclaimed; refer to ctx->platform_data, ScanClientData, scan_done_handler, esp_event_handler_unregister and free in your fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 166-171: Update the paragraph that restricts scanning to “only
before association”: remove the “no connection has been initiated or associated”
clause and rephrase to state that network:wifi_scan/0 and network:wifi_scan/1
are available once the interface is running in STA or AP+STA mode; also note
that scans may temporarily inhibit traffic on an active network and may affect
associated connections, while keeping the existing explanation about default
active scans. Ensure references to network:wifi_scan/0 and network:wifi_scan/1
and the STA/AP+STA modes remain in the sentence so readers can locate the APIs
and the runtime mode behavior.
In `@libs/eavmlib/src/network.erl`:
- Around line 600-606: The call site in network.erl is forcing a 120ms dwell by
using proplists:get_value(scan_dwell_ms, StaCfg, 120) which overrides
wifi_scan/1’s passive default of 360ms; change the construction so that when
scan_dwell_ms is not present you do not pass a dwell value (e.g., return
undefined or omit the {dwell, ...} tuple) and only include {dwell, Dwell} in the
argument list when scan_dwell_ms exists on StaCfg, while still passing
scan_passive (from StaCfg) to wifi_scan/1 so wifi_scan/1 can apply its own
passive default dwell.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 444-452: The OOM branch that returns early after failing to
allocate wifi_ap_record_t (ap_records) must call esp_wifi_clear_ap_list() to
release scan results held by the Wi‑Fi driver; modify the error path in the
allocation failure block (where ap_records is NULL and the code builds the
ERROR_ATOM reply) to call esp_wifi_clear_ap_list() before composing/sending the
port message so the driver AP list is cleared just like the later failure path
around esp_wifi_scan_get_ap_records().
---
Duplicate comments:
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1464-1468: The code clears ctx->platform_data before freeing the
previous ScanClientData, causing free(NULL) and leaking the old scan context;
change the sequence to stash the old pointer (e.g., ScanClientData *old =
ctx->platform_data), then if old != NULL call
esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE,
&scan_done_handler), set ctx->platform_data = NULL, and finally free(old) so the
previous ScanClientData is reclaimed; refer to ctx->platform_data,
ScanClientData, scan_done_handler, esp_event_handler_unregister and free in your
fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef125e79-077a-4a48-a6a6-3fbe3bb6d463
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/erlang/esp32/wifi_scan_callback.erl
- src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl
- CHANGELOG.md
- src/platforms/esp32/test/main/test_main.c
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
2662076 to
066489c
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/eavmlib/src/network.erl`:
- Line 589: Update the outdated docstring that mentions the nonexistent
sta_config/0: change the comment for the wifi_scan/0 wrapper (the line
referencing `wifi_scan/1`) to indicate it uses default options from the server's
current STA configuration rather than `sta_config/0`; locate the doc around the
`wifi_scan/1`/`wifi_scan/0` definitions in network.erl and replace the stale
reference with wording that explicitly says it reads defaults from the
server-held STA configuration.
- Around line 84-85: The public type scan_done_config() currently only permits a
fun but at runtime the scan delivery accepts either a fun or a pid; update the
type to reflect the runtime contract by widening it to include pid() (e.g.
{scan_done, pid() | fun((scan_results() | {error, Reason :: term()}) ->
term())}) so Dialyzer and users see the valid shapes; apply the same widening to
the equivalent type declarations referenced in the file (the other scan_done_*
type aliases around the other occurrences) so all published types match the
pid-or-fun runtime behavior.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 1465-1469: The cleanup currently nulls ctx->platform_data before
freeing it causing a leak; change the sequence in the teardown for
ScanClientData used by network driver: store the existing ctx->platform_data in
a temporary pointer (e.g., old = ctx->platform_data), unregister the event
handler (esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE,
&scan_done_handler)), set ctx->platform_data = NULL, then call free(old); ensure
you reference the same ScanClientData pointer when freeing so the allocated
memory is released.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d085faab-2d92-42d5-99af-b3c4f69c7107
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan_callback.erl
- doc/src/network-programming-guide.md
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
066489c to
de16582
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl (1)
26-47:⚠️ Potential issue | 🟠 MajorOnly call
network:stop/0after a successful start.
afterstill runs ifnetwork:start/1fails or badmatches, so this test can raisenoprocand hide the actual startup failure.🧪 Guard cleanup behind a successful start
start() -> - try - {ok, _Pid} = network:start([{sta, [managed]}]), - {ok, {Num, Networks}} = network:wifi_scan(), - io:format("network:wifi_scan found ~p networks.~n", [Num]), - lists:foreach( - fun( - _Network = #{ - ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number - } - ) -> - io:format( - "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", - [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] - ) - end, - Networks - ), - true = lists:any(fun(#{ssid := SSID}) -> SSID =:= <<"Wokwi-GUEST">> end, Networks), - ok - after - ok = network:stop() - end. + case network:start([{sta, [managed]}]) of + {ok, _Pid} -> + try + {ok, {Num, Networks}} = network:wifi_scan(), + io:format("network:wifi_scan found ~p networks.~n", [Num]), + lists:foreach( + fun( + _Network = #{ + ssid := SSID, rssi := DBm, authmode := Mode, bssid := BSSID, channel := Number + } + ) -> + io:format( + "Network: ~p, BSSID: ~p, signal ~p dBm, Security: ~p, channel ~p~n", + [SSID, binary:encode_hex(BSSID), DBm, Mode, Number] + ) + end, + Networks + ), + true = lists:any(fun(#{ssid := SSID}) -> SSID =:= <<"Wokwi-GUEST">> end, Networks), + ok + after + ok = network:stop() + end; + {error, Reason} -> + erlang:error({network_start_failed, Reason}) + end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl` around lines 26 - 47, The cleanup in start/0 currently calls network:stop() unconditionally in the after clause which can run even if network:start/1 failed and cause a noproc that masks the real error; change the flow so you only call network:stop() when network:start/1 succeeded (e.g., capture the result of network:start/1 into a variable like {ok, Pid} or {error, _} before entering the block that performs wifi_scan() and only invoke network:stop() when you previously matched {ok, Pid}); update the start/0 function to guard the call to network:stop/0 behind that successful start result (references: start/0, network:start/1, network:stop/0, network:wifi_scan/0).libs/eavmlib/src/network.erl (1)
217-223:⚠️ Potential issue | 🟡 MinorSync this EDoc block with the current per-target scan caps.
The prose here still describes the old “ESP32 classic = 14, others = 20” limits, but
src/platforms/esp32/components/avm_builtins/network_driver.cnow uses 10/14/20/64 depending on target and PSRAM. The type above already allows up to 64, so the documentation is now misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` around lines 217 - 223, Update the EDoc block in network.erl that describes the `results` cap to reflect the current per-target limits used by the network driver (see network_driver.c): document that supported caps are 10/14/20/64 depending on target and PSRAM configuration rather than the old “ESP32 classic = 14, others = 20”; keep the note that the type allows up to 64, preserve the description of `dwell`, `show_hidden`, and defaults `[{results, 6}, {dwell, 120}]` (with passive dwell 360 ms), and explicitly map which targets/PSRAM states correspond to 10, 14, 20, and 64 so readers can find the correct limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 166-167: Remove the stray plus sign leading the word "available"
in the sentence that reads "available access points using" so it no longer
renders as literal text; update the paragraph in
doc/src/network-programming-guide.md (the sentence shown in the diff) to simply
read "available access points using" without the leading "+".
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 440-533: send_scan_results posts the {scan_results,...} message
while ctx->platform_data still points to data, allowing the Erlang side to start
a new scan and free ctx->platform_data (via wifi_scan) before this function
unregisters and frees data, causing a UAF/double-free; fix by unregistering the
scan_done_handler and clearing ctx->platform_data (set data->ctx->platform_data
= NULL) and handling esp_event_handler_unregister errors before calling
port_send_message_from_task or freeing data so that no messages are delivered
while data is still owned, i.e. move the
esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE,
&scan_done_handler) + ctx->platform_data clear/free logic to occur prior to any
port_send_message_from_task in send_scan_results (and do the same ordering in
all error branches).
- Around line 466-507: The code currently uses the potentially-truncated
num_results from esp_wifi_scan_get_ap_records() as the "total discovered" count;
instead call esp_wifi_scan_get_ap_num(&total_discovered) before
esp_wifi_scan_get_ap_records(), fall back to using total_discovered for the
first element of the scan_results tuple while still passing the truncated
num_results/buffer to wifi_ap_records_to_list_maybe_gc for the list, and
handle/report any error from esp_wifi_scan_get_ap_num() similarly to other esp
error checks; update references to num_results/ap_records and the tuple creation
at term scan_results so the head is total_discovered and the list remains the
returned (possibly truncated) records.
---
Duplicate comments:
In `@libs/eavmlib/src/network.erl`:
- Around line 217-223: Update the EDoc block in network.erl that describes the
`results` cap to reflect the current per-target limits used by the network
driver (see network_driver.c): document that supported caps are 10/14/20/64
depending on target and PSRAM configuration rather than the old “ESP32 classic =
14, others = 20”; keep the note that the type allows up to 64, preserve the
description of `dwell`, `show_hidden`, and defaults `[{results, 6}, {dwell,
120}]` (with passive dwell 360 ms), and explicitly map which targets/PSRAM
states correspond to 10, 14, 20, and 64 so readers can find the correct limit.
In `@src/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erl`:
- Around line 26-47: The cleanup in start/0 currently calls network:stop()
unconditionally in the after clause which can run even if network:start/1 failed
and cause a noproc that masks the real error; change the flow so you only call
network:stop() when network:start/1 succeeded (e.g., capture the result of
network:start/1 into a variable like {ok, Pid} or {error, _} before entering the
block that performs wifi_scan() and only invoke network:stop() when you
previously matched {ok, Pid}); update the start/0 function to guard the call to
network:stop/0 behind that successful start result (references: start/0,
network:start/1, network:stop/0, network:wifi_scan/0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 552dfca5-974d-4752-9536-5ac688a46c8a
📒 Files selected for processing (10)
CHANGELOG.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erlsrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.c
🚧 Files skipped from review as they are similar to previous changes (4)
- src/platforms/esp32/test/main/test_main.c
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- examples/erlang/esp32/CMakeLists.txt
- examples/erlang/esp32/wifi_scan.erl
doc/src/network-programming-guide.md
Outdated
Comment on lines
+166
to
+167
| After the network has been configured for STA or AP+STA mode and started, you may scan for | ||
| +available access points using |
There was a problem hiding this comment.
Remove the stray + in this paragraph.
The leading + will render as literal text in the guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/src/network-programming-guide.md` around lines 166 - 167, Remove the
stray plus sign leading the word "available" in the sentence that reads
"available access points using" so it no longer renders as literal text; update
the paragraph in doc/src/network-programming-guide.md (the sentence shown in the
diff) to simply read "available access points using" without the leading "+".
Comment on lines
+466
to
+507
| err = esp_wifi_scan_get_ap_records(&num_results, ap_records); | ||
| if (UNLIKELY_NOT_ESP_OK(err)) { | ||
| // the ap_list must be cleared on failures to prevent a memory leak | ||
| esp_wifi_clear_ap_list(); | ||
| ESP_LOGE(TAG, "Failed to obtain scan results, reason: %s", esp_err_to_name(err)); | ||
| const char *error = esp_err_to_name(err); | ||
| size_t error_len = strlen(error); | ||
|
|
||
| BEGIN_WITH_STACK_HEAP(PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2) + error_len, heap); | ||
| term reason = term_from_string((const uint8_t *) error, (uint16_t) error_len, &heap); | ||
| term error_tuple = port_heap_create_tuple2(&heap, ERROR_ATOM, reason); | ||
| term error_reply = port_heap_create_tuple2(&heap, make_atom(data->ctx->global, ATOM_STR("\xc", "scan_results")), error_tuple); | ||
| term ref = term_from_ref_ticks(data->ref_ticks, &heap); | ||
| term msg = port_heap_create_tuple2(&heap, ref, error_reply); | ||
| port_send_message_from_task(data->ctx->global, term_from_local_process_id(data->owner_process_id), msg); | ||
| END_WITH_STACK_HEAP(heap, data->ctx->global); | ||
| free(ap_records); | ||
| err = esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); | ||
| if (UNLIKELY(err != ESP_OK)) { | ||
| ESP_LOGE(TAG, "Failed to unregister event handler for reason %s, future scans may fail", esp_err_to_name(err)); | ||
| } else { | ||
| data->ctx->platform_data = NULL; | ||
| free(data); | ||
| } | ||
| return; | ||
| } | ||
| ESP_LOGD(TAG, "Scan found %i networks", num_results); | ||
|
|
||
| // return data: {scan_results, {NumberResults, [#{ssid => SSID, rssi => DBM, authmode => Mode, bssid => Bssid, chanel => ChNumber}]}} | ||
| size_t ap_data_size = TUPLE_SIZE(2) + term_map_size_in_terms(5) + TERM_BINARY_HEAP_SIZE(SSID_MAX_SIZE) + TERM_BINARY_HEAP_SIZE(BSSID_SIZE); | ||
| size_t results_size = PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2) + LIST_SIZE(num_results, ap_data_size); | ||
| ESP_LOGD(TAG, "Requesting size '%i' on heap for scan results", results_size); | ||
| BEGIN_WITH_STACK_HEAP(results_size, heap); | ||
|
|
||
| term networks_data_list = term_nil(); | ||
| if (num_results != 0) { | ||
| networks_data_list = wifi_ap_records_to_list_maybe_gc(data->ctx->global, ap_records, num_results, &heap); | ||
| } | ||
| free(ap_records); | ||
| term scan_results = port_heap_create_tuple2(&heap, term_from_int(num_results), networks_data_list); | ||
| term scan_results_atom = make_atom(data->ctx->global, ATOM_STR("\xc", "scan_results")); | ||
| term ret = port_heap_create_tuple2(&heap, scan_results_atom, scan_results); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the network_driver.c file to understand the current implementation
cd src/platforms/esp32/components/avm_builtins && cat -n network_driver.c | sed -n '466,507p'Repository: UncleGrumpy/AtomVM
Length of output: 2934
🏁 Script executed:
# Let's also check the broader context around the wifi_scan_get_ap_records call
cd src/platforms/esp32/components/avm_builtins && cat -n network_driver.c | sed -n '440,520p'Repository: UncleGrumpy/AtomVM
Length of output: 5420
🏁 Script executed:
# Check the documentation file mentioned
cat doc/src/network-programming-guide.md | grep -A 10 -B 5 "truncat\|scan_results" | head -100Repository: UncleGrumpy/AtomVM
Length of output: 1514
🏁 Script executed:
# Check the network.erl file to see how it handles scan results
cd libs/eavmlib/src && cat -n network.erl | grep -A 20 "scan_results"Repository: UncleGrumpy/AtomVM
Length of output: 7654
🏁 Script executed:
# Search for esp_wifi_scan_get_ap_num usage in the codebase
rg "esp_wifi_scan_get_ap_num" --type cRepository: UncleGrumpy/AtomVM
Length of output: 44
🏁 Script executed:
# Search for ScanClientData definition
cd src/platforms/esp32/components/avm_builtins && grep -n "struct ScanClientData\|typedef.*ScanClientData" network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 297
🏁 Script executed:
# Look for where num_results is assigned in ScanClientData
cd src/platforms/esp32/components/avm_builtins && grep -n "ScanClientData\|\.num_results\s*=" network_driver.c | head -30Repository: UncleGrumpy/AtomVM
Length of output: 297
🏁 Script executed:
# Search for the scan request handler to see how num_results is set
cd src/platforms/esp32/components/avm_builtins && grep -n "handle_request\|{scan\|wifi_scan" network_driver.c | head -40Repository: UncleGrumpy/AtomVM
Length of output: 738
🏁 Script executed:
# Look for comments about truncation or AP count limits
cd src/platforms/esp32/components/avm_builtins && grep -n "truncat\|limit\|max.*AP\|discovered\|total" network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 98
🏁 Script executed:
# Look at the struct definition
cd src/platforms/esp32/components/avm_builtins && sed -n '140,160p' network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 674
🏁 Script executed:
# Look at the wifi_scan function where ScanClientData is allocated
cd src/platforms/esp32/components/avm_builtins && sed -n '1449,1485p' network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 1515
🏁 Script executed:
# Check for any ESP-IDF documentation or comments about scan results
cd src/platforms/esp32/components/avm_builtins && grep -B 5 -A 5 "esp_wifi_scan_get_ap_records\|esp_wifi_scan_get_ap_num" network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 470
🏁 Script executed:
# Look for the scan_done_handler to see if there's any truncation logic
cd src/platforms/esp32/components/avm_builtins && sed -n '660,700p' network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 1147
🏁 Script executed:
# Search for any ESP-IDF header files or references to esp_wifi_scan_get_ap_num
find . -type f -name "*.h" | xargs grep -l "esp_wifi_scan_get_ap_num" 2>/dev/nullRepository: UncleGrumpy/AtomVM
Length of output: 46
🏁 Script executed:
# Look for any include statements that might reference ESP-IDF WiFi headers
cd src/platforms/esp32/components/avm_builtins && grep -n "include.*wifi\|esp_wifi.h" network_driver.c | head -20Repository: UncleGrumpy/AtomVM
Length of output: 86
🏁 Script executed:
# Search for how the `results` parameter is passed to wifi_scan and whether it's the limit or expected total
cd src/platforms/esp32/components/avm_builtins && sed -n '1349,1440p' network_driver.cRepository: UncleGrumpy/AtomVM
Length of output: 4022
🏁 Script executed:
# Check if there are any comments or documentation about AP result truncation
rg "truncat|max.*AP|results.*limit" --type cRepository: UncleGrumpy/AtomVM
Length of output: 1570
🏁 Script executed:
# Let's verify if esp_wifi_scan_get_ap_num is a real ESP-IDF function
web_search "esp_wifi_scan_get_ap_num ESP-IDF API"Repository: UncleGrumpy/AtomVM
Length of output: 112
🏁 Script executed:
# Also check for any documentation about ESP-IDF wifi scanning and result truncation
web_search "esp_wifi_scan_get_ap_records truncation maximum results ESP-IDF"Repository: UncleGrumpy/AtomVM
Length of output: 112
🌐 Web query:
esp_wifi_scan_get_ap_num ESP-IDF API documentation
💡 Result:
esp_wifi_scan_get_ap_num() (ESP-IDF Wi‑Fi API) returns the number of access points found in the most recent Wi‑Fi scan:
- Prototype:
esp_err_t esp_wifi_scan_get_ap_num(uint16_t *number) - Purpose: “Get number of APs found in last scan.”
- Important: Call it only after the scan is complete (e.g., after
WIFI_EVENT_SCAN_DONEor after a blockingesp_wifi_scan_start(..., true)), otherwise the value may be wrong. - Returns:
ESP_OK,ESP_ERR_WIFI_NOT_INIT,ESP_ERR_WIFI_NOT_STARTED,ESP_ERR_INVALID_ARG.
[1]
Typical flow described in the Wi‑Fi guide: on WIFI_EVENT_SCAN_DONE, call esp_wifi_scan_get_ap_num(), allocate storage, then call esp_wifi_scan_get_ap_records() (which frees the driver’s stored AP records when called). [2]
Docs page (ESP32, ESP‑IDF v5.1.2) is here:
https://docs.espressif.com/projects/esp-idf/en/v5.1.2/esp32/api-reference/network/esp_wifi.html
Sources: [1] (docs.espressif.com) [2] (espressif-docs.readthedocs-hosted.com)
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/v5.1.2/esp32/api-reference/network/esp_wifi.html
- 2: https://espressif-docs.readthedocs-hosted.com/projects/esp-idf/en/stable/api-guides/wifi.html
🌐 Web query:
esp_wifi_scan_get_ap_records truncation maximum results ESP-IDF
💡 Result:
esp_wifi_scan_get_ap_records() truncates by design: the number argument is [inout]—you pass in the maximum number of wifi_ap_record_t entries your buffer can hold, and the function returns up to that many, updating *number to how many entries it actually wrote. If more APs were found than your input *number, the “extra” APs are simply not returned. [1]
Maximum results: in ESP-IDF’s Wi-Fi scan example docs, the scan list size (DEFAULT_SCAN_LIST_SIZE, from CONFIG_EXAMPLE_SCAN_LIST_SIZE) has a maximum effective value of 64—setting it higher won’t yield more than 64 records. [2]
If you want to detect truncation, first call esp_wifi_scan_get_ap_num() to get the total AP count, then call esp_wifi_scan_get_ap_records() with a buffer size up to 64, and compare “total found” vs “returned.” [1][2]
Sources:
[1] ESP-IDF Wi-Fi API reference (esp_wifi_scan_get_ap_records() parameter semantics) (docs.espressif.com)
[2] ESP-Techpedia “Scan Example” (max effective scan list size = 64) (docs.espressif.com)
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/v5.1.2/esp32/api-reference/network/esp_wifi.html
- 2: https://docs.espressif.com/projects/esp-techpedia/en/latest/esp-friends/get-started/case-study/wifi-examples/scan-example.html
Report the total discovered AP count in the tuple, not just the returned records.
The esp_wifi_scan_get_ap_records() function truncates results silently: the num_results parameter is [inout]—you pass the buffer capacity as input, and it returns the actual records written. The current code reports {returned_count, network_list} in the tuple, but the type contract in libs/eavmlib/src/network.erl line 253 ({NetworksDiscovered :: 0..64, ...}) and the variable naming indicate the first element should be the total discovered count, even when results are truncated.
Retrieve the total discovered count using esp_wifi_scan_get_ap_num() before esp_wifi_scan_get_ap_records(), then use the total in the tuple head while the list remains truncated to the buffer limit.
📏 Use the total discovered count in the tuple head
+ uint16_t found_aps = 0;
+ err = esp_wifi_scan_get_ap_num(&found_aps);
+ if (UNLIKELY_NOT_ESP_OK(err)) {
+ ...
+ }
+
err = esp_wifi_scan_get_ap_records(&num_results, ap_records);
...
- term scan_results = port_heap_create_tuple2(&heap, term_from_int(num_results), networks_data_list);
+ term scan_results = port_heap_create_tuple2(&heap, term_from_int(found_aps), networks_data_list);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines
466 - 507, The code currently uses the potentially-truncated num_results from
esp_wifi_scan_get_ap_records() as the "total discovered" count; instead call
esp_wifi_scan_get_ap_num(&total_discovered) before
esp_wifi_scan_get_ap_records(), fall back to using total_discovered for the
first element of the scan_results tuple while still passing the truncated
num_results/buffer to wifi_ap_records_to_list_maybe_gc for the list, and
handle/report any error from esp_wifi_scan_get_ap_num() similarly to other esp
error checks; update references to num_results/ap_records and the tuple creation
at term scan_results so the head is total_discovered and the list remains the
returned (possibly truncated) records.
Owner
Author
There was a problem hiding this comment.
When esp_wifi_scan_get_ap_records is called num_results holds the number of networks that should be returned, after esp_wifi_scan_get_ap_records returns an esp_err_t num_results holds the value representing the total number of networks found, there is no need to use both esp_wifi_scan_get_ap_num and esp_wifi_scan_get_ap_records. Typically esp_wifi_scan_get_ap_num is used when retreiving results one-by-one with esp_wifi_scan_get_ap_record.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
esp_wifi_scan_get_ap_records num_results inout total found after call ESP-IDF
💡 Result:
esp_wifi_scan_get_ap_records(uint16_t *number, wifi_ap_record_t *ap_records) uses number as an in/out parameter:
- On input (
*number): set it to the capacity of yourap_recordsarray (max records you want copied). [1][2] - On output (
*number): it becomes the actual number of records copied into your array (i.e.,min(capacity, total_found)). [1][2]
If you need the total number of APs found by the scan, call:
esp_wifi_scan_get_ap_num(&total_found)after the scan completes. [1][2]
Also note: calling esp_wifi_scan_get_ap_records() will free the internal memory used to hold the scanned AP list, so you generally call it once per scan result (or otherwise clear the list). [2]
Typical pattern
uint16_t total = 0;
ESP_ERROR_CHECK(esp_wifi_scan_get_ap_num(&total));
uint16_t n = MIN(total, MAX_APS);
wifi_ap_record_t aps[MAX_APS];
ESP_ERROR_CHECK(esp_wifi_scan_get_ap_records(&n, aps));
// here: total = total found; n = actually copiedSources: ESP-IDF Wi-Fi API reference / esp_wifi.h. [1][2]
@UncleGrumpy, thank you for the clarification, but the ESP-IDF documentation actually describes the opposite behavior. According to the API reference, esp_wifi_scan_get_ap_records() uses number as a true in/out parameter where:
- On input: the capacity of your
ap_recordsbuffer (the max records to copy) - On output: the actual number of records copied — i.e.,
min(capacity, total_found)
If you need the total number of APs found by the scan, you must call esp_wifi_scan_get_ap_num(&total_found) after the scan completes.
The typical pattern described in the documentation is:
uint16_t total = 0;
ESP_ERROR_CHECK(esp_wifi_scan_get_ap_num(&total));
uint16_t n = MIN(total, MAX_APS);
wifi_ap_record_t aps[MAX_APS];
ESP_ERROR_CHECK(esp_wifi_scan_get_ap_records(&n, aps));
// here: total = total found; n = actually copiedSo if the user's buffer holds 6 records but 12 APs were discovered, num_results after the call would be 6 (the copied count), not 12 (the total discovered). The current code would then report {6, [...6 networks...]}, but the Erlang spec names the first element NetworksDiscovered, suggesting it should be 12.
The suggestion to call esp_wifi_scan_get_ap_num() before esp_wifi_scan_get_ap_records() to correctly populate the NetworksDiscovered field in the response tuple remains valid.
🧠 Learnings used
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: src/platforms/esp32/components/avm_builtins/network_driver.c:1446-1468
Timestamp: 2026-03-09T00:49:00.445Z
Learning: In `src/platforms/esp32/components/avm_builtins/network_driver.c`, the `wifi_scan` function intentionally calls `esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler)` before registering a new handler for each scan request. No error check on unregister is needed because a failure only indicates no handler was previously registered. The Erlang gen_server is responsible for sending `{error, canceled}` to the previous caller on the Erlang side. This design is intentional and should not be changed to use `esp_event_handler_instance_register`.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: libs/eavmlib/src/network.erl:597-605
Timestamp: 2026-03-04T07:09:26.484Z
Learning: In `libs/eavmlib/src/network.erl`, the scan cancellation behavior in `handle_call({scan, ...})` is intentional: ESP-IDF does not support concurrent WiFi scans; initiating a second scan cancels the first at the platform level. The gen_server mirrors this by immediately replying `{error, canceled}` to the previous caller and overwriting `scan_receiver` with the new caller. This is by design and should not be changed to serialize/queue scans.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: libs/eavmlib/src/network.erl:559-567
Timestamp: 2026-03-08T23:27:29.576Z
Learning: In `libs/eavmlib/src/network.erl`, `wifi_scan/1` (and `wifi_scan/0`) intentionally calls `error(unsupported_platform)` — raising an Erlang exception — on non-ESP32 platforms rather than returning `{error, unsupported_platform}`. This is explicitly documented in the function's edoc ("will raise an `unsupported_platform` error") and is a deliberate design decision. Do not flag this as a spec mismatch or suggest changing it to a return-value error; the throwing behaviour is by design.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: examples/erlang/esp32/wifi_scan_callback.erl:28-34
Timestamp: 2026-03-04T11:01:37.985Z
Learning: In the `UncleGrumpy/AtomVM` repository, example files under `examples/erlang/esp32/` are intentionally minimal and focused on demonstrating a single feature or function. They are not expected to be robust applications, so missing error handling (e.g., unchecked return values of `network:start/1`) in these files is by design and should not be flagged in reviews.
JIT: convert all backends to bitmask-based register tracking 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
…ctions Add missing declarations for nifs/bifs in erlang module 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
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
de16582 to
a0401fa
Compare
Corrects the type name db() to the more correct `dbm()`, and adds a brief edoc explanation for the value. Signed-off-by: Winford <winford@object.stream>
ESP-IDF 5.1 has been end-of-life since December 2025 and will not receive future security patches. Remove it from the workflow test matrix. Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
2 times, most recently
from
10e731b to
e1b68de
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| BEGIN_WITH_STACK_HEAP(PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2), heap); | ||
| term error = port_heap_create_tuple2(&heap, ERROR_ATOM, OUT_OF_MEMORY_ATOM); | ||
| term error_reply = port_heap_create_tuple2(&heap, make_atom(ctx->global, ATOM_STR("\xc", "scan_results")), error); | ||
| term ref = term_from_ref_ticks(ref_ticks, &heap); |
Check failure
Code scanning / CodeQL
Passing a term to a function expecting a non-term
| term reason = term_from_string((const uint8_t *) error, (uint16_t) error_len, &heap); | ||
| term error_tuple = port_heap_create_tuple2(&heap, ERROR_ATOM, reason); | ||
| term error_reply = port_heap_create_tuple2(&heap, make_atom(ctx->global, ATOM_STR("\xc", "scan_results")), error_tuple); | ||
| term ref = term_from_ref_ticks(ref_ticks, &heap); |
Check failure
Code scanning / CodeQL
Passing a term to a function expecting a non-term
| term scan_results = port_heap_create_tuple2(&heap, term_from_int(num_results), networks_data_list); | ||
| term scan_results_atom = make_atom(ctx->global, ATOM_STR("\xc", "scan_results")); | ||
| term ret = port_heap_create_tuple2(&heap, scan_results_atom, scan_results); | ||
| term ref = term_from_ref_ticks(ref_ticks, &heap); |
Check failure
Code scanning / CodeQL
Passing a term to a function expecting a non-term
| BEGIN_WITH_STACK_HEAP(PORT_REPLY_SIZE + TUPLE_SIZE(2) + TUPLE_SIZE(2) + error_len, heap); | ||
| term reason = term_from_string((const uint8_t *) error, (uint16_t) error_len, &heap); | ||
| term error_reply = port_heap_create_tuple2(&heap, ERROR_ATOM, reason); | ||
| term ref = term_from_ref_ticks(ref_ticks, &heap); |
Check failure
Code scanning / CodeQL
Passing a term to a function expecting a non-term
Signed-off-by: Winford <winford@object.stream>
Adds documentation for `network:wifi_scan/0,1` and updates details for `network:sta_rssi/0`. Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
esp32_wifi_scan
branch
from
e1b68de to
dfd3ff4
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/platforms/esp32/components/avm_builtins/i2c_resource.c (1)
219-237:⚠️ Potential issue | 🟠 MajorAvoid deleting the I2C driver after
objhas taken ownership.After Line 225, the resource dtor is another teardown path for the same port. The second OOM branch at Lines 234-237 still calls
i2c_driver_delete(i2c_num), so a later GC of the discarded resource can delete the port a second time and potentially tear down a newly reopened driver.🧰 Proposed fix
- if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) != MEMORY_GC_OK)) { + size_t requested_size = TERM_BOXED_RESOURCE_SIZE + TUPLE_SIZE(3) + REF_SIZE; + if (UNLIKELY(memory_ensure_free(ctx, requested_size) != MEMORY_GC_OK)) { i2c_driver_delete(i2c_num); enif_release_resource(rsrc_obj); ESP_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } term obj = term_from_resource(rsrc_obj, &ctx->heap); enif_release_resource(rsrc_obj); @@ - size_t requested_size = TUPLE_SIZE(3) + REF_SIZE; - if (UNLIKELY(memory_ensure_free_with_roots(ctx, requested_size, 1, &obj, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { - i2c_driver_delete(i2c_num); - ESP_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); - RAISE_ERROR(OUT_OF_MEMORY_ATOM); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/i2c_resource.c` around lines 219 - 237, The second OOM path is deleting the I2C driver after ownership of the resource has already been transferred to obj (via term_from_resource + enif_release_resource), risking a double-delete when the resource dtor later runs; to fix, remove the i2c_driver_delete(i2c_num) call from the failure branch that follows memory_ensure_free_with_roots (or guard it behind an explicit ownership check) so that once obj has been formed we no longer delete the driver there; keep the earlier i2c_driver_delete in the pre-object OOM branch (the one after memory_ensure_free) intact.src/platforms/esp32/components/avm_builtins/dac_driver.c (1)
113-138:⚠️ Potential issue | 🔴 CriticalReserve and root
chan_objbefore constructing the return tuple.After Line 118, this function allocates the resource term on the process heap, but the success path at Lines 129-138 still builds
{'$dac', ..., Ref}without a correspondingmemory_ensure_free_with_roots. Under memory pressure that can corrupt the heap or drop the boxed resource across GC.🧱 Proposed fix
- ERL_NIF_TERM chan_obj = term_from_resource(chan_rsrc, &ctx->heap); + term chan_obj = term_from_resource(chan_rsrc, &ctx->heap); @@ enif_release_resource(chan_rsrc); if (!err) { + size_t requested_size = TUPLE_SIZE(2) + TUPLE_SIZE(3) + REF_SIZE; + if (UNLIKELY(memory_ensure_free_with_roots(ctx, requested_size, 1, &chan_obj, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } term chan_tup = term_alloc_tuple(3, &ctx->heap); term_put_tuple_element(chan_tup, 0, globalcontext_make_atom(ctx->global, ATOM_STR("\x4", "$dac"))); term_put_tuple_element(chan_tup, 1, chan_obj);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/dac_driver.c` around lines 113 - 138, Reserve and root the boxed resource term before building the return tuple: replace the plain memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) + term_from_resource(chan_rsrc, &ctx->heap) sequence with a call to memory_ensure_free_with_roots (or equivalent) that both ensures space and registers chan_obj as a root, then call term_from_resource(chan_rsrc, &ctx->heap) into that rooted slot; after that proceed to call dac_oneshot_new_channel and build the tuple with term_alloc_tuple/term_put_tuple_element as before, keeping enif_release_resource(chan_rsrc) and the existing error path intact so the resource is rooted during any GC while constructing the {'$dac', ..., Ref} tuple.src/platforms/esp32/components/avm_builtins/adc_driver.c (1)
497-503:⚠️ Potential issue | 🔴 CriticalFix the misplaced
!= MEMORY_GC_OKcomparison.Line 497 passes
TERM_BOXED_RESOURCE_SIZE != MEMORY_GC_OKas the size argument tomemory_ensure_free, so only 0 or 1 byte is reserved instead of the requiredTERM_BOXED_RESOURCE_SIZE. Whenterm_from_resourceallocates on the heap at line 503, this causes heap corruption or out-of-memory failure due to insufficient reservation.Proposed fix
- if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE != MEMORY_GC_OK))) { + if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) != MEMORY_GC_OK)) { enif_release_resource(chan_rsrc); ESP_LOGE(TAG, "failed to allocate memory for resource: %s:%i.", __FILE__, __LINE__); RAISE_ERROR(OUT_OF_MEMORY_ATOM); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/adc_driver.c` around lines 497 - 503, The call to memory_ensure_free currently passes the boolean result of the comparison (TERM_BOXED_RESOURCE_SIZE != MEMORY_GC_OK) as the size, reserving 0/1 bytes and causing heap corruption when term_from_resource allocates; update the condition to call memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) and compare its return value to MEMORY_GC_OK (i.e., if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) != MEMORY_GC_OK)) { ... }) so the correct number of bytes is reserved before using term_from_resource with chan_rsrc and ctx->heap.src/libAtomVM/posix_nifs.c (1)
849-855:⚠️ Potential issue | 🟠 MajorRelease
dir_objbefore raising on OOM.If the heap check fails here, the newly allocated resource still owns an open
DIR *. Raising withoutenif_release_resource(dir_obj)leaks the handle.💡 Minimal fix
if (UNLIKELY(memory_ensure_free_opt( ctx, TUPLE_SIZE(2) + TERM_BOXED_REFERENCE_RESOURCE_SIZE, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) { + enif_release_resource(dir_obj); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } term obj = term_from_resource(dir_obj, &ctx->heap); enif_release_resource(dir_obj); // decrement refcount after enif_alloc_resource🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libAtomVM/posix_nifs.c` around lines 849 - 855, The current code calls memory_ensure_free_opt() and immediately RAISE_ERROR(OUT_OF_MEMORY_ATOM) on failure while dir_obj still owns the DIR*, causing a resource leak; modify the failure path in the block that calls memory_ensure_free_opt() (the code invoking term_from_resource and using dir_obj) to call enif_release_resource(dir_obj) before raising the OUT_OF_MEMORY_ATOM error so the resource reference is decremented and the DIR* is closed.src/libAtomVM/otp_socket.c (1)
640-645:⚠️ Potential issue | 🟠 MajorRelease the resource on the new OOM branches.
These paths now raise before
enif_release_resource(...), so an OOM during boxing leaks the freshly created socket resource and its underlying fd/PCB.💡 Minimal cleanup patch
if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_REFERENCE_RESOURCE_SIZE) != MEMORY_GC_OK)) { AVM_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); + enif_release_resource(rsrc_obj); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } term obj = term_from_resource(rsrc_obj, &ctx->heap); enif_release_resource(rsrc_obj); // decrement refcount after enif_alloc_resource @@ if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_REFERENCE_RESOURCE_SIZE + TUPLE_SIZE(2) + REF_SIZE) != MEMORY_GC_OK)) { + enif_release_resource(conn_rsrc_obj); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } term obj = term_from_resource(conn_rsrc_obj, &ctx->heap); enif_release_resource(conn_rsrc_obj); // decrement refcount after enif_allocate_resource in make_accepted_socket_resource @@ if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_REFERENCE_RESOURCE_SIZE) != MEMORY_GC_OK)) { AVM_LOGW(TAG, "Failed to allocate memory: %s:%i.", __FILE__, __LINE__); + enif_release_resource(conn_rsrc_obj); RAISE_ERROR(OUT_OF_MEMORY_ATOM); } term new_resource = term_from_resource(conn_rsrc_obj, &ctx->heap); enif_release_resource(conn_rsrc_obj); // decrement refcount after enif_alloc_resourceAlso applies to: 1817-1821, 1905-1910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libAtomVM/otp_socket.c` around lines 640 - 645, An OOM during boxing leaks the newly created socket resource because branches that call RAISE_ERROR(OUT_OF_MEMORY_ATOM) return early without calling enif_release_resource(rsrc_obj); update the OOM error paths in otp_socket.c around the memory_ensure_free(...) checks (the block that currently logs and RAISE_ERROR after UNLIKELY) to call enif_release_resource(rsrc_obj) before raising, and do the same fix for the analogous locations referenced (around lines corresponding to 1817-1821 and 1905-1910) so term_from_resource(rsrc_obj, &ctx->heap) never leaves the resource unreleased on error.src/libAtomVM/resources.c (1)
511-528:⚠️ Potential issue | 🔴 CriticalValidate and decrement the exact monitored resource here.
Line 519 only checks the resource type, but the monitor is located by
mon->ref_ticks. Ifobjis a different live resource of the same type, this path removes the real monitor and then subtractsREFC_MONITOR_INCfrom the wrong object at Line 527.🐛 Proposed fix
struct RefcBinary *resource = refc_binary_from_data(obj); @@ if (monitor->ref_ticks == mon->ref_ticks) { - if (resource->resource_type != mon->resource_type) { + if (monitor->resource != resource || resource->resource_type != mon->resource_type) { synclist_unlock(&resource_type->monitors); return -1; } @@ - (void) refc_binary_sub_refcount(resource, REFC_MONITOR_INC); + (void) refc_binary_sub_refcount(monitor->resource, REFC_MONITOR_INC); free(monitor); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libAtomVM/resources.c` around lines 511 - 528, The current loop in resources.c finds a monitor by matching monitor->ref_ticks to mon->ref_ticks but then operates on `resource = refc_binary_from_data(obj)`, which can be a different live resource of the same type; instead ensure you validate the monitor refers to the exact same resource instance before removing it and decrementing its refcount: in the LIST_FOR_EACH that inspects struct ResourceMonitor *monitor (and compares monitor->ref_ticks and resource_type), also compare the actual monitored resource identity (e.g., monitor->resource pointer or stored refc id) against the resource returned by refc_binary_from_data(obj) (or obtain the resource to decrement from monitor itself), and then call refc_binary_sub_refcount on that exact resource (not the possibly-different `resource` variable) after list_remove and free(monitor).libs/jit/src/jit_x86_64.erl (1)
325-336:⚠️ Potential issue | 🟠 MajorKeep
available_regsconstrained to?AVAILABLE_REGS_MASK.These free paths now OR whatever
reg_bit/1returns intoavailable_regs. Becausex86_64_register()still includesrdx/rsi/rdi, freeing one of those registers will poison the allocator mask with non-allocatable bits, andassert_all_native_free/1can never pass again. Mask the result back down, or reject frees outside the allocatable set.🛠️ Minimal fix
State#state{ - available_regs = Available0 bor Bit, + available_regs = (Available0 bor Bit) band ?AVAILABLE_REGS_MASK, used_regs = Used0 band (bnot Bit) }; ... State0#state{ - available_regs = AvR0 bor Bit, + available_regs = (AvR0 bor Bit) band ?AVAILABLE_REGS_MASK, used_regs = UR0 band (bnot Bit) };Also applies to: 1018-1023
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/jit/src/jit_x86_64.erl` around lines 325 - 336, The free paths in free_native_register currently OR reg_bit(Reg) into available_regs without constraining to ?AVAILABLE_REGS_MASK, which lets non-allocatable bits leak and break assert_all_native_free; update the code in free_native_register (the clause that computes Bit = reg_bit(Reg) and sets available_regs = Available0 bor Bit) to mask the result back with Available0 bor Bit band ?AVAILABLE_REGS_MASK (or validate Reg against the allocatable set and reject frees outside it), and apply the same masking/validation change to the other free path(s) referenced (around the 1018-1023 region) so all frees keep available_regs constrained to ?AVAILABLE_REGS_MASK.
♻️ Duplicate comments (3)
src/platforms/esp32/components/avm_builtins/network_driver.c (2)
470-509:⚠️ Potential issue | 🟠 MajorThe tuple head still looks like “records returned”, not “networks discovered”.
If the caller caps
results, Line 509 is built fromnum_resultsafteresp_wifi_scan_get_ap_records(). Per ESP-IDF, that out-parameter is the number copied intoap_records, so truncated scans under-report the first element even though the public contract still names itNetworksDiscovered. Please verify this against the ESP-IDF docs and, if confirmed, fetch the total viaesp_wifi_scan_get_ap_num()before draining the AP list.ESP-IDF esp_wifi_scan_get_ap_records number out parameter copied count vs total found, and when esp_wifi_scan_get_ap_num should be used to report total discovered APs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 470 - 509, The code uses esp_wifi_scan_get_ap_records to fill ap_records and then reports num_results as the "NetworksDiscovered" count, but ESP-IDF's out-parameter can be the number copied (truncated) so the reported total may be wrong; call esp_wifi_scan_get_ap_num() before freeing the AP list to get the true total, store that in a new variable (e.g., total_results) and use total_results for the tuple head while still passing the possibly-truncated num_results into wifi_ap_records_to_list_maybe_gc to convert the returned records; update references to num_results when building scan_results to use total_results so the public tuple reflects total discovered networks.
449-466:⚠️ Potential issue | 🔴 CriticalMove scan-handler teardown ahead of both error replies.
These two branches still send
{scan_results, {error, ...}}whilectx->platform_datapoints atdata. Once that message is delivered, Line 1470 can start a new scan, freectx->platform_data, and then this function continues into its own unregister/free path. That reopens the UAF/double-free race the success path at Lines 518-537 already avoids.Suggested reordering
- port_send_message_from_task(ctx->global, local_process_term, msg); - END_WITH_STACK_HEAP(heap, ctx->global); err = esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler); if (UNLIKELY(err != ESP_OK)) { ESP_LOGE(TAG, "Failed to unregister event handler for reason %s, future scans may fail", esp_err_to_name(err)); } else { ctx->platform_data = NULL; - free(data); } + port_send_message_from_task(ctx->global, local_process_term, msg); + END_WITH_STACK_HEAP(heap, ctx->global); + if (err == ESP_OK) { + free(data); + } return;Also applies to: 471-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platforms/esp32/components/avm_builtins/network_driver.c` around lines 449 - 466, The error paths currently send the `{scan_results, {error, ...}}` message while `ctx->platform_data` still points to `data`, which reintroduces a UAF/double-free race; move the scan-handler teardown (call to esp_event_handler_unregister with `scan_done_handler` and the subsequent `ctx->platform_data = NULL; free(data)`) to occur before creating/sending the error reply (the `port_heap_create_tuple2`/`port_send_message_from_task` sequence) in both branches where `ap_records` is NULL (the block using `esp_wifi_clear_ap_list`, `port_heap_create_tuple2`, `port_send_message_from_task`) and the similar branch at 471-493 so the platform data is cleared and freed before any message that might trigger a new scan is dispatched.libs/eavmlib/src/network.erl (1)
589-589:⚠️ Potential issue | 🟡 MinorUpdate the stale
sta_config/0reference.
wifi_scan/0is pulling defaults from the server’s current STA config, not from asta_config/0function. The current doc line points readers to an API that does not exist.Doc fix
-%% `@doc` Equivalent to `wifi_scan/1' with default options from `sta_config/0'. +%% `@doc` Equivalent to `wifi_scan/1' with default options from the current STA configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/eavmlib/src/network.erl` at line 589, The doc line incorrectly references a non-existent sta_config/0; update the comment for wifi_scan/0 to state that it pulls defaults from the server's current STA config (not from sta_config/0) and that it is equivalent to wifi_scan/1 with those server-side defaults; make the change in the documentation string near the wifi_scan/0 definition and remove or replace the sta_config/0 mention so readers are pointed to the server's current STA config behavior.
🧹 Nitpick comments (4)
src/libAtomVM/term.h (1)
3011-3022: Clarify theterm_from_resource_binary()contract in the header.The new
void *objsignature no longer makes it obvious thatobjmust be a RefcBinary-backed resource data pointer, and this block also omits the heap-space precondition that callers are now enforcing. Making both requirements explicit here will prevent incorrect future call sites.📝 Suggested doc update
/** * `@brief` Create a resource managed binary. * `@details` Increment reference count of resource and create a refc binary for - * the pointer and size. - * - * `@param` obj the managing resource object + * the pointer and size. + * + * Callers must ensure `TERM_BOXED_REFC_BINARY_SIZE` free terms are available + * in `heap` before calling this helper. + * + * `@param` obj the managing resource object returned as a RefcBinary-backed + * resource data pointer (for example from `enif_alloc_resource`) * `@param` data the pointer to the data * `@param` size the size of the binary * `@param` heap the heap to allocate memory in * `@param` glb the global context * `@return` a binary term managed by obj */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libAtomVM/term.h` around lines 3011 - 3022, Update the term_from_resource_binary() header comment to explicitly state that the void *obj parameter must be a pointer to the resource's RefcBinary-backed data (i.e., the resource that owns the binary and whose refcount will be incremented), that data must point into that resource's buffer, and that callers must ensure the provided Heap has sufficient free space to allocate the resulting binary of the given size (document the precondition: heap must have at least the binary header + size available); also mention that glb must be a valid GlobalContext. Reference term_from_resource_binary(void *obj, const void *data, size_t size, Heap *heap, GlobalContext *glb) in the comment so future callers know these constraints.tests/libs/jit/jit_armv6m_tests.erl (1)
3011-3015: Please avoid hard-coding backend tuple slots in the test setup.These
setelement(7, ...)/setelement(8, ...)calls bake in both the current state tuple layout and the register-bit mapping. A tiny helper that marks registers as used would keep this test focused onmove_to_array_element/5instead of backend internals.Also applies to: 3028-3032
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/libs/jit/jit_armv6m_tests.erl` around lines 3011 - 3015, The test is directly manipulating backend tuple slots via setelement(7, ...)/setelement(8, ...) which hard-codes tuple layout and register-bit mapping; replace these with a small test helper that marks registers as used so the test focuses on move_to_array_element/5. Add a helper (e.g., mark_regs_used(State0, [r4,r3]) or mark_used_regs(State0, RegList)) that encapsulates the setelement logic and/or calls a backend helper like ?BACKEND:mark_reg_used/2, then use that to produce State2 and call ?BACKEND:used_regs(State2) instead of inlining setelement(7/8,...). This keeps references to State0/State1/State2 and used_regs/move_to_array_element/5 but removes hard-coded tuple indices.tests/libs/jit/jit_aarch64_tests.erl (1)
1965-1968: Hide the raw state tuple surgery behind a helper.These
setelement(7, ...)/setelement(8, ...)updates hard-code both the backend state layout and the register-bit mapping. A small helper that marks registers as used by name would make the test easier to follow and much less brittle.Also applies to: 1981-1984
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/libs/jit/jit_aarch64_tests.erl` around lines 1965 - 1968, Replace the raw tuple surgery with a small helper that marks registers used by name: implement a helper like mark_reg_used(State, RegIndex) (or mark_regs_used(State, [RegIndexes])) that encapsulates the setelement/element bit-clear on slot 7 and bit-set on slot 8, then replace the inline State1/State2 updates (the setelement(7, ...), setelement(8, ...), element(7, State0) band (bnot ...), and (1 bsl N) bor ...) with calls to that helper in the test; this keeps register-bit mapping and backend state layout inside the helper and makes the test call simply mark_reg_used(State0, 8) / mark_reg_used(State0, 9) or a batch call for both registers.tests/libs/jit/jit_riscv32_tests.erl (1)
2545-2549: Avoid hard-coding backend tuple slots and mask literals in these fixtures.
setelement(7/8, ...)plus16#7700/8 bor 2048ties the test to the current record layout and register-bit encoding. The next state-field reorder or mask tweak will break the fixture setup instead of the behavior under test. A small helper that names theavailable_regs/used_regsmutation in one place would make these cases much less brittle.Also applies to: 2563-2567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/libs/jit/jit_riscv32_tests.erl` around lines 2545 - 2549, The test hard-codes backend tuple slots and register-mask literals (using setelement(7/8, State0, ...) and 16#7700 / 8 bor 2048) which makes fixtures brittle; instead add and use a small helper function (e.g., set_available_regs(State, Mask) and mark_used_regs(State, RegList) or a single mutate_regs(State, AvailableMask, UsedList)) and update these fixtures to call that helper rather than calling setelement or using raw hex/bor values; locate usages around State1/State2 and the ?BACKEND:used_regs check and replace the literal mask and setelement calls with the helper so tests mutate the available_regs/used_regs fields by name rather than by slot/magic numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/src/network-programming-guide.md`:
- Around line 202-205: Update the chip name typo in the scan-limit sentence:
replace "ESP32-C61" with the correct "ESP32-C6" in the network programming guide
text (the sentence listing per-target limits that currently reads
"ESP32-S2/ESP32-C61/ESP32-C5 up to 14"). Ensure the corrected string appears
exactly as "ESP32-C6" so the per-target limit list is accurate.
In `@libs/estdlib/src/lists.erl`:
- Around line 861-867: The clauses for sublist/3 that handle Start >= 2 must
also validate Len: add the same guards is_integer(L) and L >= 0 to both clauses
sublist([], S, _L) when is_integer(S), S >= 2 and sublist([_H | T], S, L) when
is_integer(S), S >= 2 so out-of-range paths fail when Len is invalid; update
sublist/3 accordingly and add a regression in test_sublist3/0 that asserts calls
like lists:sublist([], 2, -1) and lists:sublist([a], 3, foo) crash (i.e. do not
return []) to ensure the new guards are enforced.
In `@libs/jit/src/jit_aarch64.erl`:
- Around line 1294-1298: mask_to_list/1 currently omits r17 (REG_BIT_R17),
causing AvailableScratchMask built in set_args/2 from ?SCRATCH_REGS_MASK to
reduce to REG_BIT_R17 which mask_to_list/1 turns into [] and lets set_args0/5
fall through instead of using the IPC scratch; fix by updating mask_to_list/1 to
include r17 when REG_BIT_R17 is present (or special-case REG_BIT_R17 in
set_args/2) so AvailableScratchGP contains r17 as a fallback scratch register;
touch mask_to_list/1 (and repeat the same change in the other occurrence around
lines 2636-2665) so code paths in set_args/2 and set_args0/5 can consume r17 as
a valid scratch.
In `@libs/jit/src/jit_armv6m.erl`:
- Around line 1942-1946: AvailableScratchGP is being produced by mask_to_list/1
which filters to AVAILABLE_REGS and therefore drops real spill-only scratch
temps; replace this with a conversion that respects the full scratch mask.
Modify the code that sets AvailableScratchGP (currently: AvailableScratchMask =
?SCRATCH_REGS_MASK band (bnot (ParamMask bor ArgsMask bor UsedRegsMask)),
AvailableScratchGP = mask_to_list(AvailableScratchMask)) to call or implement a
helper that converts AvailableScratchMask into a list of scratch registers
without restricting to AVAILABLE_REGS (e.g. mask_to_scratch_list/1 or
mask_to_list/2 with a scratch-only flag), so set_registers_args0/6 sees the
actual spare scratch registers; keep the rest of the mask computation unchanged
and reuse ?SCRATCH_REGS_MASK and AvailableScratchMask names.
- Around line 1785-1828: The fallback branch can call
first_avail(AvailForResultMask) when AvailForResultMask is 0; instead detect
AvailForResultMask == 0 and that the original FuncPtrReg was saved in the push
frame (i.e., SavedRegs included the FuncPtr from FuncPtrTuple) and, in that
case, emit a store of r0 into the stack slot corresponding to that saved FuncPtr
(compute offset with jit_armv6m_asm:reg_to_num(FuncPtrReg)*4 and create a str),
append it to Stream5, update UsedRegs1Mask to include reg_bit(FuncPtrReg) and
return FuncPtrReg as the ResultReg instead of calling first_avail; leave the
existing move-to-register path for non-zero AvailForResultMask.
In `@libs/jit/src/jit_riscv32.erl`:
- Around line 1676-1714: The fallback arm can call
first_avail(AvailableRegs1Mask) when AvailableRegs1Mask is 0 (e.g., after
call_primitive/3 with available_regs = 0), causing use of an invalid register;
handle the `{primitive, _}` / FuncPtrReg-saved path before that fallback by
detecting when FuncPtrReg is dead-but-saved (inspect FuncPtrTuple or
AvailableRegs1Mask == 0 and whether FuncPtrReg is in SavedRegs) and, if so,
store a0 to the saved stack slot (use jit_riscv32_asm:sw(sp, a0,
StoreResultStackOffset)) instead of moving into a register; otherwise when
FuncPtrReg is free keep the mv path. Ensure this logic touches the same
variables used in the diff (FuncPtrTuple/ResultFPReg0/ResultFPReg1, SavedRegs,
UsedRegs1Mask, Stream5, a0) and prevents calling first_avail(0) before
pop_registers/4.
In `@libs/jit/src/jit_x86_64.erl`:
- Around line 528-538: call_primitive_last/3 unconditionally calls
first_avail(ScratchMask) which crashes when ScratchMask == 0; guard that case by
checking ScratchMask before computing Temp and TempBit and, when ScratchMask ==
0, take the same fallback path used in call_primitive/3 (i.e., skip allocating a
Temp register and compute AvailableRegs1/UsedRegs accordingly) so you do not
call first_avail(0); update the logic surrounding Temp, TempBit, AvailableRegs1
and UsedRegs to reflect the two branches and keep behavior identical to
call_primitive/3 in the zero-scratch case.
In `@src/libAtomVM/otp_ssl.c`:
- Around line 238-242: The OOM branch is releasing the resource
(enif_release_resource(rsrc_obj)) before the mbedTLS context is initialized,
which can run the destructor on an uninitialized mbedtls_* context and crash;
fix by either removing the second memory_ensure_free check or—preferably—moving
all mbedtls_*_init(...) calls to occur immediately after
term_from_resource(rsrc_obj, &ctx->heap) and before any enif_release_resource
calls so the destructor sees a fully-initialized struct; apply the same reorder
to the other init blocks mentioned (the other three init functions at the noted
sites) and ensure rsrc_obj is only released after initialization completes or on
cleanup paths where the mbedTLS context has been initialized.
In `@src/libAtomVM/refc_binary.h`:
- Around line 112-125: refc_binary_cas_refcount currently uses a weak CAS
(ATOMIC_COMPARE_EXCHANGE_WEAK_INT) on PLATFORM_ATOMIC builds which requires
retry loops but callers (refc_binary.c caller at the CF where
refc_binary_cas_refcount is invoked and the resources.c caller) don't retry;
also refc_binary_get_refcount does a plain non-atomic read causing races. Fix by
replacing the weak CAS with a strong compare-exchange on the PLATFORM_ATOMIC
path (or emulate strong semantics by looping on the weak CAS until
success/expected changes) inside refc_binary_cas_refcount, and change
refc_binary_get_refcount to perform an atomic load of ref_count (using the same
platform-atomic API or C11 atomic_load equivalent) so reads are race-free;
ensure the function names refc_binary_cas_refcount and refc_binary_get_refcount
are updated and callers no longer need to implement retries.
In `@src/libAtomVM/term.c`:
- Around line 1378-1380: enif_alloc_resource(...) can return NULL; check the
returned pointer before dereferencing resource_binary in this allocation block
(the allocation of struct ResourceBinary using enif_alloc_resource with
glb->resource_binary_resource_type), and when NULL handle the failure cleanly
(e.g., free any prior resources if needed and return an error/NULL from the
enclosing function) instead of proceeding to call refc_binary_from_data(obj) or
assign resource_binary->data; add a NULL-check for resource_binary and an
appropriate early return/cleanup path so ResourceBinary is only dereferenced
when allocation succeeds.
In `@src/platforms/rp2/src/lib/platform_atomic.h`:
- Around line 153-180: The three helpers smp_atomic_fetch_add_size,
smp_atomic_fetch_sub_size, and smp_atomic_fetch_or_size are currently omitted
under AVM_NO_SMP; add the same single-core fallback used by the compare/exchange
helpers by providing implementations guarded for AVM_NO_SMP that use
save_and_disable_interrupts() to disable interrupts, perform the
read-modify-write (compute old, update *object), then restore_interrupts()
before returning old, so the functions are available and atomic on single-core
builds.
---
Outside diff comments:
In `@libs/jit/src/jit_x86_64.erl`:
- Around line 325-336: The free paths in free_native_register currently OR
reg_bit(Reg) into available_regs without constraining to ?AVAILABLE_REGS_MASK,
which lets non-allocatable bits leak and break assert_all_native_free; update
the code in free_native_register (the clause that computes Bit = reg_bit(Reg)
and sets available_regs = Available0 bor Bit) to mask the result back with
Available0 bor Bit band ?AVAILABLE_REGS_MASK (or validate Reg against the
allocatable set and reject frees outside it), and apply the same
masking/validation change to the other free path(s) referenced (around the
1018-1023 region) so all frees keep available_regs constrained to
?AVAILABLE_REGS_MASK.
In `@src/libAtomVM/otp_socket.c`:
- Around line 640-645: An OOM during boxing leaks the newly created socket
resource because branches that call RAISE_ERROR(OUT_OF_MEMORY_ATOM) return early
without calling enif_release_resource(rsrc_obj); update the OOM error paths in
otp_socket.c around the memory_ensure_free(...) checks (the block that currently
logs and RAISE_ERROR after UNLIKELY) to call enif_release_resource(rsrc_obj)
before raising, and do the same fix for the analogous locations referenced
(around lines corresponding to 1817-1821 and 1905-1910) so
term_from_resource(rsrc_obj, &ctx->heap) never leaves the resource unreleased on
error.
In `@src/libAtomVM/posix_nifs.c`:
- Around line 849-855: The current code calls memory_ensure_free_opt() and
immediately RAISE_ERROR(OUT_OF_MEMORY_ATOM) on failure while dir_obj still owns
the DIR*, causing a resource leak; modify the failure path in the block that
calls memory_ensure_free_opt() (the code invoking term_from_resource and using
dir_obj) to call enif_release_resource(dir_obj) before raising the
OUT_OF_MEMORY_ATOM error so the resource reference is decremented and the DIR*
is closed.
In `@src/libAtomVM/resources.c`:
- Around line 511-528: The current loop in resources.c finds a monitor by
matching monitor->ref_ticks to mon->ref_ticks but then operates on `resource =
refc_binary_from_data(obj)`, which can be a different live resource of the same
type; instead ensure you validate the monitor refers to the exact same resource
instance before removing it and decrementing its refcount: in the LIST_FOR_EACH
that inspects struct ResourceMonitor *monitor (and compares monitor->ref_ticks
and resource_type), also compare the actual monitored resource identity (e.g.,
monitor->resource pointer or stored refc id) against the resource returned by
refc_binary_from_data(obj) (or obtain the resource to decrement from monitor
itself), and then call refc_binary_sub_refcount on that exact resource (not the
possibly-different `resource` variable) after list_remove and free(monitor).
In `@src/platforms/esp32/components/avm_builtins/adc_driver.c`:
- Around line 497-503: The call to memory_ensure_free currently passes the
boolean result of the comparison (TERM_BOXED_RESOURCE_SIZE != MEMORY_GC_OK) as
the size, reserving 0/1 bytes and causing heap corruption when
term_from_resource allocates; update the condition to call
memory_ensure_free(ctx, TERM_BOXED_RESOURCE_SIZE) and compare its return value
to MEMORY_GC_OK (i.e., if (UNLIKELY(memory_ensure_free(ctx,
TERM_BOXED_RESOURCE_SIZE) != MEMORY_GC_OK)) { ... }) so the correct number of
bytes is reserved before using term_from_resource with chan_rsrc and ctx->heap.
In `@src/platforms/esp32/components/avm_builtins/dac_driver.c`:
- Around line 113-138: Reserve and root the boxed resource term before building
the return tuple: replace the plain memory_ensure_free(ctx,
TERM_BOXED_RESOURCE_SIZE) + term_from_resource(chan_rsrc, &ctx->heap) sequence
with a call to memory_ensure_free_with_roots (or equivalent) that both ensures
space and registers chan_obj as a root, then call term_from_resource(chan_rsrc,
&ctx->heap) into that rooted slot; after that proceed to call
dac_oneshot_new_channel and build the tuple with
term_alloc_tuple/term_put_tuple_element as before, keeping
enif_release_resource(chan_rsrc) and the existing error path intact so the
resource is rooted during any GC while constructing the {'$dac', ..., Ref}
tuple.
In `@src/platforms/esp32/components/avm_builtins/i2c_resource.c`:
- Around line 219-237: The second OOM path is deleting the I2C driver after
ownership of the resource has already been transferred to obj (via
term_from_resource + enif_release_resource), risking a double-delete when the
resource dtor later runs; to fix, remove the i2c_driver_delete(i2c_num) call
from the failure branch that follows memory_ensure_free_with_roots (or guard it
behind an explicit ownership check) so that once obj has been formed we no
longer delete the driver there; keep the earlier i2c_driver_delete in the
pre-object OOM branch (the one after memory_ensure_free) intact.
---
Duplicate comments:
In `@libs/eavmlib/src/network.erl`:
- Line 589: The doc line incorrectly references a non-existent sta_config/0;
update the comment for wifi_scan/0 to state that it pulls defaults from the
server's current STA config (not from sta_config/0) and that it is equivalent to
wifi_scan/1 with those server-side defaults; make the change in the
documentation string near the wifi_scan/0 definition and remove or replace the
sta_config/0 mention so readers are pointed to the server's current STA config
behavior.
In `@src/platforms/esp32/components/avm_builtins/network_driver.c`:
- Around line 470-509: The code uses esp_wifi_scan_get_ap_records to fill
ap_records and then reports num_results as the "NetworksDiscovered" count, but
ESP-IDF's out-parameter can be the number copied (truncated) so the reported
total may be wrong; call esp_wifi_scan_get_ap_num() before freeing the AP list
to get the true total, store that in a new variable (e.g., total_results) and
use total_results for the tuple head while still passing the possibly-truncated
num_results into wifi_ap_records_to_list_maybe_gc to convert the returned
records; update references to num_results when building scan_results to use
total_results so the public tuple reflects total discovered networks.
- Around line 449-466: The error paths currently send the `{scan_results,
{error, ...}}` message while `ctx->platform_data` still points to `data`, which
reintroduces a UAF/double-free race; move the scan-handler teardown (call to
esp_event_handler_unregister with `scan_done_handler` and the subsequent
`ctx->platform_data = NULL; free(data)`) to occur before creating/sending the
error reply (the `port_heap_create_tuple2`/`port_send_message_from_task`
sequence) in both branches where `ap_records` is NULL (the block using
`esp_wifi_clear_ap_list`, `port_heap_create_tuple2`,
`port_send_message_from_task`) and the similar branch at 471-493 so the platform
data is cleared and freed before any message that might trigger a new scan is
dispatched.
---
Nitpick comments:
In `@src/libAtomVM/term.h`:
- Around line 3011-3022: Update the term_from_resource_binary() header comment
to explicitly state that the void *obj parameter must be a pointer to the
resource's RefcBinary-backed data (i.e., the resource that owns the binary and
whose refcount will be incremented), that data must point into that resource's
buffer, and that callers must ensure the provided Heap has sufficient free space
to allocate the resulting binary of the given size (document the precondition:
heap must have at least the binary header + size available); also mention that
glb must be a valid GlobalContext. Reference term_from_resource_binary(void
*obj, const void *data, size_t size, Heap *heap, GlobalContext *glb) in the
comment so future callers know these constraints.
In `@tests/libs/jit/jit_aarch64_tests.erl`:
- Around line 1965-1968: Replace the raw tuple surgery with a small helper that
marks registers used by name: implement a helper like mark_reg_used(State,
RegIndex) (or mark_regs_used(State, [RegIndexes])) that encapsulates the
setelement/element bit-clear on slot 7 and bit-set on slot 8, then replace the
inline State1/State2 updates (the setelement(7, ...), setelement(8, ...),
element(7, State0) band (bnot ...), and (1 bsl N) bor ...) with calls to that
helper in the test; this keeps register-bit mapping and backend state layout
inside the helper and makes the test call simply mark_reg_used(State0, 8) /
mark_reg_used(State0, 9) or a batch call for both registers.
In `@tests/libs/jit/jit_armv6m_tests.erl`:
- Around line 3011-3015: The test is directly manipulating backend tuple slots
via setelement(7, ...)/setelement(8, ...) which hard-codes tuple layout and
register-bit mapping; replace these with a small test helper that marks
registers as used so the test focuses on move_to_array_element/5. Add a helper
(e.g., mark_regs_used(State0, [r4,r3]) or mark_used_regs(State0, RegList)) that
encapsulates the setelement logic and/or calls a backend helper like
?BACKEND:mark_reg_used/2, then use that to produce State2 and call
?BACKEND:used_regs(State2) instead of inlining setelement(7/8,...). This keeps
references to State0/State1/State2 and used_regs/move_to_array_element/5 but
removes hard-coded tuple indices.
In `@tests/libs/jit/jit_riscv32_tests.erl`:
- Around line 2545-2549: The test hard-codes backend tuple slots and
register-mask literals (using setelement(7/8, State0, ...) and 16#7700 / 8 bor
2048) which makes fixtures brittle; instead add and use a small helper function
(e.g., set_available_regs(State, Mask) and mark_used_regs(State, RegList) or a
single mutate_regs(State, AvailableMask, UsedList)) and update these fixtures to
call that helper rather than calling setelement or using raw hex/bor values;
locate usages around State1/State2 and the ?BACKEND:used_regs check and replace
the literal mask and setelement calls with the helper so tests mutate the
available_regs/used_regs fields by name rather than by slot/magic numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dc1af10-9e72-484a-848d-6b686f54986b
📒 Files selected for processing (54)
.github/workflows/esp32-build.yamlCHANGELOG.mddoc/src/build-instructions.mddoc/src/network-programming-guide.mdexamples/erlang/esp32/CMakeLists.txtexamples/erlang/esp32/wifi_scan.erlexamples/erlang/esp32/wifi_scan_callback.erllibs/eavmlib/src/network.erllibs/estdlib/src/CMakeLists.txtlibs/estdlib/src/erlang.erllibs/estdlib/src/filename.erllibs/estdlib/src/lists.erllibs/jit/src/jit_aarch64.erllibs/jit/src/jit_armv6m.erllibs/jit/src/jit_riscv32.erllibs/jit/src/jit_x86_64.erlsrc/libAtomVM/bifs.gperfsrc/libAtomVM/erl_nif.hsrc/libAtomVM/globalcontext.csrc/libAtomVM/jit_stream_flash.csrc/libAtomVM/nifs.csrc/libAtomVM/nifs.gperfsrc/libAtomVM/otp_socket.csrc/libAtomVM/otp_ssl.csrc/libAtomVM/posix_nifs.csrc/libAtomVM/refc_binary.csrc/libAtomVM/refc_binary.hsrc/libAtomVM/resources.csrc/libAtomVM/term.csrc/libAtomVM/term.hsrc/platforms/emscripten/src/lib/platform_nifs.csrc/platforms/esp32/components/avm_builtins/adc_driver.csrc/platforms/esp32/components/avm_builtins/dac_driver.csrc/platforms/esp32/components/avm_builtins/i2c_resource.csrc/platforms/esp32/components/avm_builtins/network_driver.csrc/platforms/esp32/components/avm_builtins/storage_nif.csrc/platforms/esp32/components/avm_sys/platform_nifs.csrc/platforms/esp32/test/main/test_erl_sources/CMakeLists.txtsrc/platforms/esp32/test/main/test_erl_sources/test_wifi_scan.erlsrc/platforms/esp32/test/main/test_main.csrc/platforms/generic_unix/lib/jit_stream_mmap.csrc/platforms/rp2/src/lib/platform_atomic.htests/erlang_tests/CMakeLists.txttests/erlang_tests/test_erlang_builtins.erltests/erlang_tests/test_min_heap_size.erltests/libs/estdlib/CMakeLists.txttests/libs/estdlib/test_filename.erltests/libs/estdlib/test_lists.erltests/libs/estdlib/tests.erltests/libs/jit/jit_aarch64_tests.erltests/libs/jit/jit_armv6m_tests.erltests/libs/jit/jit_riscv32_tests.erltests/test-enif.ctests/test.c
💤 Files with no reviewable changes (1)
- .github/workflows/esp32-build.yaml
✅ Files skipped from review due to trivial changes (2)
- src/libAtomVM/nifs.gperf
- src/libAtomVM/bifs.gperf
🚧 Files skipped from review as they are similar to previous changes (5)
- src/platforms/esp32/test/main/test_erl_sources/CMakeLists.txt
- examples/erlang/esp32/CMakeLists.txt
- src/platforms/esp32/test/main/test_main.c
- CHANGELOG.md
- examples/erlang/esp32/wifi_scan.erl
Comment on lines
+202
to
+205
| To minimize the risk of out-of-memory errors, this driver limits the maximum number of returned | ||
| networks depending on the target and memory configuration: | ||
| ESP32-C2 supports up to 10, ESP32-S2/ESP32-C61/ESP32-C5 up to 14, most other targets up to 20, | ||
| and ESP32-P4 or PSRAM-enabled builds up to 64. |
There was a problem hiding this comment.
Fix the chip name in the scan-limit table.
ESP32-C61 looks like a typo for ESP32-C6. As written, the per-target limit list is inaccurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/src/network-programming-guide.md` around lines 202 - 205, Update the chip
name typo in the scan-limit sentence: replace "ESP32-C61" with the correct
"ESP32-C6" in the network programming guide text (the sentence listing
per-target limits that currently reads "ESP32-S2/ESP32-C61/ESP32-C5 up to 14").
Ensure the corrected string appears exactly as "ESP32-C6" so the per-target
limit list is accurate.
Owner
Author
There was a problem hiding this comment.
No, the ESP32-C61 is a different chipset from the ESP32-C6, the C61 has less available RAM than the C6, hence the lower return limit, the C6 falls into the catchall group that return at most 20 networks.
There was a problem hiding this comment.
@UncleGrumpy, thank you for the clarification! You're absolutely right — the ESP32-C61 is a distinct chipset with less available RAM than the ESP32-C6, which is why it has the lower scan return limit (up to 14 networks) while the ESP32-C6 falls into the general group returning up to 20. My flagging of ESP32-C61 as a typo was incorrect, and no change is needed. Apologies for the noise!
✏️ Learnings added
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: doc/src/network-programming-guide.md:202-205
Timestamp: 2026-03-09T21:30:21.758Z
Learning: In `doc/src/network-programming-guide.md` (UncleGrumpy/AtomVM PR `#11`), `ESP32-C61` is a real, distinct Espressif chipset — it is NOT a typo for `ESP32-C6`. The C61 has less available RAM than the C6, so it has a lower wifi_scan result limit (up to 14 networks). The ESP32-C6 falls into the general catchall group that returns up to 20 networks. Do not flag `ESP32-C61` as a typo in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: libs/eavmlib/src/network.erl:559-567
Timestamp: 2026-03-08T23:27:29.576Z
Learning: In `libs/eavmlib/src/network.erl`, `wifi_scan/1` (and `wifi_scan/0`) intentionally calls `error(unsupported_platform)` — raising an Erlang exception — on non-ESP32 platforms rather than returning `{error, unsupported_platform}`. This is explicitly documented in the function's edoc ("will raise an `unsupported_platform` error") and is a deliberate design decision. Do not flag this as a spec mismatch or suggest changing it to a return-value error; the throwing behaviour is by design.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: src/platforms/esp32/components/avm_builtins/network_driver.c:1446-1468
Timestamp: 2026-03-09T00:49:00.445Z
Learning: In `src/platforms/esp32/components/avm_builtins/network_driver.c`, the `wifi_scan` function intentionally calls `esp_event_handler_unregister(WIFI_EVENT, WIFI_EVENT_SCAN_DONE, &scan_done_handler)` before registering a new handler for each scan request. No error check on unregister is needed because a failure only indicates no handler was previously registered. The Erlang gen_server is responsible for sending `{error, canceled}` to the previous caller on the Erlang side. This design is intentional and should not be changed to use `esp_event_handler_instance_register`.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: libs/eavmlib/src/network.erl:597-605
Timestamp: 2026-03-04T07:09:26.484Z
Learning: In `libs/eavmlib/src/network.erl`, the scan cancellation behavior in `handle_call({scan, ...})` is intentional: ESP-IDF does not support concurrent WiFi scans; initiating a second scan cancels the first at the platform level. The gen_server mirrors this by immediately replying `{error, canceled}` to the previous caller and overwriting `scan_receiver` with the new caller. This is by design and should not be changed to serialize/queue scans.
Learnt from: UncleGrumpy
Repo: UncleGrumpy/AtomVM PR: 11
File: examples/erlang/esp32/wifi_scan_callback.erl:28-34
Timestamp: 2026-03-04T11:01:37.985Z
Learning: In the `UncleGrumpy/AtomVM` repository, example files under `examples/erlang/esp32/` are intentionally minimal and focused on demonstrating a single feature or function. They are not expected to be robust applications, so missing error handling (e.g., unchecked return values of `network:start/1`) in these files is by design and should not be flagged in reviews.
Comment on lines
+861
to
+867
| -spec sublist(List :: [Elem], Start :: pos_integer(), Len :: non_neg_integer()) -> [Elem]. | ||
| sublist(List, 1, L) when is_list(List), is_integer(L), L >= 0 -> | ||
| sublist(List, L); | ||
| sublist([], S, _L) when is_integer(S), S >= 2 -> | ||
| []; | ||
| sublist([_H | T], S, L) when is_integer(S), S >= 2 -> | ||
| sublist(T, S - 1, L). |
There was a problem hiding this comment.
Validate Len on the out-of-range path too.
When Start runs past the end, these clauses ignore Len, so calls like lists:sublist([], 2, -1) and lists:sublist([a], 3, foo) currently return [] instead of failing like the Start = 1 path. Please guard L here as well, and add a regression for that combination in test_sublist3/0.
Suggested fix
sublist(List, 1, L) when is_list(List), is_integer(L), L >= 0 ->
sublist(List, L);
-sublist([], S, _L) when is_integer(S), S >= 2 ->
+sublist([], S, L) when is_integer(S), S >= 2, is_integer(L), L >= 0 ->
[];
-sublist([_H | T], S, L) when is_integer(S), S >= 2 ->
+sublist([_H | T], S, L) when is_integer(S), S >= 2, is_integer(L), L >= 0 ->
sublist(T, S - 1, L).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/estdlib/src/lists.erl` around lines 861 - 867, The clauses for sublist/3
that handle Start >= 2 must also validate Len: add the same guards is_integer(L)
and L >= 0 to both clauses sublist([], S, _L) when is_integer(S), S >= 2 and
sublist([_H | T], S, L) when is_integer(S), S >= 2 so out-of-range paths fail
when Len is invalid; update sublist/3 accordingly and add a regression in
test_sublist3/0 that asserts calls like lists:sublist([], 2, -1) and
lists:sublist([a], 3, foo) crash (i.e. do not return []) to ensure the new
guards are enforced.
Comment on lines
+1294
to
+1298
| ParamMask = regs_to_mask(ParamRegs), | ||
| ArgsMask = regs_to_mask(ArgsRegs), | ||
| AvailableScratchMask = | ||
| ?SCRATCH_REGS_MASK band (bnot (ParamMask bor ArgsMask bor UsedRegs)), | ||
| AvailableScratchGP = mask_to_list(AvailableScratchMask), |
There was a problem hiding this comment.
mask_to_list/1 drops r17, so set_args/2 can run out of scratch registers and crash.
set_args/2 builds AvailableScratchMask from ?SCRATCH_REGS_MASK, which explicitly includes r17, but mask_to_list/1 never emits r17. When all normal scratch regs are live, AvailableScratchMask can become only ?REG_BIT_R17; today that turns into [], and any parameter shuffle that needs a temp falls off the [Avail | AvailGPT] clause in set_args0/5 instead of using the IPC scratch register.
🔧 Minimal fix
-%% Order: [r7, r8, r9, r10, r11, r12, r13, r14, r15, r3, r4, r5, r6]
+%% Order: [r7, r8, r9, r10, r11, r12, r13, r14, r15, r3, r4, r5, r6, r17]
first_avail(Mask) when Mask band ?REG_BIT_R7 =/= 0 -> r7;
first_avail(Mask) when Mask band ?REG_BIT_R8 =/= 0 -> r8;
first_avail(Mask) when Mask band ?REG_BIT_R9 =/= 0 -> r9;
@@
%% Convert bitmask to list, matching the order
mask_to_list(0) -> [];
mask_to_list(Mask) -> mask_to_list_r7(Mask).
@@
-mask_to_list_r6(Mask) when Mask band ?REG_BIT_R6 =/= 0 -> [r6];
-mask_to_list_r6(_Mask) -> [].
+mask_to_list_r6(Mask) when Mask band ?REG_BIT_R6 =/= 0 -> [r6 | mask_to_list_r17(Mask)];
+mask_to_list_r6(Mask) -> mask_to_list_r17(Mask).
+mask_to_list_r17(Mask) when Mask band ?REG_BIT_R17 =/= 0 -> [r17];
+mask_to_list_r17(_Mask) -> [].Also applies to: 2636-2665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/jit/src/jit_aarch64.erl` around lines 1294 - 1298, mask_to_list/1
currently omits r17 (REG_BIT_R17), causing AvailableScratchMask built in
set_args/2 from ?SCRATCH_REGS_MASK to reduce to REG_BIT_R17 which mask_to_list/1
turns into [] and lets set_args0/5 fall through instead of using the IPC
scratch; fix by updating mask_to_list/1 to include r17 when REG_BIT_R17 is
present (or special-case REG_BIT_R17 in set_args/2) so AvailableScratchGP
contains r17 as a fallback scratch register; touch mask_to_list/1 (and repeat
the same change in the other occurrence around lines 2636-2665) so code paths in
set_args/2 and set_args0/5 can consume r17 as a valid scratch.
Comment on lines
+1785
to
1828
| {Stream6, UsedRegs2Mask, ResultReg} = | ||
| case {length(SavedRegs), FuncPtrTuple} of | ||
| {8, {free, ResultFPReg0}} -> | ||
| % Registers exhausted: use FuncPtrReg which is free after the call | ||
| StoreResultStackOffset0 = jit_armv6m_asm:reg_to_num(ResultFPReg0) * 4, | ||
| StoreResult0 = jit_armv6m_asm:str(r0, {sp, StoreResultStackOffset0}), | ||
| { | ||
| StreamModule:append(Stream5, StoreResult0), | ||
| UsedRegs1Mask bor reg_bit(ResultFPReg0), | ||
| ResultFPReg0 | ||
| }; | ||
| {8, _} when PaddingReg =/= undefined -> | ||
| % Use PaddingReg which was saved just for alignment | ||
| StoreResultStackOffset1 = jit_armv6m_asm:reg_to_num(PaddingReg) * 4, | ||
| StoreResult1 = jit_armv6m_asm:str(r0, {sp, StoreResultStackOffset1}), | ||
| { | ||
| StreamModule:append(Stream5, StoreResult1), | ||
| UsedRegs1Mask bor reg_bit(PaddingReg), | ||
| PaddingReg | ||
| }; | ||
| {_, {free, ResultFPReg1}} when ResultFPReg1 =/= PaddingReg -> | ||
| % FuncPtrReg is free after the call and not saved, use it for result | ||
| MoveResult0 = jit_armv6m_asm:mov(ResultFPReg1, r0), | ||
| { | ||
| StreamModule:append(Stream5, MoveResult0), | ||
| UsedRegs1Mask bor reg_bit(ResultFPReg1), | ||
| ResultFPReg1 | ||
| }; | ||
| _ -> | ||
| % Use any free that is not in SavedRegs | ||
| [ResultReg | _] = AvailableRegs1 -- SavedRegs, | ||
| MoveResult = jit_armv6m_asm:mov(ResultReg, r0), | ||
| {StreamModule:append(Stream5, MoveResult), [ResultReg | UsedRegs1]} | ||
| PaddingRegBit = | ||
| case PaddingReg of | ||
| undefined -> 0; | ||
| _ -> reg_bit(PaddingReg) | ||
| end, | ||
| AvailForResultMask = | ||
| AvailableRegs1Mask band (bnot (UsedRegs1Mask bor PaddingRegBit)), | ||
| ResultReg0 = first_avail(AvailForResultMask), | ||
| MoveResult0 = jit_armv6m_asm:mov(ResultReg0, r0), | ||
| { | ||
| StreamModule:append(Stream5, MoveResult0), | ||
| UsedRegs1Mask bor reg_bit(ResultReg0), | ||
| ResultReg0 | ||
| } | ||
| end, |
There was a problem hiding this comment.
The {primitive, _} no-free-register path still has no result register.
From Line 637, this path is taken exactly when all allocatable low regs were already live. If none of the args frees one, AvailForResultMask becomes 0 here and the fallback branch hits first_avail(0) after blx. In the failing path FuncPtrReg already has a slot in the 8-register push frame, so r0 needs to be written there instead of asking for a new register.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/jit/src/jit_armv6m.erl` around lines 1785 - 1828, The fallback branch
can call first_avail(AvailForResultMask) when AvailForResultMask is 0; instead
detect AvailForResultMask == 0 and that the original FuncPtrReg was saved in the
push frame (i.e., SavedRegs included the FuncPtr from FuncPtrTuple) and, in that
case, emit a store of r0 into the stack slot corresponding to that saved FuncPtr
(compute offset with jit_armv6m_asm:reg_to_num(FuncPtrReg)*4 and create a str),
append it to Stream5, update UsedRegs1Mask to include reg_bit(FuncPtrReg) and
return FuncPtrReg as the ResultReg instead of calling first_avail; leave the
existing move-to-register path for non-zero AvailForResultMask.
Comment on lines
+1942
to
+1946
| ParamMask = regs_to_mask(ParamRegs), | ||
| ArgsMask = regs_to_mask(ArgsRegs), | ||
| AvailableScratchMask = | ||
| ?SCRATCH_REGS_MASK band (bnot (ParamMask bor ArgsMask bor UsedRegsMask)), | ||
| AvailableScratchGP = mask_to_list(AvailableScratchMask), |
There was a problem hiding this comment.
AvailableScratchGP is dropping real spill temps.
AvailableScratchMask is derived from ?SCRATCH_REGS_MASK, but mask_to_list/1 only emits the AVAILABLE_REGS subset. If a register shuffle only has r12, r2, or r0 left, set_registers_args0/6 will hit [Avail | AvailGPT] with [] even though scratch regs still exist. This needs a dedicated scratch-mask conversion here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/jit/src/jit_armv6m.erl` around lines 1942 - 1946, AvailableScratchGP is
being produced by mask_to_list/1 which filters to AVAILABLE_REGS and therefore
drops real spill-only scratch temps; replace this with a conversion that
respects the full scratch mask. Modify the code that sets AvailableScratchGP
(currently: AvailableScratchMask = ?SCRATCH_REGS_MASK band (bnot (ParamMask bor
ArgsMask bor UsedRegsMask)), AvailableScratchGP =
mask_to_list(AvailableScratchMask)) to call or implement a helper that converts
AvailableScratchMask into a list of scratch registers without restricting to
AVAILABLE_REGS (e.g. mask_to_scratch_list/1 or mask_to_list/2 with a
scratch-only flag), so set_registers_args0/6 sees the actual spare scratch
registers; keep the rest of the mask computation unchanged and reuse
?SCRATCH_REGS_MASK and AvailableScratchMask names.
Comment on lines
528
to
538
| ParamRegs = lists:sublist(?PARAMETER_REGS, length(Args)), | ||
| ArgsRegs = args_regs(Args), | ||
| ScratchRegs = ?AVAILABLE_REGS -- ArgsRegs -- ParamRegs, | ||
| [Temp | AvailableRegs1] = ScratchRegs, | ||
| UsedRegs = ?AVAILABLE_REGS -- AvailableRegs1, | ||
| ParamMask = regs_to_mask(ParamRegs), | ||
| ArgsMask = regs_to_mask(ArgsRegs), | ||
| ScratchMask = | ||
| ?AVAILABLE_REGS_MASK band (bnot (ArgsMask bor ParamMask)), | ||
| Temp = first_avail(ScratchMask), | ||
| TempBit = reg_bit(Temp), | ||
| AvailableRegs1 = ScratchMask band (bnot TempBit), | ||
| UsedRegs = ?AVAILABLE_REGS_MASK band (bnot AvailableRegs1), | ||
| PrepCall = |
There was a problem hiding this comment.
Guard the empty scratch-set case in call_primitive_last/3.
call_primitive/3 already has a fallback when every non-parameter scratch register is consumed, but this path still does Temp = first_avail(ScratchMask) unconditionally. A 6-argument tail call sourced from rax, r10, r11, rcx, r8, and r9 makes ScratchMask = 0, so Line 534 turns into a compiler crash instead of emitting code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/jit/src/jit_x86_64.erl` around lines 528 - 538, call_primitive_last/3
unconditionally calls first_avail(ScratchMask) which crashes when ScratchMask ==
0; guard that case by checking ScratchMask before computing Temp and TempBit
and, when ScratchMask == 0, take the same fallback path used in call_primitive/3
(i.e., skip allocating a Temp register and compute AvailableRegs1/UsedRegs
accordingly) so you do not call first_avail(0); update the logic surrounding
Temp, TempBit, AvailableRegs1 and UsedRegs to reflect the two branches and keep
behavior identical to call_primitive/3 in the zero-scratch case.
Comment on lines
+238
to
+242
| if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_REFERENCE_RESOURCE_SIZE) != MEMORY_GC_OK)) { | ||
| enif_release_resource(rsrc_obj); | ||
| RAISE_ERROR(OUT_OF_MEMORY_ATOM); | ||
| } | ||
| term obj = term_from_resource(rsrc_obj, &ctx->heap); |
There was a problem hiding this comment.
Don't release these resources before their mbedTLS structs are initialized.
The new OOM branch calls enif_release_resource(rsrc_obj) before mbedtls_*_init(...). That immediately runs the destructor, which frees an uninitialized mbedTLS context and can crash on low-memory paths. Please either drop the second memory_ensure_free entirely or move initialization ahead of any possible release.
🛠️ Safer pattern
struct EntropyContextResource *rsrc_obj = enif_alloc_resource(entropycontext_resource_type, sizeof(struct EntropyContextResource));
if (IS_NULL_PTR(rsrc_obj)) {
AVM_LOGW(TAG, "Failed to allocate memory: %s:%i.\n", __FILE__, __LINE__);
RAISE_ERROR(OUT_OF_MEMORY_ATOM);
}
- if (UNLIKELY(memory_ensure_free(ctx, TERM_BOXED_REFERENCE_RESOURCE_SIZE) != MEMORY_GC_OK)) {
- enif_release_resource(rsrc_obj);
- RAISE_ERROR(OUT_OF_MEMORY_ATOM);
- }
- term obj = term_from_resource(rsrc_obj, &ctx->heap);
- enif_release_resource(rsrc_obj); // decrement refcount after enif_alloc_resource
-
mbedtls_entropy_init(&rsrc_obj->context);
+ term obj = term_from_resource(rsrc_obj, &ctx->heap);
+ enif_release_resource(rsrc_obj); // decrement refcount after enif_alloc_resourceApply the same ordering to the other three init functions.
Also applies to: 265-269, 321-325, 378-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libAtomVM/otp_ssl.c` around lines 238 - 242, The OOM branch is releasing
the resource (enif_release_resource(rsrc_obj)) before the mbedTLS context is
initialized, which can run the destructor on an uninitialized mbedtls_* context
and crash; fix by either removing the second memory_ensure_free check
or—preferably—moving all mbedtls_*_init(...) calls to occur immediately after
term_from_resource(rsrc_obj, &ctx->heap) and before any enif_release_resource
calls so the destructor sees a fully-initialized struct; apply the same reorder
to the other init blocks mentioned (the other three init functions at the noted
sites) and ensure rsrc_obj is only released after initialization completes or on
cleanup paths where the mbedTLS context has been initialized.
Comment on lines
+112
to
+125
| static inline bool refc_binary_cas_refcount(struct RefcBinary *refc, size_t *expected, size_t desired) | ||
| { | ||
| #if defined(AVM_NO_SMP) | ||
| if (refc->ref_count == *expected) { | ||
| refc->ref_count = desired; | ||
| return true; | ||
| } | ||
| *expected = refc->ref_count; | ||
| return false; | ||
| #elif defined(HAVE_ATOMIC) | ||
| return atomic_compare_exchange_strong(&refc->ref_count, expected, desired); | ||
| #else // HAVE_PLATFORM_ATOMIC_H guaranteed by #error above | ||
| return ATOMIC_COMPARE_EXCHANGE_WEAK_INT(&refc->ref_count, expected, desired); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd platform_atomic.h
rg -n -C3 'ATOMIC_COMPARE_EXCHANGE_WEAK_INT|refc_binary_cas_refcount\s*\(|refc_binary_get_refcount\s*\('
rg -n -C2 'ref_count\b|smp_atomic_.*size'Repository: UncleGrumpy/AtomVM
Length of output: 22399
Weak CAS on platform-atomic builds requires caller retry loops; non-atomic reads introduce races.
On HAVE_PLATFORM_ATOMIC_H builds, refc_binary_cas_refcount() uses ATOMIC_COMPARE_EXCHANGE_WEAK_INT() (weak semantics), but callers in refc_binary.c:89 and resources.c:473 do not retry on failure. Additionally, refc_binary_get_refcount() performs a plain (non-atomic) read of ref_count when it should use an atomic load to prevent races with concurrent writes. The C11 path (HAVE_ATOMIC) uses strong CAS semantics, creating an unhandled inconsistency in failure behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libAtomVM/refc_binary.h` around lines 112 - 125, refc_binary_cas_refcount
currently uses a weak CAS (ATOMIC_COMPARE_EXCHANGE_WEAK_INT) on PLATFORM_ATOMIC
builds which requires retry loops but callers (refc_binary.c caller at the CF
where refc_binary_cas_refcount is invoked and the resources.c caller) don't
retry; also refc_binary_get_refcount does a plain non-atomic read causing races.
Fix by replacing the weak CAS with a strong compare-exchange on the
PLATFORM_ATOMIC path (or emulate strong semantics by looping on the weak CAS
until success/expected changes) inside refc_binary_cas_refcount, and change
refc_binary_get_refcount to perform an atomic load of ref_count (using the same
platform-atomic API or C11 atomic_load equivalent) so reads are race-free;
ensure the function names refc_binary_cas_refcount and refc_binary_get_refcount
are updated and callers no longer need to implement retries.
Comment on lines
+1378
to
+1380
| struct ResourceBinary *resource_binary = enif_alloc_resource(glb->resource_binary_resource_type, sizeof(struct ResourceBinary)); | ||
| resource_binary->managing_resource = refc_binary_from_data(obj); | ||
| resource_binary->data = data; |
There was a problem hiding this comment.
Handle enif_alloc_resource() failure before using resource_binary.
If enif_alloc_resource() returns NULL, Line 1379 dereferences it immediately and crashes the VM instead of failing cleanly.
🐛 Proposed fix
term term_from_resource_binary(void *obj, const void *data, size_t size, Heap *heap, GlobalContext *glb)
{
struct ResourceBinary *resource_binary = enif_alloc_resource(glb->resource_binary_resource_type, sizeof(struct ResourceBinary));
+ if (UNLIKELY(resource_binary == NULL)) {
+ return term_invalid_term();
+ }
resource_binary->managing_resource = refc_binary_from_data(obj);
resource_binary->data = data;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| struct ResourceBinary *resource_binary = enif_alloc_resource(glb->resource_binary_resource_type, sizeof(struct ResourceBinary)); | |
| resource_binary->managing_resource = refc_binary_from_data(obj); | |
| resource_binary->data = data; | |
| struct ResourceBinary *resource_binary = enif_alloc_resource(glb->resource_binary_resource_type, sizeof(struct ResourceBinary)); | |
| if (UNLIKELY(resource_binary == NULL)) { | |
| return term_invalid_term(); | |
| } | |
| resource_binary->managing_resource = refc_binary_from_data(obj); | |
| resource_binary->data = data; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libAtomVM/term.c` around lines 1378 - 1380, enif_alloc_resource(...) can
return NULL; check the returned pointer before dereferencing resource_binary in
this allocation block (the allocation of struct ResourceBinary using
enif_alloc_resource with glb->resource_binary_resource_type), and when NULL
handle the failure cleanly (e.g., free any prior resources if needed and return
an error/NULL from the enclosing function) instead of proceeding to call
refc_binary_from_data(obj) or assign resource_binary->data; add a NULL-check for
resource_binary and an appropriate early return/cleanup path so ResourceBinary
is only dereferenced when allocation succeeds.
Comment on lines
+153
to
+180
| #ifndef AVM_NO_SMP | ||
| static inline size_t smp_atomic_fetch_add_size(size_t *object, size_t delta) | ||
| { | ||
| critical_section_enter_blocking(&atomic_cas_section); | ||
| size_t old = *object; | ||
| *object = old + delta; | ||
| critical_section_exit(&atomic_cas_section); | ||
| return old; | ||
| } | ||
|
|
||
| static inline size_t smp_atomic_fetch_sub_size(size_t *object, size_t delta) | ||
| { | ||
| critical_section_enter_blocking(&atomic_cas_section); | ||
| size_t old = *object; | ||
| *object = old - delta; | ||
| critical_section_exit(&atomic_cas_section); | ||
| return old; | ||
| } | ||
|
|
||
| static inline size_t smp_atomic_fetch_or_size(size_t *object, size_t mask) | ||
| { | ||
| critical_section_enter_blocking(&atomic_cas_section); | ||
| size_t old = *object; | ||
| *object = old | mask; | ||
| critical_section_exit(&atomic_cas_section); | ||
| return old; | ||
| } | ||
| #endif // AVM_NO_SMP |
There was a problem hiding this comment.
Add the AVM_NO_SMP fallback for these new fetch helpers.
Line 153 gates all three APIs out of single-core builds, while the existing compare/exchange helpers in this header still provide atomicity there by disabling interrupts. That makes the new helpers unavailable or non-portable for AVM_NO_SMP configurations. Please mirror the same save_and_disable_interrupts() / restore_interrupts() path here instead of omitting the functions entirely.
Suggested fix
-#ifndef AVM_NO_SMP
static inline size_t smp_atomic_fetch_add_size(size_t *object, size_t delta)
{
+#ifndef AVM_NO_SMP
critical_section_enter_blocking(&atomic_cas_section);
+#else
+ uint32_t save = save_and_disable_interrupts();
+#endif
size_t old = *object;
*object = old + delta;
+#ifndef AVM_NO_SMP
critical_section_exit(&atomic_cas_section);
+#else
+ restore_interrupts(save);
+#endif
return old;
}
static inline size_t smp_atomic_fetch_sub_size(size_t *object, size_t delta)
{
+#ifndef AVM_NO_SMP
critical_section_enter_blocking(&atomic_cas_section);
+#else
+ uint32_t save = save_and_disable_interrupts();
+#endif
size_t old = *object;
*object = old - delta;
+#ifndef AVM_NO_SMP
critical_section_exit(&atomic_cas_section);
+#else
+ restore_interrupts(save);
+#endif
return old;
}
static inline size_t smp_atomic_fetch_or_size(size_t *object, size_t mask)
{
+#ifndef AVM_NO_SMP
critical_section_enter_blocking(&atomic_cas_section);
+#else
+ uint32_t save = save_and_disable_interrupts();
+#endif
size_t old = *object;
*object = old | mask;
+#ifndef AVM_NO_SMP
critical_section_exit(&atomic_cas_section);
+#else
+ restore_interrupts(save);
+#endif
return old;
}
-#endif // AVM_NO_SMP📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #ifndef AVM_NO_SMP | |
| static inline size_t smp_atomic_fetch_add_size(size_t *object, size_t delta) | |
| { | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| size_t old = *object; | |
| *object = old + delta; | |
| critical_section_exit(&atomic_cas_section); | |
| return old; | |
| } | |
| static inline size_t smp_atomic_fetch_sub_size(size_t *object, size_t delta) | |
| { | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| size_t old = *object; | |
| *object = old - delta; | |
| critical_section_exit(&atomic_cas_section); | |
| return old; | |
| } | |
| static inline size_t smp_atomic_fetch_or_size(size_t *object, size_t mask) | |
| { | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| size_t old = *object; | |
| *object = old | mask; | |
| critical_section_exit(&atomic_cas_section); | |
| return old; | |
| } | |
| #endif // AVM_NO_SMP | |
| static inline size_t smp_atomic_fetch_add_size(size_t *object, size_t delta) | |
| { | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| `#else` | |
| uint32_t save = save_and_disable_interrupts(); | |
| `#endif` | |
| size_t old = *object; | |
| *object = old + delta; | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_exit(&atomic_cas_section); | |
| `#else` | |
| restore_interrupts(save); | |
| `#endif` | |
| return old; | |
| } | |
| static inline size_t smp_atomic_fetch_sub_size(size_t *object, size_t delta) | |
| { | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| `#else` | |
| uint32_t save = save_and_disable_interrupts(); | |
| `#endif` | |
| size_t old = *object; | |
| *object = old - delta; | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_exit(&atomic_cas_section); | |
| `#else` | |
| restore_interrupts(save); | |
| `#endif` | |
| return old; | |
| } | |
| static inline size_t smp_atomic_fetch_or_size(size_t *object, size_t mask) | |
| { | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_enter_blocking(&atomic_cas_section); | |
| `#else` | |
| uint32_t save = save_and_disable_interrupts(); | |
| `#endif` | |
| size_t old = *object; | |
| *object = old | mask; | |
| `#ifndef` AVM_NO_SMP | |
| critical_section_exit(&atomic_cas_section); | |
| `#else` | |
| restore_interrupts(save); | |
| `#endif` | |
| return old; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platforms/rp2/src/lib/platform_atomic.h` around lines 153 - 180, The
three helpers smp_atomic_fetch_add_size, smp_atomic_fetch_sub_size, and
smp_atomic_fetch_or_size are currently omitted under AVM_NO_SMP; add the same
single-core fallback used by the compare/exchange helpers by providing
implementations guarded for AVM_NO_SMP that use save_and_disable_interrupts() to
disable interrupts, perform the read-modify-write (compute old, update *object),
then restore_interrupts() before returning old, so the functions are available
and atomic on single-core builds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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