-
Notifications
You must be signed in to change notification settings - Fork 605
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
[nrf fromtree] Bluetooth: att: don't re-use the ATT buffer for confir… #1335
[nrf fromtree] Bluetooth: att: don't re-use the ATT buffer for confir… #1335
Conversation
@jori-nordic Some sdk-nrf update maybe? |
I thought that was a solved problem, that it triggered the right CI automatically, isn't that the case? |
sdk-zephyr is part of sdk-nrf setup, and this will not get merged unless pointed by the sdk-nrf west manifest. |
…mations If the peer is a zephyr host, there is no problem, as the Zephyr host limits sending parallel REQs and INDs. But the spec allows sending those in parallel, and it may end up that the re-used REQ buffer hasn't been destroyed when an indication comes. Only re-use the buffer when enqueuing ATT responses. This means that we may run out of buffers if the peer sends too many indications and our application also sends a lot of commands/notifications. The rationale for this is that having to handle a lot of requests is a more plausible scenario (e.g. being discovered by multiple peers) than handling lots of parallel indications. Signed-off-by: Jonathan Rico <[email protected]> (cherry picked from commit 7093538)
c0a6e18
to
957f48d
Compare
Here it is then: nrfconnect/sdk-nrf#12554 IMO, the CI should most definitely trigger sdk-nrf before having to create the manifest PR. It's unnecessary friction/noise for PR authors, reviewers and release engineers. |
How would CI figure your environment in case when you depend on changes in more than one repo, for example hal and sdk-zephyr? |
Could be solved with a label e.g. Point is, there are a bunch of backports like this one that only change sdk-zephyr, with no other deps, and the PR process could be streamlined. I've also seen times, usually at release, that there are multiple redundant manifest updates. |
…mations
If the peer is a zephyr host, there is no problem, as the Zephyr host limits sending parallel REQs and INDs.
But the spec allows sending those in parallel, and it may end up that the re-used REQ buffer hasn't been destroyed when an indication comes.
Only re-use the buffer when enqueuing ATT responses.
This means that we may run out of buffers if the peer sends too many indications and our application also sends a lot of commands/notifications.
The rationale for this is that having to handle a lot of requests is a more plausible scenario (e.g. being discovered by multiple peers) than handling lots of parallel indications.
Signed-off-by: Jonathan Rico [email protected]
(cherry picked from commit 7093538)