-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misc fixes and changes #3
base: master
Are you sure you want to change the base?
Conversation
This helps recovering from communication errors when connecting to already "busy" line mid-message.
This is causing some issues when using cross-compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mostly, my only problem is with buffer handling, see above.
@@ -78,15 +79,16 @@ impl SerialClient { | |||
received: &mut Vec<u8>, | |||
messages: &mut Vec<Message>, | |||
) -> Result<(), ClientError> { | |||
// log::debug!("Received: {:?}", received); | |||
self.buffer.append(received); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by removing this you actually made self.buffer
totally useless: it is never written, only read, but it will always be empty. The whole idea of this buffer was to support partial messages. On my experience it was possible to receive partial messages in read_and_decode
, that's why we had to save it in a buffer and append the next message to it and do the decoding on the merged message (not just the currently received stuff).
Of course it might be possible that your device is never sending these partial messages, and you have no need for this. But are you sure you have to remove this functionality? Is it causing a problem for you, or you just wanted to simplify things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this buffer thing was something we really struggled with whenever we connected our Coin Acceptor to the cctalk bus and received the data mid-packet.
Right now we have 30+ coin-acceptor devices working 24/7 and we haven't seen any issues on cctalk side.
@kmuuk Ideas?
This is a somewhat messy pull request containing most of the stuff we have accumulated, notably:
If needed, I can split it into separate pull requests.