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

Guarantee word-aligned writes #366

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Guarantee word-aligned writes #366

merged 1 commit into from
Jan 19, 2021

Conversation

philips77
Copy link
Member

This PR fixed #358. I still need to know why it is important before merging it.

@philips77
Copy link
Member Author

@SuhaibI Could you please review?

@SuhaibI
Copy link

SuhaibI commented Dec 9, 2020

@philips77 Really sorry I missed your pings, and this slipped my mind as we rolled the change into a private fork.

This change has no effect in the majority of cases I've seen with iOS because the nRF5 SDK logic negotiates an MTU that is word aligned - the nominal case with iOS is: iOS sends 185 as max MTU, device saves 183 as desired MTU, which ends up being word aligned.
I've seen an issue in the following rare case:

  • iOS rarely negotiates a much higher MTU (255 bytes)
  • Device sets maximum MTU to 255 bytes
  • On the next connection iOS now reports a max of 185 bytes - this becomes the negotiated MTU since it's the minimum of 185 and 255
  • Writes are no longer word aligned
    A potential improvement to the nRF5 SDK would be to stop initiating a MTU re-negotiation and depend on the client to make word-aligned writes. This could save some time during connection.

@philips77
Copy link
Member Author

Ok, let's merge it. I'll release it with the next version.

@philips77 philips77 merged commit b7efb89 into develop Jan 19, 2021
@philips77 philips77 deleted the bugfix/358 branch January 19, 2021 11:07
@philips77 philips77 mentioned this pull request Apr 29, 2021
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