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

Serial Synchronization #24 #48

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

Conversation

tommywatson
Copy link
Contributor

@tommywatson tommywatson commented Oct 20, 2023

Fix for issue #24

I am unable to test it as I don't have any real hardware, I have attached a test program that runs it through short reads and invalid data.

main.c.txt

Copy link
Collaborator

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I think it looks to solve the issues.

components/bm1397/serial.c Outdated Show resolved Hide resolved
components/bm1397/serial.c Outdated Show resolved Hide resolved
components/bm1397/serial.c Outdated Show resolved Hide resolved
@tommywatson
Copy link
Contributor Author

I don't believe that this will resolve the issue. I think you still have an issue when receiving a smaller response everything will be off by a couple of bytes. i think potentially an easier solution might be to tighten the timeout to reduce the chance of getting multiple nonce responses but we also need to handle when we receive partial nonce response.

The test program does random short reads/0 reads and it resyncs with the incoming data.

@johnny9
Copy link
Collaborator

johnny9 commented Oct 20, 2023

I don't believe that this will resolve the issue. I think you still have an issue when receiving a smaller response everything will be off by a couple of bytes. i think potentially an easier solution might be to tighten the timeout to reduce the chance of getting multiple nonce responses but we also need to handle when we receive partial nonce response.

The test program does random short reads/0 reads and it resyncs with the incoming data.

yeah i think i understand it now.

components/bm1397/serial.c Outdated Show resolved Hide resolved
components/bm1397/serial.c Outdated Show resolved Hide resolved
@johnny9
Copy link
Collaborator

johnny9 commented Oct 20, 2023

Concept ACK.

I will try to replicate the issue on actual hardware to verify before merging.

@johnny9
Copy link
Collaborator

johnny9 commented Oct 20, 2023

Please squash commits as well and give a better commit message. Commit header should be clear with what was worked on and in the imperative mood. So something like, Ensure bm1366 serial data is the expected size. Then the description can include more details about the error and why. Issue number should go after the description/body of the commit.

Something like this for commit template.

<scope>: <short summary>

<optional body/description>

<related issue numbers>

tommy added 2 commits October 19, 2023 23:03
User reported bm1397 received an invalid packet and was unable to
continue without manually reset the device. This patch enables the
serial comms to resync when invalid/short data is received

issue skot#24
@johnny9
Copy link
Collaborator

johnny9 commented Nov 22, 2023

Sorry for the delays here. I'll do my best to get this tested soon. This shouldnt have been stuck in PR hell this long.

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