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

[nrf fromtree] Bluetooth: att: don't re-use the ATT buffer for confir… #1335

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

jori-nordic
Copy link
Contributor

…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 jonathan.rico@nordicsemi.no
(cherry picked from commit 7093538)

@jori-nordic jori-nordic added this to the ncs-2.5.0 milestone Oct 4, 2023
@jori-nordic jori-nordic added the bug Something isn't working label Oct 4, 2023
@de-nordic
Copy link
Contributor

@jori-nordic Some sdk-nrf update maybe?

@jori-nordic
Copy link
Contributor Author

Some sdk-nrf update maybe

I thought that was a solved problem, that it triggered the right CI automatically, isn't that the case?

@de-nordic
Copy link
Contributor

sdk-zephyr is part of sdk-nrf setup, and this will not get merged unless pointed by the sdk-nrf west manifest.
Currently it has been only tested in context of sdk-zephyr itself so it will not get merged because its effects on sdk-nrf is unknown.

…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 <jonathan.rico@nordicsemi.no>
(cherry picked from commit 7093538)
@jori-nordic
Copy link
Contributor Author

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.

@de-nordic
Copy link
Contributor

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?

@jori-nordic
Copy link
Contributor Author

Could be solved with a label e.g. test-sdk-nrf or something like that. Then you manually add it. Going further, the nordic bot could even open the PR.

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.
The bot could also detect this, so we'd have a sort of merge-queue instead of having to do busy-work keeping up with which manifest PR is merged or going to be merged and rebasing, and retriggering CI wasting nodes etc..

@rlubos rlubos merged commit 168b413 into nrfconnect:main Oct 6, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants