Bluetooth: host: Add support for retrieving the remote version #17246

Blackladder · 2019-07-02T09:16:59Z

Make remote features and remote version accesible to the application
through the bt_conn_get_info object.
get_remote_version() is called if the remote version is not stored
in the new fields in struct bt_conn.

zephyrbot · 2019-07-02T09:28:12Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:341: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#341: FILE: subsys/bluetooth/host/conn.c:1955:
+		*  extended features has finished. */

-:341: WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each line
#341: FILE: subsys/bluetooth/host/conn.c:1955:
+		/* TODO: Make sure the HCI commands to read br features and
+		*  extended features has finished. */

-:678: WARNING:LONG_LINE: line over 80 characters
#678: FILE: subsys/bluetooth/shell/bt.c:258:
+			    "manufacturer 0x%04x", ver_str(remote_info->version),

- total: 0 errors, 3 warnings, 673 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

include/bluetooth/conn.h

subsys/bluetooth/host/hci_core.c

subsys/bluetooth/host/conn.c

include/bluetooth/conn.h

subsys/bluetooth/host/conn.c

Blackladder · 2019-07-03T07:25:23Z

Added the changes mentioned by @joerchan

subsys/bluetooth/host/conn_internal.h

Vudentz · 2019-07-18T08:50:01Z

If I recall correctly, I suggested having a dedicated API to trigger the remote info, i.e bt_conn_remote_info so we don't cause extra traffic with the likes of bt_conn_info.

Blackladder · 2019-07-18T10:46:27Z

With the latest commit, I have separated the retrieval of remote version into a new function bt_conn_remote_info like @Vudentz suggested and there no longer is a rv struct in bt_conn_info.

joerchan · 2019-12-12T20:02:04Z

subsys/bluetooth/host/hci_core.c

Remove and set

carlescufi

Looking much better. I think this solution is the right one, instead of bundling this into con info. Also the callback is much better and optimal than the blocking call.
A couple of loose ends.

carlescufi · 2019-12-13T14:27:01Z

subsys/bluetooth/host/hci_core.c

Should this be enclosed in #ifdef CONFIG_BT_REMOTE_VERSION ?

It should be static atleast.

I think using IS_ENABLED(CONFIG_BT_REMOTE) { hci_read_remote_version() } is enough to optimize it out. So it should not be included this way.
Checking with nm there is no hci_read_remote_version in the zephyr.elf file.

carlescufi · 2019-12-13T14:28:46Z

subsys/bluetooth/host/hci_core.c

~~Are you using BT_CONN_AUTO_VERSION_INFO before defining it or am I seeing the wrong commit order?~~
I see now, disregard this comment.

jhedberg · 2019-12-13T15:02:19Z

subsys/bluetooth/shell/bt.c

Add a comma to the end of this line, in case the struct gets extended in the future

Good point. 👍

carlescufi

Looks great now.
Also, this PR reduces in 105 bytes the ROM footprint of samples/bluetooth/peripheral in its default config.

jhedberg · 2019-12-13T15:08:32Z

subsys/bluetooth/host/hci_core.c

The handler should either be in normal_events[] or in prio_events[] but not both. I guess you're "deprioritizing" it along with this PR, so the above chunk should be removed.

Thanks. That is an oversight. Will remove.

jhedberg · 2019-12-13T15:09:35Z

subsys/bluetooth/host/hci_core.h

Isn't the move of this define unnecessary now?

Yes, I will move it back again.

joerchan

Revert prio changes not complete,

dbkinder

Looks like doc changes are all in the doxygen comments, LGTM.

Avoid floating point usage when possible. E.g. on qemu_x86 this would otherwise result in something like the following: <err> os: Floating point unit not enabled <err> os: eax: 0x00000024, ebx: 0x00000000, ecx: 0x00248fc8, edx: 0x00146120 <err> os: esi: 0x0012b30c, edi: 0x0024c628, ebp: 0x0024c638, esp: 0x0024c5b8 <err> os: eflags: 0x00000216 cs: 0x0008 cr3: 0x001465a0 <err> os: call trace: <err> os: eip: 0x00121c8d <err> os: 0x0010679c (0x127750) <err> os: 0x0010693b (0x100f33) <err> os: 0x001061bc (0x12b800) <err> os: 0x001074fb (0x0) <err> os: 0x00101151 (0x12b800) <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception <err> os: Current thread: 0x00146120 (shell_uart) <err> os: Halting system Signed-off-by: Johan Hedberg <[email protected]>

Fixed revert of prio changes

joerchan · 2019-12-19T16:56:19Z

@jhedberg @carlescufi Fixed reverting of the prio changes. I had some trouble with git giving me the wrong commit in blame which ended up silently bringing the changes back.

So please give this a thorough overview :) in case other cases of amending commits went wrong.

I have looked over now and it looks ok to me. But I don't think it is right that I approve my own changes so I won't approve this.

Stale, please re-review

jhedberg · 2019-12-19T20:17:07Z

subsys/bluetooth/host/hci_core.c

It seems that you haven't introduced the bt_hci_evt_read_remote_version_complete() yet in this patch - it only comes in a later one. Btw, a good way to check that each commit builds on its own is something like git rebase -i --exec="west build" origin/master. That will run the command given to the --exec switch for every commit that's part of the rebase.

Actually, you haven't introduced the CONFIG_BT_REMOTE_VERSION option either here. I know this is a rather pedantic approach but it helps later when doing something like git bisect since every commit that it might land on should be sane and buildable.

Thanks, good catch. I don't know exactly how it happened but some of my fixup commits ended up in the wrong commits.
The --exec="" was a good tip. Although in this case it wouldn't have caught it as it still compiled due to the CONFIG_BT_REMOTE_VERSION not having been defined.

Moved the change to the right commit.

This commit reverts the change that moved the remote version event from a priority event to a normal event. This is done because the strategy for using this event has been changed and will be used with a callback instead of a semaphore that could be locked from the RX thread. This commit retains the infrastructure that was added in the controller so that moving events to priority processing is still possible. Signed-off-by: Joakim Andersson <[email protected]>

Refactor the handling of the host auto initiated LL procedures. This makes it easier to add new auto initiated procedures as well as reduced the maintenance by reducing code duplication. Signed-off-by: Joakim Andersson <[email protected]>

Make remote features and remote version accesible to the application through the bt_conn_get_remote_info object. The host will auto initiate the procedures. If the procedures have not finished with the application calls bt_conn_get_remote_info then EBUSY will be returned. The procedures should finish during the first 10 connection intervals. Signed-off-by: Sverre Storvold <[email protected]> Signed-off-by: Joakim Andersson <[email protected]>

Add printing of the remote version information whenever the new CONFIG_BT_REMOTE_VERSION option is enabled. Signed-off-by: Johan Hedberg <[email protected]>

klgsnd · 2020-04-03T16:32:49Z

subsys/bluetooth/host/hci_core.c

+	    ((conn->role == BT_HCI_ROLE_MASTER) ||
+	     BT_FEAT_LE_SLAVE_FEATURE_XCHG(bt_dev.le.features))) {
+		err = hci_le_read_remote_features(conn);
+		if (!err) {


Hi guys, @jhedberg @Blackladder

I wonder whether the condition is correct. Shouldn't it be just if (err) ? It looks like it was borrowed from the commits this PR was based on.

I found this issue when tried to update connection parameters from the app level. They are simply not updated, because of the timer-specifics involved in the call of slave_update_conn_param() which is below in this function. And this slave_update_conn_param() is not called when conditions like I mentioned return from the conn_auto_initiate()..

if (!err) is the correct expression here. It returns on success of sending the command. If not success it moves to the next one. You can see it does that for all of the HCI procedures in this function. Once the procedure completes this function will be called and the next procedure should be initiated.

Thanks for the reply!
Sorry, I still don't get it. Shouldn't the happy-flow mean that all calls should succeed (given that their corresponding if-conditions are true - IS_ENABLED):

hci_le_read_remote_features()

hci_read_remote_version()

hci_le_set_phy()

hci_le_set_data_len()

slave_update_conn_param()

Why quit conn_auto_initiate() if PHY succeeds for example? Is there a relation between success on remote features and auto PHY negotiation, and DLE and Peripheral Conn Params Update Request?

My issue is that if slave_update_conn_param() is not called at all, then there is no way peripheral can initiate the update of connection parameters after the connection is established. Neither the initial auto update (slave_update_conn_param), nor from 'bt_le_conn_param_update()`. Isn't it a bug?

@dnsglk The intention is to call all of them. But the three first in the list are asynchronous, and only one can be active at a time.
If hci_le_read_remote_features is called, then conn_auto_initiate will be called again from bt_hci_evt_read_remote_version_complete, and this time it will call the next function in your list.
hci_le_set_data_len() and slave_update_conn_param() can be called simultaneously, so they don't have this behavior.

I see now. In my case then, gdb shows that le_phy_update_complete() is not called for whatever reason...

@joerchan thanks for spending time explaining it. Anyway, it still looks like something to improve IMO. Because, if remote doesn't respond to that query (which is my guess on why the phy callback is not triggered), it still means that not all of auto initialization has been completed.

@dnsglk Can you create an bug-report with the missing le_phy_update_complete and tell us how to reproduce this problem? Please be as detailed as possible

@joerchan I'll post a report if it I have more time to dig deeper into the issue. Thanks for understanding.

Blackladder requested review from Vudentz, jhedberg and joerchan as code owners July 2, 2019 09:17

zephyrbot added area: Bluetooth area: API Changes to public APIs labels Jul 2, 2019

Vudentz reviewed Jul 2, 2019

View reviewed changes

include/bluetooth/conn.h Outdated Show resolved Hide resolved

jhedberg requested a review from cvinayak July 2, 2019 10:41

Blackladder closed this Jul 2, 2019

Blackladder force-pushed the master branch from a0d5205 to efd99e6 Compare July 2, 2019 10:53

Blackladder reopened this Jul 2, 2019

Blackladder force-pushed the master branch 2 times, most recently from bd0aea3 to 79788e8 Compare July 2, 2019 11:50

joerchan mentioned this pull request Jul 2, 2019

Bluetooth: Add read remote version on startup and added to bt_conn_get_info #17244

Closed

joerchan suggested changes Jul 2, 2019

View reviewed changes

subsys/bluetooth/host/hci_core.c Outdated Show resolved Hide resolved

subsys/bluetooth/host/conn.c Outdated Show resolved Hide resolved

include/bluetooth/conn.h Outdated Show resolved Hide resolved

subsys/bluetooth/host/conn.c Outdated Show resolved Hide resolved

Blackladder force-pushed the master branch 2 times, most recently from f9b7beb to 73451b9 Compare July 3, 2019 07:23

Blackladder force-pushed the master branch from 73451b9 to 54bda9d Compare July 3, 2019 11:58

joerchan approved these changes Jul 18, 2019

View reviewed changes

joerchan requested a review from Vudentz July 18, 2019 08:42

jhedberg previously requested changes Jul 18, 2019

View reviewed changes

subsys/bluetooth/host/conn_internal.h Outdated Show resolved Hide resolved

Blackladder force-pushed the master branch 2 times, most recently from 3251ca3 to cec8a54 Compare July 18, 2019 10:43

Blackladder force-pushed the master branch 4 times, most recently from 467dba9 to 24d4df9 Compare July 18, 2019 12:14

joerchan reviewed Dec 12, 2019

View reviewed changes

subsys/bluetooth/host/hci_core.c Outdated

Copy link

Contributor

joerchan Dec 12, 2019

Choose a reason for hiding this comment

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

Remove and set

joerchan self-requested a review December 12, 2019 20:02

carlescufi requested changes Dec 13, 2019

View reviewed changes

carlescufi changed the title ~~Bluetooth: host: Add calling of read_remote_version~~ Bluetooth: host: Add support for retrieving the remote version Dec 13, 2019

joerchan force-pushed the master branch from 91faa23 to 52dec94 Compare December 13, 2019 14:54

Blackladder requested a review from dbkinder as a code owner December 13, 2019 14:54

joerchan force-pushed the master branch from 52dec94 to aafe74a Compare December 13, 2019 14:57

jhedberg reviewed Dec 13, 2019

View reviewed changes

carlescufi approved these changes Dec 13, 2019

View reviewed changes

jhedberg reviewed Dec 13, 2019

View reviewed changes

zephyrbot added the area: Documentation label Dec 13, 2019

joerchan force-pushed the master branch from aafe74a to bcc4e00 Compare December 13, 2019 16:22

joerchan previously requested changes Dec 13, 2019

View reviewed changes

dbkinder approved these changes Dec 16, 2019

View reviewed changes

joerchan force-pushed the master branch 2 times, most recently from bd847e8 to b316687 Compare December 19, 2019 16:50

jhedberg reviewed Dec 19, 2019

View reviewed changes

joerchan and others added 4 commits December 19, 2019 22:49

Bluetooth: shell: Add printing of remote version information

edde26b

Add printing of the remote version information whenever the new CONFIG_BT_REMOTE_VERSION option is enabled. Signed-off-by: Johan Hedberg <[email protected]>

joerchan force-pushed the master branch from b316687 to edde26b Compare December 19, 2019 21:50

jhedberg approved these changes Dec 23, 2019

View reviewed changes

jhedberg merged commit 4e135d7 into zephyrproject-rtos:master Dec 23, 2019

klgsnd reviewed Apr 3, 2020

View reviewed changes

Bluetooth: host: Add support for retrieving the remote version #17246

Bluetooth: host: Add support for retrieving the remote version #17246

Uh oh!

Conversation

Blackladder commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

checkpatch (informational only, not a failure)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Blackladder commented Jul 3, 2019

Uh oh!

Uh oh!

Vudentz commented Jul 18, 2019

Uh oh!

Blackladder commented Jul 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joerchan left a comment

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

joerchan commented Dec 19, 2019

Uh oh!

jhedberg Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klgsnd Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joerchan Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klgsnd Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Blackladder commented Jul 2, 2019 •

edited

Loading

zephyrbot commented Jul 2, 2019 •

edited

Loading

carlescufi Dec 13, 2019 •

edited

Loading

jhedberg Dec 19, 2019 •

edited

Loading

klgsnd Apr 3, 2020 •

edited

Loading

joerchan Apr 3, 2020 •

edited

Loading

klgsnd Apr 3, 2020 •

edited

Loading