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

nimble: Eatt initial implementation #1549

Merged
merged 16 commits into from
Aug 24, 2023

Conversation

KKopyscinski
Copy link
Contributor

This is rebased #773

Before merging this needs to be checked against specification and tested

@KKopyscinski KKopyscinski force-pushed the eatt-initial branch 6 times, most recently from 7b12d91 to 7720d63 Compare July 17, 2023 10:39
@KKopyscinski KKopyscinski force-pushed the eatt-initial branch 2 times, most recently from 65d2699 to 879f76b Compare July 17, 2023 11:05
@KKopyscinski
Copy link
Contributor Author

TODO: it may be worth checking client_att_busy flag reset after disconnection. Because ble_gatt_conn_test_disconnect tests only GATT API for broken connection (connection instance still exists) it probably should be done as separate test, maybe in GAP.

@@ -734,6 +734,9 @@ ble_hs_init(void)
rc = ble_l2cap_init();
SYSINIT_PANIC_ASSERT(rc == 0);

rc = ble_gap_init();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to do something with line 752 I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, ble_gap_init should be outside #if NIMBLE_BLE_CONNECT - it includes code for features without connection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


#if MYNEWT_VAL(BLE_EATT_CHAN_NUM) > 0
if (cid != BLE_L2CAP_CID_ATT) {
return ble_eatt_tx(conn_handle, cid, txom);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part should be in following patch I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@KKopyscinski
Copy link
Contributor Author

KKopyscinski commented Jul 21, 2023

Found bug in Read Multiple Variable Length handling: implementation is OK, but btshell must be changed.
Code seems copied from other read/write callbacks, but here format is different. Response is single message with multiple tuples of {length, value} pairs, that is up to MTU in size. If more should be returned, it's trimmed: so we don't use BLE_HS_EDONE to mark final message, we will receive only one, with success code of 0. struct ble_gatt_attr *attr points to the first attreibute in list, and num_handles must be used to access the rest

edit: on the other hand, current implementation in shell expects we continue reading via Read Long procedure, and mark end of it with BLE_HS_EDONE. So implementing this behavior might be beneficial (if rsp buffer is full ocntinue reading until read < MTU -1)

@KKopyscinski
Copy link
Contributor Author

This was tested against PTS and implemented features are passing. After rebase some commits were added and some modified. Changes between rebase and current version can be seen here: https://gist.github.com/KKopyscinski/5ed12fe57bb457f21a8d98d2a84e0406
rebased version without these changes is here https://github.com/KKopyscinski/mynewt-nimble/tree/eatt_initial_rebase

TODO: Add support for sending Multiple Handle Variable Length Notification.

@KKopyscinski KKopyscinski changed the title nimble: [WIP] Eatt initial implementation nimble: Eatt initial implementation Aug 4, 2023
@KKopyscinski KKopyscinski marked this pull request as ready for review August 4, 2023 11:19
@KKopyscinski KKopyscinski requested a review from sjanc August 4, 2023 11:30
This is needed as eatt is registering listener in gap nowadays
This patch adds CID to the data path for all the GATT operations.
It also updates unit tests after the change.
The idea for EATT in Mynewt is as follow.
EATT is not exposed to the user i.e. GATT API stays the same. User can
define number of EATT channels supported by the host and it is host
managing those channels.
EATT will be fist choice channel for every ATT operation. In the future
we can make it smarted and make a choice based on the ATT data in the
packet

TODO:
* handle reading and setting client/server Supported features in the
stack instead of eatt code
* add test API to choose channel to use (needed by PTS)
rymanluk and others added 12 commits August 22, 2023 08:52
This is initial implementation of handling new GATT opcodes.
Gatt Client supports now:
ATT multi Variable read req
receiving ATT multi notifications

Gatt server supports now:
ATT multi Variable read rsp
In real case, requests are scheduled to be sent after previous one
completes. That happens after response is received. In unittests,
internal API is used for holding pending requests, from which they can
be manually dequeued. Because dequeue operation is always manual and
does not happed after response reception (when will never arrive, there
is no second device) we need to clear flag to be able to start another
procedure.
This is needed to properly handle failed procedure, and pass error to
the app.
This was missing and assert in ble_eatt_l2cap_event_fn was hit
GATT Client by writing to Client Supported Features characteristic
reports to server what features it supports. These features should be
saved for future use so appropriate subprocedures can be selected.
Channel was taken and function returns before release.
Assert was hit when BLE_ATT_OP_NOTIFY_MULTI_REQ was received.
…config

EATT supported bit is returned when EATT channel number is greater than 0
and Multiple Handle notification based on BLE_ATT_SVR_NOTIFY_MULTI.
For EATT bearer, ATT MTU is same for bot RX and TX and is minimum of the
two. L2CAP Credit Based Reconfigure Request can be used to change its
value.
Added check for link encryption. Removed unused flags for client/server
process. Before processing it is checked if PDU is an ATT one, and
channel is disconnected if not. If after processing PDU next one can't
be received link is discconnected.
@KKopyscinski KKopyscinski merged commit 10b3aa0 into apache:master Aug 24, 2023
7 of 8 checks passed
@rymanluk
Copy link
Contributor

Looks like it was merged accidentally but if all is working then it is fine. I was about to approve it anyways.

@KKopyscinski KKopyscinski deleted the eatt-initial branch February 13, 2024 07:55
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