Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: KNX empty UDP packets sent to the network #8459

Merged

Conversation

morgenroth
Copy link
Contributor

@morgenroth morgenroth commented May 16, 2020

Description:

The flush() call after read() causes empty packets being sent as response
for all UDP packets received on the KNX UDP port. In my tests with sonoff devices, the zero length packets mentioned in #8332 vanished after removing that flush() call.

Since I do not have any ESP32 device, it would be nice if someone else could test PR.

Related issue (if applicable): fixes #8332

Checklist:

  • The pull request is done against the latest dev branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR.
  • The code change is tested and works on core ESP8266 V.2.7.1
  • The code change is tested and works on core ESP32 V.1.12.0
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

The flush() call after read() causes empty packets being sent as response
for all UDP packets received on that port.
@ascillato
Copy link
Contributor

Nice.

Please, let me do some tests over this.

On ESP32 that issue didn't happen because the problem on empty response with flush command is just arduino core related and arduino core for ESP32 don't have that issue.

@arendst
Copy link
Owner

arendst commented May 17, 2020

@ascillato Adrian let me know if to merge.

@ascillato
Copy link
Contributor

ascillato commented May 17, 2020

Testing. Please, give me some time.

@ascillato
Copy link
Contributor

ascillato commented May 18, 2020

This Bug is only in the Arduino Core for ESP8266. It have happened before, it was fixed in the core but some changes in the core have reintroduce it.

The FLUSH command is needed in the ESP KNX Library. It can't be deleted. If deleted, in a busy network, your device will start to be unresponsive or slow to respond due to full UDP RX buffer on both ESP8266 and ESP32.

Checking the source code of the Cores, you can easily see that the flush command is not performing as it should, but only in ESP8266.

On Arduino ESP8266 Core - File /libraries/ESP8266WiFi/src/WiFiUdp.cpp:

void WiFiUDP::flush()
{
    endPacket();
}

int WiFiUDP::endPacket()
{
    if (!_ctx)
        return 0;

    return (_ctx->send()) ? 1 : 0;
}

On Arduino ESP32 Core - File /libraries/WiFi/src/WiFiUdp.cpp:

void WiFiUDP::flush(){
  if(!rx_buffer) return;
  cbuf *b = rx_buffer;
  rx_buffer = 0;
  delete b;
}

Therefore, I'm creating an issue in Arduino ESP8266 repository and closing this PR. Anyway, your research was very useful to find faster the bug in the core.


EDIT: Seems that the bug was introduced here esp8266/Arduino#3967 esp8266/Arduino@26980b3#diff-9480bf2495731fa559bb9c3019650538

@ascillato
Copy link
Contributor

ascillato commented May 18, 2020

As explained in:

they move the flush command to match the actual Arduino framework, being now a TX command instead of clearing buffer commands.

So, what was happening was intended from the core side.

Your change was correct and other optimizations should be implemented on the driver.

For now, it is better to merge your changes as it is and do the optimizations later on.

Thanks for sharing.

@ascillato2 ascillato2 reopened this May 18, 2020
@ascillato2 ascillato2 merged commit 70f51b7 into arendst:development May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KNX empty UDP packets sent to the network
4 participants