Skip to content

cpu/esp32: esp_wifi improvements and cleanups#11997

Merged
benpicco merged 7 commits intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/esp_wifi/cleanups
Sep 5, 2019
Merged

cpu/esp32: esp_wifi improvements and cleanups#11997
benpicco merged 7 commits intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/esp_wifi/cleanups

Conversation

@gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Aug 12, 2019

Contribution description

This PR provides several improvements and cleanups that

  • reduces packet loss rate
  • increases stability
  • provides code similarity with esp_wifi of ESP8266 for future code deduplication

In detail the PR provides the following changes:

  1. Receive call back function _esp_wifi_rx_cb is called from WiFi hardware driver with a pointer to a frame buffer that is allocated in the WiFi hardware driver. This frame buffer was freed immediately after copying its content to a single local receive buffer of the esp_wifi netdev. The local receive buffer then remained occupied until the protocol stack had processed it. Further incoming packets were dropped.
    Very often, however, multiple consecutive WiFi frames are received simultaneously before the first is fully processed. Having the local receive buffer to hold only one received frame, led to a number of lost packets, even at low network load.
    Therefore, a ringbuffer of rx_buf elements was introduced which doesn't store the frames directly but only references to the frame buffers allocated in WiFi hardware driver. Since there is enough memory for a number of such frames, the frames buffers allocated in the WiFi hardware driver are no longer freed immediately, but are kept until the frame has been processed by the protocol stack. This leads to a much lower loss rate of packets.
  2. Instead of having a send buffer as member esp_wifi netdev, a local variable is now used as send buffer. This avoids the need for a locking mechanism and reduces the risk of deadlocks.
  3. Events of different types can be pending at the same time. Therefore it is not possible to use ascending identifiers for the presence of pending events. Rather, each event type has to be represented by one bit. Thes bits ORed identify all types of pending events. In the esp_wifi_isr function all pending events are then handled in one call. Otherwise, some events might be lost.
  4. Parameter checks were replaced by assertions.
  5. Some small cleanups.

Testing procedure

Compile and flash examples/gnrc_networking to one ESP32 node.

USEMODULE='esp_wifi' CFLAGS='-DESP_WIFI_SSID=\"ssid\" -DESP_WIFI_PASS=\"passphrase\"' make BOARD=esp32-wroom-32 -C examples/gnrc_networking flash
  1. Simultaneously ping the ESP32 node from different terminals or machines with very high, but different rates and different payload sizes:

    term 1> ping6 <ipv6_address>%eth0 -i 0.001
    
    term 2> ping6 <ipv6_address>%eth0 -i 0.004 -s 1200
    
    term 3> ping6 <ipv6_address>%eth0 -i 0.008 -s 1392
    

    The loss rate should be 0 % for all pings.

    Compare with current master, where the loss rates is already 20 % with only one ping.

    term 1> ping6 <ipv6_address>%eth0 -i 0.08
    

    The reason is the missing ring buffer for simultaneous incoming WiFi frames.

  2. Again, simultaneously ping the ESP32 node from different terminals or machines with high, but different rates and different payload sizes. Additionally, send UDP packets from the ESP32 node:

    term 1> ping6 <ipv6_address>%eth0 -i 0.02
    
    term 2> ping6 <ipv6_address>%eth0 -i 0.03 -s 1200
    
    term 3> ping6 <ipv6_address>%eth0 -i 0.04 -s 1392
    
    term 4> nc -6ul 12345
    
    esp32> udp send <ipv6_address of term4> 12345 "." 10000 10000
    

    Once the udp command has been finished, stop nc with

    term 5> killall -TERM nc
    

    and check the number of characters received by nc. It should be 10.000, that is 0 % loss rate. Also the ping commands should still have a loss rate of 0 %.

Issues/PRs references

Depends on PR #11947

@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 12, 2019
@gschorcht gschorcht force-pushed the cpu/esp32/esp_wifi/cleanups branch from a9fcab6 to f39e6d5 Compare August 12, 2019 14:43
@gschorcht gschorcht added the Platform: ESP Platform: This PR/issue effects ESP-based platforms label Aug 12, 2019
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 17, 2019
@benpicco
Copy link
Contributor

benpicco commented Sep 5, 2019

This looks good!
The esp32 can now keep up with even two computers flood-pinging it.

With ringbuffers you could even do without locking if you sacrifice one element. I thought tsrb would already provide a convenient implementation for that, but I don't see how it could guarantee integrity for records > 1 bytes.

Anyway, this is already a big improvement, we can always optimize it further in another PR if that's necessary.

@benpicco
Copy link
Contributor

benpicco commented Sep 5, 2019

Just squash already

Events of different type can be pending at the same time. Therefore it is not possible to use ascending identifiers for the presence of a pending event. Rather, each event type has to be represented by one bit. Thes bits ORed identify all types of pending events. In the esp_wifi_isr function all pending events are then handled in one call. Otherwise, some events might be lost.
Receive call back function `_esp_wifi_rx_cb` is called from WiFi hardware driver with a pointer to a frame buffer that is allocated in the WiFi hardware driver. This frame buffer was freed immediately after copying its content to a single local receive buffer of the `esp_wifi` netdev. The local receive buffer remained occupied until the protocol stack had processed it. Further incoming packets were dropped.  However, very often a number of subsequent WiFi frames are received at the same time before the first one is processed completely. Having the single local receive buffer to hold only one received frame, led to a number of lost packets, even at low network load. Therefore, a ringbuffer of rx_buf elements was introduced which doesn't store the frames directly but only references to the frame buffers allocated in WiFi hardware driver. Since there is enough memory to hold several frames, the frames buffers allocated in WiFi hardware driver aren't freed immediatly any longer but are kept until the frame is processed by the protocol stack. This results in a much less loss rate of packets.
Instead of having a send buffer as member `esp_wifi` netdev, a local variable is used now as send buffer. This avoids the need for a locking mechanism and reduces the risk of deadlocks.
@gschorcht gschorcht force-pushed the cpu/esp32/esp_wifi/cleanups branch from 63a8f04 to 6d77529 Compare September 5, 2019 10:51
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on esp32 with flood pings, dropped packets reduced significantly.

@benpicco benpicco merged commit 999fffd into RIOT-OS:master Sep 5, 2019
@gschorcht
Copy link
Contributor Author

@benpicco Thanks for reviewing and testing.

@gschorcht gschorcht deleted the cpu/esp32/esp_wifi/cleanups branch September 5, 2019 12:07
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@gschorcht gschorcht restored the cpu/esp32/esp_wifi/cleanups branch September 18, 2019 16:46
@gschorcht gschorcht deleted the cpu/esp32/esp_wifi/cleanups branch September 18, 2019 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: cpu Area: CPU/MCU ports Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants