Skip to content

Conversation

@JaagupAverin
Copy link
Contributor

@JaagupAverin JaagupAverin commented Nov 29, 2025

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.

@JPHutchins
Copy link
Collaborator

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!

@JPHutchins
Copy link
Collaborator

Type ignores can be OK for mocking and monkey patching in the tests.

@JaagupAverin JaagupAverin force-pushed the main branch 3 times, most recently from 0737650 to f1108df Compare November 29, 2025 20:13
@JaagupAverin
Copy link
Contributor Author

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.
Will be testing and trying this out a bit more thoroughly in the next week myself.

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.
Previously the packet read timeout wasn't actually used, as it was passed to Serial, but serial was read with .read_all(), which does not respect any timeouts.
Rename, reformat and clean up some code from the previous commit.
@JaagupAverin JaagupAverin marked this pull request as draft December 1, 2025 14:05
Single bytes cannot be flushed to serial if they may be starts of SMP packets. Technically this introduces an edge case where 0x04 or 0x06 is the very last character of a valid serial data stream, and will then not get flushed properly, but the chances of that are very small, and there aren't any good workarounds to this.
@JaagupAverin JaagupAverin marked this pull request as ready for review December 2, 2025 12:59
@JaagupAverin JaagupAverin force-pushed the main branch 2 times, most recently from 835d6af to 634f5ce Compare December 18, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants