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

Inform application of MTU when reading #47

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

chrysn
Copy link
Contributor

@chrysn chrysn commented Aug 3, 2022

This fixes the first half of #46 (informing the application, not checking), with some caveats:

  • This is an API breaking change as users might destructure the ReadRequest struct. Future breakage of that kind is avoided by making the struct non_exhaustive.
  • I don't know if 23 is really the default value -- and if the kernel guarantees that it's here, it might be better just to unwrap.
  • Informing the application of the negotiated MTU is correct but requires some knowledge of mapping between MTUs and value lengths (which have a difference of 3). It might be better API to just expose the relevant length; ideally it should use the established name for that maximum. (Then the default would be 20).

The second half is harder to fix, as the response is sent through a channel without a back channel. It might be an option just to panic -- after all, it's like running copy_from_slice with an overly large slice.

@chrysn
Copy link
Contributor Author

chrysn commented Mar 10, 2023

Ping -- how can we make progress here?

Do you have preferences as to whether the reported MTU or the usable slice length should be reported, and maybe experience as to whether that offset is always 3? (AIU the BLE specs it is, but I don't have a good overview of it.)

@dfrankland dfrankland merged commit b2d374a into dfrankland:master Mar 11, 2023
@dfrankland
Copy link
Owner

Sorry about that. I haven't been paying much attention since I stopped working on my BLE project. I was able to see that everything could build (I wasn't able to test), so I merged and published this.

@chrysn chrysn deleted the inform-mtu branch July 23, 2024 19:37
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