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

Offset field in struct ble_gatt_access_ctxt as an indication of long attribute read #1846

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

vandy
Copy link
Contributor

@vandy vandy commented Aug 10, 2024

To obtain an attribute value when the attribute size is greater than configured MTU long attribute read procedure could be used.
A client sends consecutive ATT_READ_BLOB_REQ requests (passing along an offset) to consume the attribute value. The offset is used internally in the nimble stack but is not passed to a user provided access callback in a GATT characteristic description. This callback is called the same number of times as the number of blob requests, but it's not possible to say whether it's a first request (or maybe just ATT_READ_REQ) or consecutive request.

The provided offset could indicate this situation to the app developer. However, Core Spec 5.4 Vol 3 Part F 3.4.4.5 notes:

Note: The value of a Long Attribute may change between the server receiving one ATT_READ_BLOB_REQ PDU and the next ATT_READ_BLOB_REQ PDU. A higher layer specification should be aware of this and define appropriate behavior.

It's up to a developer to decide what to do, when consecutive (long attribute) reading takes place. This was discussed in #1090.

@github-actions github-actions bot added host size/M Medium PR labels Aug 10, 2024
@vandy
Copy link
Contributor Author

vandy commented Aug 10, 2024

I've also made some changes to other parts of the code (only code style without change in functionality). <- Moved to other PRs.

And provided some points to consider with REVIEW comments. I'd love to get a response on this topics and if they are approved I'll make changes to the code as described in this comments.

porting/nimble/src/os_mbuf.c Outdated Show resolved Hide resolved
@vandy
Copy link
Contributor Author

vandy commented Aug 10, 2024

Right now the nimble stack takes all attribute data from an application and truncates it to be the size of MTU (actually MTU - 1). E.g. attribute length is 500 bytes, MTU is 23 bytes. So the application copies 500 bytes to the provided mbuf, and then nimble stack copies only 22 bytes according to an offset.

I think it would be nice to allow the application callback to provide only part of the data (as it already knows the offset and could obtain mtu length information). What do you think? @sjanc

@andrzej-kaczmarek
Copy link
Contributor

I've also made some changes to other parts of the code (only code style without change in functionality).

code style fixes should be at least in separate commit, but preferably in separate PR

@vandy
Copy link
Contributor Author

vandy commented Aug 11, 2024

@andrzej-kaczmarek, I've split this PR into 3: #1847 and #1848. But left REVIEW comments here until the discussion and decision is settled. I believe it would be better to make another PR for discussed changes.

@andrzej-kaczmarek
Copy link
Contributor

you can make separate PR for other changes so we can discuss them.
just keep adding the offset field to the struct in this PR and it's good to go. please don't reformat other pieces of code though...

@vandy
Copy link
Contributor Author

vandy commented Aug 12, 2024

Done, left only changes related to the offset.

@vandy vandy changed the title Offset field in struct ble_gatt_access_ctxt as an indication of long attribute read [WIP] Offset field in struct ble_gatt_access_ctxt as an indication of long attribute read Aug 12, 2024
@sjanc
Copy link
Contributor

sjanc commented Aug 12, 2024

Hi,

Code looks OK to me, thanks! But could you amend commit message to follow project style?
This should be summary line (with area prefix, eg "nimble/host:" ) and short description in commit body.
Also note preferred 72 characters line limit

eg

nimble/host: Add offset in GATT server long read context

This allows application to easily determine if client is performing
GATT Long Read operation.

@vandy
Copy link
Contributor Author

vandy commented Aug 12, 2024

Amended the commit message. Also, moved issues to discuss into #1849.

@vandy
Copy link
Contributor Author

vandy commented Aug 19, 2024

@sjanc, would you merge the PR?

nimble/host/src/ble_gatts.c Outdated Show resolved Hide resolved
This allows application to easily determine if client is performing
GATT Long Read operation.
@vandy
Copy link
Contributor Author

vandy commented Oct 19, 2024

@andrzej-kaczmarek, can we merge now?

@andrzej-kaczmarek andrzej-kaczmarek merged commit d3bf5ec into apache:master Oct 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host size/M Medium PR size/S Small PR size/XS Extra small PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants