search

OpenSourceEBike / TSDZ2-Smart-EBike

Flexible OpenSource firmware for TongSheng TSDZ2 mid drive ebike motor
GNU General Public License v3.0
251 stars 130 forks source link

Fix two bugs in UART packet parsing #154

Closed anszom closed 2 years ago

anszom commented 2 years ago

IMPORTANT: the analysis below is just a speculation, I have tracked this down while debugging an issue with my SW102 firmware, but I haven't checked these fixes yet, connecting the programmer to my bike is a bit cumbersome. I'd be grateful if someone could verify this.

The first bug is the condition in the while loop. The ring buffer is designed so that the read and write indices are equal if the buffer is empty. This means that the +1 in the loop condition is superfluous and will cause the parser to postpone reading the last received byte.

The second bug is a lack of 'return' when a full packet is received. If subsequent bytes are available in the ringbuffer, they will be processed as a beginning of a new packet, overwriting the one which hasn't yet been processed. This can happen frequently because of the first issue - the last byte of the previous packet will be processed ONLY if more bytes are available.

In effect, received packets frequently corrupt each other. The reason why this works at all, is that the packets are rarely changing, so that the overwritten bytes will most likely have the same values. When two different packets are sent, a CRC error occurs. However, if some value is changing continuously (as was the case with my firmware), all packets get rejected and the motor controller fails with a FATAL_ERROR.

With my SW102 firmware, I was even able to reproduce this by hammering on the +/- buttons. Your version seems to have more aggressive key debouncing (or some other limiting factor) and I've not been able to trigger the bug there.

As a simple workaround, the display can send one dummy byte after each packet. This has an effect of triggering the parser to consume the whole packet before a new one is received. In my tests, this eliminates the problem entirely.

casainho commented 2 years ago

Thanks, I need to read carefully all this information.

Are you using the configurations? Because I found an issue there that I already solved, but on the DIY display repository.

anszom commented 2 years ago

Are you using the configurations?

I'm not sure what you mean...

anszom commented 2 years ago

By the way, I've released my SW102 software here https://github.com/anszom/SW102_LCD. Maybe you'd be interested in seeing a different approach to the UI. Also, I'd be grateful if you mentioned it somewhere on the wiki :)

stsdc commented 2 years ago

Wow, @anszom, I dreamed about something like this!

casainho commented 2 years ago

By the way, I've released my SW102 software here https://github.com/anszom/SW102_LCD. Maybe you'd be interested in seeing a different approach to the UI. Also, I'd be grateful if you mentioned it somewhere on the wiki :)

I am very curious. I just saw your repo to try understand. But I have no SW102 and I will never have it again - can you please take some good pictures so you can use them to promote it?? Please post a message on the TSDZ2 forum (or I can do that for you if you do not do it, I really need that good pictures).

I did put a link on the wiki, at other projects section. See that I will very soon delete the references to the SW102 and 860C displays, as I decided to move on with the OpenSource display only, which happens to be very similar to SW102 - so I can take advantage of your firmware :-) - and so I will not give focus on your firmware, at least before I see it and eventually I have it working to the OpenSource display.