serial_device read_line() does not parse lines correctly

[mstovenour]

Under certain circumstances read_line() does not properly detect the \r\n end of line sequence. This function reads all the data waiting in the serial device buffer and parses it up to the first \r\n. It then saves any remainder in self._buffer for the next function invocation. If that remainder contains a \r\n then the newline is not detected. This results in messages that are concatenated together with \r\n between them.

This occurs because any data waiting in self._buffer is not parsed for newlines.

[mstovenour]

I'll submit a pull request as a proposed fix.

[f34rdotcom]

I am still reviewing. I have a few ideas on this code. Still amazed this has not shown up and I need to run more tests to see why / how it never showed up. It may be how serial is processed differs in Python3 and this never somehow happened.

[mstovenour]

The algorithmic flaw in the current code is clear. As are the conditions necessary to trigger the flaw. If the alarm decoder writes out two or more complete lines before read_line is called by the library, then the second call to read_line will return a string with newlines imbedded. All you need to do to prove this is put a big delay in the polling loop that calls read_line. So a system that gets busy or is running on the slow side would be more suceptiable. Python3 May also introduce more delay. Keep in mind that I’m running this from home assistant which may cause pauses in the python cooperative threading model. It just all comes down to being to slow to capture every line as output by alarmdecoder.
When running on my system, I saw this in two ways. First was that the start up messages were alway concatinated but I suspect this was benign because those are not parsed. Second when running, the alarm decoder is printing the keypad status message on a timer. If that status message timer just happens to trigger immediately ahead of a real event like a zone restoral, then the zone restoral is concatenation on the end of the status message. The message parsing code assume only one line is present and treasts that data as a status message never seeing the zone restoral.

[mstovenour]

I also wrote a version of the serial_device read_line that looks exactly like the usb_device read_line. It required a little more CPU time because it only reads one character at a time from the device input buffer. I can paste that into a gist if you want to experiment with it.

[f34rdotcom]

The more I look at this function the more I hate it :) Your patch seems fine but it still potentially very inefficient and may scan the same buffer over and over again for the EOL. It relies heavily on how fast the host is vs how fast the data is received and with lower speed devices like the AD2Serial it may have other to scan the buffer several times. A better approach is to read a single character at a time and track a state var when the line is finished like how I did the serial code in the AD2 C# GUI. I would like to see your efforts on reading one character at a time.

[ajschmidt8]

@mstovenour, any chance you'll be continuing work on this? I believe this issue is related to #47 which I just opened up.

[ajschmidt8]

@mstovenour or @f34rdotcom, I'm trying to test @mstovenour's PR fix that was merged by @f34rdotcom. however, because this issue was intermittent, it makes it hard to confirm whether or not the PR has resolved it. do either of you know how I might be able to force this issue to occur?

[ajschmidt8]

@mstovenour, the recent 1.13.10 release should resolve this. It incorporates the fixes below which seem to have resolved serial reading issues in my local testing.

d809cb3
#56

You can get the latest version from PyPI. Let me know if you're still experiencing issues on the latest release. If not, we can close this issue.

https://pypi.org/project/alarmdecoder

[ajschmidt8]

Closing issue since @mstovenour has not responded and the issue should be resolved in 1.13.10.