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

Initial impl of escaped SOF #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mon
Copy link
Contributor

@mon mon commented Oct 8, 2019

Related to #16

I'm not entirely happy with this impl - while it's simple enough, it completely removes buffering on TF_WriteImpl. I could improve this to fill the buffer with escaped bytes, or split buffer writes on each escaped char, but it's a bit more code refactoring.

I've tested this internally and it works, and when disabled does not affect old code. Thoughts on the 1-byte-at-a-time aspect? Not sure if it's mergeable.

I also have an impl of this for PonyFrame, if you like. It's much simpler, at least.

@MightyPork
Copy link
Owner

Need to read it more closely, but at first glance it looks good. I'll try to write some actual unit tests so it can be properly tested.

The one-byte-at-a-time is not great, could you try rewriting it so it sends contiguous chunks in one call to TF_WriteImpl? Keep track of the last plain character and then send the "buffered" plain portion before escaping the offending char.. and don't forget to send the leftover at the end of the loop.

@iceberg1369
Copy link

This is a good idea to have an escape char . I have seen tf lost the sync with the stream becsuse of sof byte in the middle. So this is a must have.

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