Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Misc fixes and changes #3

wants to merge 14 commits into from

Conversation

plaes
Copy link
Contributor

@plaes plaes commented Mar 11, 2022

This is a somewhat messy pull request containing most of the stuff we have accumulated, notably:

  • Improves protocol decoding (we ran into various decoding issues when we attempted to connect to already communicating cctalk bus)
  • Drops libudev dependency from serialport (simplifies cross-compilation)
  • Updates clap library dependency required for examples
  • Adds some subcommands to cctalk-host example application (bunch of copy-paste and "rusty" code for now and requires eventual cleanup, but works..)
  • And includes also bunch of code smell fixes.

If needed, I can split it into separate pull requests.

Copy link
Member

@blackghost1987 blackghost1987 left a 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);
Copy link
Member

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?

Copy link
Contributor Author

@plaes plaes Apr 6, 2022

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?

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.

3 participants