fix: ethernet frame header is 14 not 18

[Easyoakland]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

ETHERNET_HEADER_FRAME_SIZE = 18 in the following code

esp-hal/esp-wifi/src/wifi/mod.rs

Line 76 in f247b40

const ETHERNET_FRAME_HEADER_SIZE: usize = 18;

is only used here

esp-hal/esp-wifi/src/wifi/mod.rs

Line 950 in f247b40

const DATA_FRAME_SIZE: usize = MTU + ETHERNET_FRAME_HEADER_SIZE;

which is only used here

esp-hal/esp-wifi/src/wifi/mod.rs

Line 2715 in f247b40

static mut BUFFER: [u8; DATA_FRAME_SIZE] = [0u8; DATA_FRAME_SIZE];

I thought maybe the extra 4 bytes were for the fcs at the end but the buffer is passed with the length given by caller here

esp-hal/esp-wifi/src/wifi/mod.rs

Lines 2717 to 2721 in f247b40

	let buffer = unsafe { &mut BUFFER[..len] };
	let res = f(buffer);
	esp_wifi_send_data(self.mode.interface(), buffer);

so that can't be it.

The only reason I noticed this is because smoltcp doesn't support ipv6 fragmentation and ignores if the given MTU is too large (see here) and I tried to send a max MTU UDP packet. It panicked indexing [..len] so I calculated what the max non-fragmenting size would be like so:

    pub const UDP_FORWARD_HEADER_LEN: usize = {
        let out = smoltcp::wire::IPV6_HEADER_LEN
            + smoltcp::wire::UDP_HEADER_LEN
            + size_of::<PacketHeader>(); // repr(C) custom protocol header in the udp payload
        assert!(out == 56);
        out
    };
    pub const MTU: usize = 1492 // matching ESP32 ESP_WIFI_CONFIG_MTU
    // minus the headers
    - UDP_FORWARD_HEADER_LEN;

but was surprised to find the calculated MTU seemed to be 4 bytes less than the panic message implied was valid. I believe I tracked down the source of this discrepancy to the changed line in the PR. As seen here and here an ethernet header is 14 bytes not 18.

I don't know if this would be considered public facing enough to warrant a change log entry, but I'm pretty sure it's a bug.

If my analysis is wrong and the fcs is included in that sizing, then there should at least be an assert that the passed length matches the configured MTU and not config.MTU+4 as it does now. The index check of [..len] should optimize out if the assert occurs before it.

Sidenote: It's a good thing the buffer index was checked and panicked in the esp-wifi code or I probably would never have figured this out. Also, I'll file an issue with smoltcp to at least warn when a packet which is too large and can't be fragmented needs to be sent.

Testing

See above.

[bjoernQ]

Not sure / don't remember where the 18 is actually from.

Given https://docs.rs/smoltcp/0.12.0/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_transmission_unit

We probably should clarify the description of ESP_WIFI_CONFIG_MTU (i.e. that it is not including the frame header)

[bjoernQ]

I think we just size our internal buffer larger than needed and we don't even need to account for the ethernet frame header since max_transmission_unit already includes it

The problem you experienced was fixed by you in smoltcp-rs/smoltcp#1038

i.e. we set DeviceCapabilities::max_transmission_unit so we shouldn't get anything larger than that from smoltcp and we can just size our buffer as MTU

[Easyoakland]

Testing looks like it works with the change (at least with the default 1492).
Do you know what the min,max supported values of the config.MTU is?

[bjoernQ]

Awesome - smoltcp says

Most common IP MTU is 1500. Minimum is 576 (for IPv4) or 1280 (for IPv6). Maximum is 9216 octets.

I haven't found any documentation on the drivers itself about it

[Easyoakland]

I saw this which claims a max of 4017, but don't know if there's better info elsewhere or even if that is accurate.

[bjoernQ]

I saw this which claims a max of 4017, but don't know if there's better info elsewhere or even if that is accurate.

Interesting - but not sure if that's really the case and even if it was that issue it's quite old unfortunately