Cleanup serial data handling #87
Conversation
|
Thanks! This should resolve #12, take a look for details. I hope it's possible to avoid creating tasks and rely on queues instead. Callback could be useful for flexibility, but I suspect that the average use case would be to await the device log "channel" - the queue. The refactor does appear to simplify the code, nice work! |
|
Type ignores can be OK for mocking and monkey patching in the tests. |
JaagupAverin
force-pushed
the
main
branch
3 times, most recently
from
0737650 to
f1108df
Compare
|
I think a lot of use cases are going to need a separate task for handling the serial data anyways so I wanted to hide it underneath the library. But I understand you'd rather not have this complexity. Made quick force push with new implementation. |
JaagupAverin
force-pushed
the
main
branch
2 times, most recently
from
835d6af to
634f5ce
Compare
JaagupAverin
force-pushed
the
main
branch
2 times, most recently
from
a135d38 to
4eace68
Compare
Makes the buffer processing hopefully a bit more clear. Non-SMP data is no longer logged to stdout but can be accessed with the read_serial() function.
JaagupAverin
force-pushed
the
main
branch
3 times, most recently
from
4eace68 to
74cf97f
Compare
|
Been using this for a bit now. Found and fixed a bug related to delimiter falling exactly on buffer boundary, but other than that its been churning gigabytes of intermingled serial and SMP data. So stability side it seems okay. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the serial data handling in SMPSerialTransport to improve clarity and maintainability. The changes separate SMP packet processing from non-SMP serial data handling, replacing the previous logging-based approach with a buffer-based API.
Changes:
- Refactored buffer processing state machine for clearer separation of SMP and serial data
- Introduced
read_serial()method to access non-SMP serial data programmatically instead of logging it - Added
_reset_state()to properly reset connection state between reconnections - Expanded test coverage for serial data handling scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| smpclient/transport/serial.py | Refactored buffer processing logic with new state machine, added read_serial() method, and improved connection state management |
| tests/test_smp_serial_transport.py | Updated tests to use new internal methods, added comprehensive tests for serial/SMP data separation, and added disconnect exception handling test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Contains full SMP packets.""" | ||
| self._serial_buffer = bytearray() | ||
| """Contains any non-SMP serial data.""" | ||
| self._buffer: bytearray = bytearray([]) |
There was a problem hiding this comment.
The empty list argument [] in bytearray([]) is unnecessary. Use bytearray() directly for clarity.
|
|
||
| self._smp_packet_queue = asyncio.Queue() | ||
| self._serial_buffer.clear() | ||
| self._buffer = bytearray([]) |
There was a problem hiding this comment.
The empty list argument [] in bytearray([]) is unnecessary. Use bytearray() directly for clarity.
| self._buffer = bytearray([]) | |
| self._buffer = bytearray() |
I'll review again, but it does look good to merge - thanks for testing! |
2 participants
chore: Cleanup non-smp serial data handling.
Makes the buffer processing hopefully a bit more clear. Non-SMP data is no longer logged to stdout but can be accessed with the read_serial() function.