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

Disallow SOF in messages to handle mid-message death #16

Open
mon opened this issue Dec 20, 2018 · 6 comments
Open

Disallow SOF in messages to handle mid-message death #16

mon opened this issue Dec 20, 2018 · 6 comments

Comments

@mon
Copy link
Contributor

mon commented Dec 20, 2018

If one side of the communications dies mid-transfer, it could get "out of sync" with the state machine's idea of what things should look like.

One approach could be to have a SOF byte and an escape byte. If either of those bytes need to appear in the stream, send the escape byte and invert the bits of the required byte.

Thus, if you receive a SOF mid-message, you know the message is bad and to restart the state machine.

Thoughts?

@MightyPork
Copy link
Owner

It's a good idea, I'm only concerned about back compat. If you add it, please make it configurable, so it can be switched off.

@grantramsay
Copy link
Contributor

grantramsay commented Feb 1, 2020

I might be missing something, but wouldn't a simple back compatible way be to:

  • buffer entire packet when receiving, need to increase buffer size to handle full packet (header + payload + checksum/crc)
  • if checksum/crc fails or len > TF_MAX_PAYLOAD, memmove buffer to next SOF (if present) and reset rx index

This way the state machine will resynchronise on the next valid checksum/crc with no change to the message structure

Edit:
Also TF_ResetParser would reset the rx index, so reliable links without checksum/crc (e.g. TCP) could call it to resync if connection is restarted.
Additionally a timeout could be added to move to the next SOF. For the case where the link fails with a large but not invalid message length, and the next message is short. The state machine would need to receive extra rx data to resync, which may not be timely enough for the application

@MightyPork
Copy link
Owner

@grantramsay Sounds reasonable to me, this resyncing will work. I'd gladly merge that.

(I don't currently use TF in any active project, so I don't see implementing this myself soon..)

@mon
Copy link
Contributor Author

mon commented Feb 7, 2020

That would actually be quite an elegant solution, I might try implementing that

@grantramsay
Copy link
Contributor

@mon - I had a little go at this the other day.
It got a little more complicated than I'd like, but there might be something helpful here (definitely a "work in progress"):
master...grantramsay:wip/feat-16/sof-resync

@mon
Copy link
Contributor Author

mon commented Feb 8, 2020

Hmmm, certainly a little more complex than I expected. I'll experiment a bit over the next few weeks and see how reliable it is.

asmwarrior pushed a commit to asmwarrior/TinyFrame that referenced this issue Apr 25, 2024
…g-of-undefined-claimtx-and-releasetx

15 correct handling of undefined claimtx and releasetx
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

No branches or pull requests

3 participants