search

SolidGeek / VescUart

An Arduino library for interfacing with the VESC over UART
GNU General Public License v3.0
177 stars 92 forks source link

Buffer overflow when receiving too much data #15

Open ArminBmrt opened 4 years ago

ArminBmrt commented 4 years ago

Hi,

there is a bufffer overflow when the serialPort receives too much data.

I have a esp32 connected to a vesc75/300. Sometimes the vesc sends too much data (unknown cause still working on this). The function int VescUart::receiveUartMessage(uint8_t * payloadReceived) has a buffer overflow which results in the esp32 crashing.

The problem is that the variable counter is checked after incrementing and writing of data.

https://github.com/SolidGeek/VescUart/blob/9fc9791517968184493077667aab83bcc74d23a2/src/VescUart.cpp#L64

When the counter is too large a break is executed. This will break only the second while loop (line 36) but not the first timeout while loop (line 34). So when counter has overflown line 38 still gets executed.

I think the check on counter on line 64 should be moved before anything is written

vicgwee commented 4 years ago

Might be a bit late, but you can try increasing the RX receive buffer on your esp32?

ArminBmrt commented 4 years ago

Yes it is a bit late :) but thanks

The buffer flow still exists even if you would increase the RX receive buffer. Problem is the that the size check is inside the second while loop and when the size is too large you break from it. This still keeps you in the first while loop which will happily increment the out of bound counter and write data to it.

I have moved on to a different library which now works (I still had to heavily modify the code to make it robust).

andrespineda commented 3 years ago

Hi, You mentioned that you have moved on to a different library. may I ask which library you are now using? Trying to find the best library to start with to save some time. Thanks in advance.

ArminBmrt commented 3 years ago

@andrespineda I used https://github.com/gianmarcov/arduino_vesc but I have modified a lot. Since I did not care about being backwards compatible I am not going to release it.

Main issues were the multiple uart ports for the esp32 and also the large stack memory allocation when receiving data. The large data allocation gave unreliable results when receiving. This might not be a problem if you only use a single vesc

andrespineda commented 3 years ago

Thank you! I am using an esp32 to control two (2) VESC in a mobile robot and may run into the same problem.

I have a modified CAN bus solution working but have similar problems with unreliable data. Was looking for a simpler solution as I am more interested in reliability than speed. Can't decide if I should keep hacking at the CAN bus work in progress or switch to UART. CAN bus is fairly simple for short messages. It is the large block of data across multiple packets that is a pain. I'll take a look at this code and try to decide.

Best of luck

On Mon, Sep 7, 2020 at 1:11 AM ArminBmrt notifications@github.com wrote:

@andrespineda https://github.com/andrespineda I used https://github.com/gianmarcov/arduino_vesc but I have modified a lot. Since I did not care about being backwards compatible I am not going to release it.

Main issues were the multiple uart ports for the esp32 and also the large stack memory allocation when receiving data. The large data allocation gave unreliable results when receiving. This might not be a problem if you only use a single vesc

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SolidGeek/VescUart/issues/15#issuecomment-688141426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWGEZRQJ3MI7TZ63BAKLRTSESITTANCNFSM4KMSOOFQ .

-- Best, Andres