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

Serial buffer size fix #11448

Closed

Conversation

eriktews
Copy link

@eriktews eriktews commented Mar 24, 2021

Description:

This PR fixes an issue I have with v9.3.1 when I try to get data from my P1 smart meter and report them via MQTT. The smart meter sends datagrams via the serial line (a chunk of about 600-700 bytes once a second) and sometimes the data sent via MQTT looks like there are some bytes missing. I assumed that this is caused by a too small RX buffer of the serial line and when the buffer is full, data from the smart meter is dropped. The single like change here fixes that, at least on my system.

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works on Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with core ESP32 V.1.0.5
  • I accept the CLA.

This commit fixes serial receive buffer overflows by increasing the size of the serial receive buffer to INPUT_BUFFER_SIZE (520) bytes. It was previously disabled in 4aa1c18 but that leads to some bytes missing when trying to received data from a smart meter using the P1 port and publishing them via MQTT.
@arendst arendst added the on hold by dev team Result - Feature request put on hold by member of development team label Mar 24, 2021
@arendst
Copy link
Owner

arendst commented Mar 31, 2021

Implementing this globally is a waste of RAM. If you need a larger buffers for a specific use case you will need to do this in the specific (Smart meter) driver.

@eriktews
Copy link
Author

There is unfortunately no specific smart meter driver. Instead, I just configured Tasmota to forward everything received from the serial line split by "\n" to an MQTT server using the "serial-bridge" driver:

However, the bridge has a few commands and one might add an additional command that controls the serial buffer size. So only when this command is activated, the buffers would be set. What do you think about a: SerialBufferSize command, which controls the buffer size and when -1 is passed as an argument, the buffer is left unchanged?

@arendst
Copy link
Owner

arendst commented Mar 31, 2021

Let me think it over. I see the need indeed.

@eriktews
Copy link
Author

Maybe a few more comments on this:

When I first say the data that looked like there were a few things missing at the MQTT server, my first assumption was that there would be something wrong with the hardware such as a faulty cable. After a few hours of debugging, I realized that this was an issue with serial line buffering.

The patch I implemented fixes the issue for me and maybe for a few others. However those who might have a different MQTT server or a slower WiFi might still see issues and I think this can't be fixed perfectly just with increased buffer sizes, at least when you need to take things like the network going down for a few seconds, into account. The best thing you can do is probably make the buffer configurable so that those who need larger buffers can configure them without having to recompile the firmware. Those who don't need them (there are probably a lot of users who don't need them, otherwise the issue would have been reported earlier) have more free RAM.

What would be really helpful is when you could detect serial data loss due to full buffers at runtime and then write a log message. This way, people with the issue would at least know that something there is going wrong and that is can probably be fixed by changing a setting. However I don't know the ESP and Tasmota well enough to implement such a warning.

@arendst
Copy link
Owner

arendst commented Mar 31, 2021

Serial buffer overrun detection is present only on the serial command interface.

I'll add it to the hardware SerialBridge interface too now.

Implementing user control over serial buffer size is a different story. Currently it uses both static and dynamic buffer size declaration both to keep RAM footprint as small and unfragmented as possible. The static buffers can only be resized during a restart. I'll have to look for a way to keep the static buffer fixed while adding another user controlled size dynamic buffer to copy the static buffer data into.

@arendst
Copy link
Owner

arendst commented Mar 31, 2021

BTW what happens if you change sleep from default 50 to 0?

arendst added a commit that referenced this pull request Mar 31, 2021
Add buffer overrun message on SerialBridge (#11448)
@eriktews
Copy link
Author

Hi, I haven't tried the change to sleep yet, but I will look into it.

Regarding changing the buffer size at runtime: I think it should be possible to call Serial.setRxBufferSize with an argument x that is determined at runtime, based at for example a configuration setting. Calling Serial.setRxBufferSize is all what my patch does, it doesn't change the size of the global static buffer used here, which would be rather hard to change at runtime. I have just chosen INPUT_BUFFER_SIZE as an argument since it's 256, but it doesn't have to be 256.

I suggest to use LOG_WARN for the warning message since this is an event that will very likely lead to an error somehow (lost data at least).

arendst added a commit that referenced this pull request Apr 1, 2021
Add command ``SerialBuffer 256..520`` to change hardware serial receive buffer size from default (256) to max local buffer size (520) (#11448)
@arendst
Copy link
Owner

arendst commented Apr 1, 2021

I released command SerialBuffer allowing a user to change the hardware serial receive buffer from default 256 to max 520.

Notice that the setting is not persistent (on purpose) so if you need it always us a rule to set it on restart.

@arendst arendst closed this Apr 1, 2021
@eriktews
Copy link
Author

eriktews commented Apr 1, 2021

OK, that sounds reasonable. I assume there might be devices that crash when you set the buffer to 520, so when that would be automatically persisted, this would result in the device ending up in a boot loop. Hopefully people will only add the rule when they managed to run the command once manually.

One thing left to do is to change the log level to warning and add a hint that this can be potentially fixed using the SerialBuffer command.

@GhielenRM
Copy link

I have a bit of a followup on this one.
Using ESP-01 to connect to my Smart Meter (Type ZIV).
Generates ~900 characters every second.
I am consistently getting "Serial buffer overrun" messages and missing essential parts of the smart meter message.

Using Tasmota 12.0.2
SerialBuffer set to 520
Using MQTT to convey the packets to my HA system.
Generic 18 template an NO other sensors or devices attached.

Tried to change Sleep values.
At values of 5 it reduced the amount of Serial buffer errors (Typical 5, now 2-3), but started to get into problems with MQTT.

Looked through the documentation, issues, etc, but running out of options to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold by dev team Result - Feature request put on hold by member of development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants