Support for Async Ethernet Mode#149
Open
umarbinzahid wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds PRNG byte-generation and Ethernet packet operations to winc-rs's async/sync op framework, refactors the client Ethernet APIs around these operations, implements embassy_net_driver::Driver for AsyncClient with waker deduplication, and adds a feather_async bypass_mode ICMP ping example with embassy-net feature wiring. ChangesPRNG, Ethernet ops, AsyncClient driver, examples
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces embassy-net integration for the WINC Wi-Fi driver, adding Ethernet bypass mode and PRNG examples, reorganizing internal operations from net_ops to ops, and implementing the embassy_net_driver::Driver trait. Feedback on these changes highlights several critical issues in the async implementation: an infinite loop {} in the ping task will block the cooperative Embassy executor, and poll-count-based timeouts in both the Ethernet receive and PRNG operations will expire prematurely in tight async polling loops. Additionally, querying the MAC address synchronously on every hardware address request is highly inefficient and risks RefCell borrow panics, and the driver should dynamically report the link state rather than unconditionally returning LinkState::Up.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @umarbinzahid, you have reached your weekly rate limit of 1500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@feather_async/examples/bypass_mode.rs`:
- Around line 101-103: The `loop {}` statement in the error handling block after
the `wait_for_dhcp(&stack).await` call is a busy-spin loop that starves the
cooperative executor. Replace this non-yielding infinite loop with a proper
async construct that yields control back to the executor, such as
`futures::future::pending().await`. Apply the same fix to the other instance of
`loop {}` mentioned at line 145 to ensure the executor can properly schedule
other tasks.
- Around line 166-169: The log statement at lines 166-169 is logging the
plaintext Wi-Fi password in the format string with the password parameter.
Remove the password variable from the log output by deleting the "with password:
{}" portion from the format string and removing the password argument from the
println! macro call. Only log the SSID network name to avoid exposing sensitive
credentials in debug logs and persisted traces.
- Line 63: The code on line 63 uses unwrap() on config.gateway which will panic
if the gateway is not present in the DHCP configuration. Instead of unwrapping,
use a safer approach such as unwrap_or() to provide a default gateway value
(like all zeros), or use an Option pattern to handle the case where gateway is
legitimately missing from the DHCP config. This ensures the code handles missing
gateways gracefully without panicking.
- Line 158: The u16::from_str(test_count).unwrap() call will panic if the input
is invalid, causing a hard crash instead of graceful error handling. Replace the
unwrap() call with the error propagation operator (?) to return the error up the
call stack, allowing the calling function to handle the parsing failure
appropriately. This converts a panic into a recoverable error condition that can
be handled by the caller.
In `@winc-rs/src/async_client/ethernet.rs`:
- Around line 16-18: The documentation for the `read_ethernet_packet` method
currently describes the `buffer` parameter as a required mutable slice, but the
actual function signature defines it as `Option<&mut [u8]>` which is optional.
Update the doc comment for the `buffer` parameter to clarify that it is optional
and describe what happens when `None` is passed (typically using the default
buffer or behavior). Apply the same documentation fix to all other occurrences
of this parameter documentation in the method.
- Around line 103-107: The error handling in the poll_once result block treats
all errors the same by returning None, which masks real module/transport faults
and makes diagnostics difficult. Instead of using a simple else block that
returns None for all errors, match on the specific error types returned from
poll_once: keep GeneralTimeout errors as "no packet" (returning None silently),
but explicitly log any other unexpected errors with their details before
returning None, so real faults are visible in the logs rather than silently
hidden.
In `@winc-rs/src/ops/module/prng.rs`:
- Around line 72-73: The send_prng function calls are passing incorrect pointer
addresses. At line 72, the call passes the address of the temporary Prng struct
itself rather than the actual buffer data it contains, and at line 96 a similar
issue occurs with the slice reference. Instead of using the struct address or
reference address with pointer casts, obtain the actual buffer pointer using
as_ptr() method on the underlying buffer data within the Prng struct, then cast
that address to u32 for both send_prng calls. This ensures the firmware receives
the correct buffer address as expected by the wire protocol.
In `@winc-rs/src/ops/net_ops/ethernet_receive.rs`:
- Around line 57-60: In the timeout conversion calculation on the line with
manager.set_operation_timeout, the expression (timeout_ms * 1000) / 100 can
overflow when timeout_ms is large because the multiplication happens before the
division. Cast timeout_ms to u64 before performing the multiplication to prevent
overflow, ensuring the intermediate result fits safely before the final division
and conversion back to the required type for set_operation_timeout.
- Around line 63-87: When the caller passes an empty buffer (length 0), the code
calculates len_to_read as 0, which leaves rx_done as false (since 0 is not >=
info.packet_size), resulting in no state advancement (data_offset and
packet_size both remain unchanged) while callbacks.eth_rx_info stays active.
This causes the receive operation to get stuck in the same state on the next
call. Add an explicit check after calculating len_to_read to detect when it is 0
and handle this case by clearing callbacks.eth_rx_info to prevent the stuck
receive state, ensuring the operation doesn't loop indefinitely on an empty
buffer scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15441869-3b2b-4f98-93c6-f7599547af37
📒 Files selected for processing (34)
feather_async/Cargo.tomlfeather_async/examples/bypass_mode.rsfeather_async/examples/prng.rswinc-rs/Cargo.tomlwinc-rs/src/async_client/dns.rswinc-rs/src/async_client/ethernet.rswinc-rs/src/async_client/mod.rswinc-rs/src/async_client/module.rswinc-rs/src/async_client/tcp_stack.rswinc-rs/src/async_client/udp_stack.rswinc-rs/src/client.rswinc-rs/src/client/dns.rswinc-rs/src/client/ethernet.rswinc-rs/src/client/prng.rswinc-rs/src/client/tcp_stack.rswinc-rs/src/client/udp_stack.rswinc-rs/src/client/wifi_module.rswinc-rs/src/lib.rswinc-rs/src/manager.rswinc-rs/src/manager/net_types.rswinc-rs/src/ops/mod.rswinc-rs/src/ops/module/mod.rswinc-rs/src/ops/module/prng.rswinc-rs/src/ops/net_ops/dns.rswinc-rs/src/ops/net_ops/ethernet_receive.rswinc-rs/src/ops/net_ops/mod.rswinc-rs/src/ops/net_ops/tcp_accept.rswinc-rs/src/ops/net_ops/tcp_connect.rswinc-rs/src/ops/net_ops/tcp_receive.rswinc-rs/src/ops/net_ops/tcp_send.rswinc-rs/src/ops/net_ops/udp_receive.rswinc-rs/src/ops/net_ops/udp_send.rswinc-rs/src/ops/op.rswinc-rs/src/stack/socket_callbacks.rs
umarbinzahid
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
winc-rs/src/async_client/ethernet.rs (2)
96-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRegister the RX waker before polling
A packet that arrives in the gap between
poll_once()andregister_waker_if_new()can miss the wakeup, leaving the driver asleep with data ready.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@winc-rs/src/async_client/ethernet.rs` around lines 96 - 124, The RX path in EthernetAsyncClient::poll_next registers the waker too late, after poll_once(), which can miss a packet arrival in the gap. Move the register_waker_if_new(cx.waker().clone()) call to before polling for RxEthernetPacketInfo, and keep the existing unregister_waker(cx.waker()) handling after a packet is received to preserve wakeup correctness.
167-173: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid a shared fallback MAC address. On MAC read failure, every affected device still reports
DEFAULT_MAC_ADDRESS, which can collide on the network and break DHCP/ARP. Use a device-unique locally administered fallback or keep the interface down until the WINC MAC is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@winc-rs/src/async_client/ethernet.rs` around lines 167 - 173, The MAC fallback in `AsyncEthernetClient::hardware_address`/`get_winc_mac_address` currently returns `DEFAULT_MAC_ADDRESS` on failure, which can cause multiple devices to share the same address. Change the failure path to avoid a shared fallback by generating a device-unique locally administered MAC or by leaving the interface unavailable until a valid WINC MAC is read, and keep the behavior localized to the MAC read handling in `ethernet.rs`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@winc-rs/src/async_client/ethernet.rs`:
- Around line 96-124: The RX path in EthernetAsyncClient::poll_next registers
the waker too late, after poll_once(), which can miss a packet arrival in the
gap. Move the register_waker_if_new(cx.waker().clone()) call to before polling
for RxEthernetPacketInfo, and keep the existing unregister_waker(cx.waker())
handling after a packet is received to preserve wakeup correctness.
- Around line 167-173: The MAC fallback in
`AsyncEthernetClient::hardware_address`/`get_winc_mac_address` currently returns
`DEFAULT_MAC_ADDRESS` on failure, which can cause multiple devices to share the
same address. Change the failure path to avoid a shared fallback by generating a
device-unique locally administered MAC or by leaving the interface unavailable
until a valid WINC MAC is read, and keep the behavior localized to the MAC read
handling in `ethernet.rs`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b6aa0c9-7a06-42c4-8c04-20cf19a4a18c
📒 Files selected for processing (4)
feather_async/Cargo.tomlfeather_async/examples/bypass_mode.rswinc-rs/src/async_client/ethernet.rswinc-rs/src/manager.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- winc-rs/src/manager.rs
- feather_async/examples/bypass_mode.rs
umarbinzahid
force-pushed
the
async_ethernet_mode
branch
from
b8def6f to
f800de0
Compare
Owner
|
This one also needs a rebase now |
umarbinzahid
force-pushed
the
async_ethernet_mode
branch
from
f800de0 to
d7cc5e2
Compare
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
2 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.
This PR adds support for Ethernet mode in async and implements an example using embassy-net to demonstrate the feature.
Summary by Sourcery
Add async Ethernet-mode support with embassy-net integration and refactor PRNG and Ethernet operations into reusable ops, updating both sync and async clients and providing new examples.
New Features:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes