BLE advertising simplification #1122

benedekkupper · 2025-02-05T13:14:55Z

left uses directed advertising, right advertises HID only

device/src/bt_advertise.c

mondalaci · 2025-02-06T18:21:32Z

@kareltucek I've just tested this PR, and it removes the extra bottom entry that was exposed by the left half:

Can I merge this PR, or is there anything else that I should test?

kareltucek · 2025-02-07T09:37:38Z

@kareltucek I've just tested this PR, and it removes the extra bottom entry that was exposed by the left half:

Please wait till Benedek says that everything is ready.

You should test the case of switching to a device via "switchHost name" when the host is not connected (e.g., dongle is violet) and all peripheral connection slots are taken (e.g., there are already three blue/green dongles) - feel free to reduce the peripheral connection count for testing, and independently for production too (it has its cons as well as pros).

mondalaci · 2025-02-08T22:43:22Z

When testing this branch, the UHK 80 BLE device no longer appears on my phone in the available devices list.

mondalaci · 2025-02-09T20:39:27Z

Now, I can pair my UHK 80 to my Android phone, and the connection is established. However, the USB host connection remains, and instead of automatically switching, I have to explicitly switch to the BLE host connection.

@kareltucek Assuming it's due to the firmware logic, can you look into this?

kareltucek · 2025-02-11T15:53:14Z

@mondalaci I can't reproduce this.

Does this happen wih a "new bluetooth device", or a host-connections-list-present device? In the latter case, is your switchover checked?

mondalaci · 2025-02-11T16:04:40Z

I could reproduce the issue again, then switched to master, and the issue was still there, so this branch is probably not the culprit.

This happens (or I'd rather say not happens, that is, the automatic switchover) directly after pairing a "new bluetooth device"). I didn't save the host connections.

Does this log help?

Starting advertising, try 0!
NUS and HID advertising continued
Received passkey pairing inquiry.
Type `uhk passkey xxxxxx` to pair, or `uhk passkey -1` to reject
Sending passkey to conn 0x2000bce0
Bt connection secured: unknown (ac:d6:18:1b:3d:b5), level 4. It is auth_conn, so ignoring.
Pairing completed: unknown (ac:d6:18:1b:3d:b5), bonded 1
Con state: Host2(My Phone, Connected), Peer host1
Bt connected to host1 (ac:d6:18:1b:3d:b5)
Established HID connection with host1 (ac:d6:18:1b:3d:b5)
Con state: Host2(My Phone, Ready), Peer host1
Starting advertising, try 0!
NUS advertising continued
host1 (ac:d6:18:1b:3d:b5) conn params: interval=7 ms, latency=0, timeout=5000 ms
host1 (ac:d6:18:1b:3d:b5) conn params: interval=11 ms, latency=0, timeout=1000 ms
I: starting HID app 1: 1

kareltucek · 2025-02-11T16:17:13Z

Well, the log looks like UHK knows your "My Phone" already. In that case it behaves according to the switchover checkbox.

mondalaci · 2025-02-11T16:32:54Z

You're right, my oversight! This PR looks good to me, and I'd merge it. Any objections?

kareltucek · 2025-02-11T16:34:50Z

Any objections?

Yes.

kareltucek · 2025-02-11T16:59:14Z

device/src/bt_advertise.c

-    return connectedHids;
-}
-
-uint8_t BtAdvertise_Type() {


This doesn't look like something that should have been removed.

You are always trying to advertise, leading to errors when the peripheral connection count is reached. My code in turn tries to remedy the situation by increasingle aggressive strategies, including disconnecting current connections. The code was written so that uhk works even with CONFIG_BT_CTLR_SDC_PERIPHERAL_COUNT=1. With this settings and this code uhk will not work at all over bluetooth (dongle or hid).

Switchover scenario might need to be taken care of when the limit is reached.

How exactly does the current "simplified" advertising work on right half with respect to dongles? It broadcasts itself as a HID device, and dongles see this and try to connect despite there not being any advertisement for NUS?

I assume I should restore the necessary functionality? Will look into it tomorrow...

Only the content of the advertising is changed, the call sites of these methods control when advertising is started or stopped, I didn't change the logic there.

Although I didn't test the changes with a dongle, I did check the scanning logic, and there I saw no filtering based on service UUIDs, only an address based filtering, that's why I was brave enough to remove it. My original idea of directed advertising when intending to connect to a dongle didn't work - according to mondalaci's feedback - so you are very welcome to take a look at this yourself, or to revert the changes for the right half and continue using HID / NUS advertising content.

Only the content of the advertising is changed, the call sites of these methods control when advertising is started or stopped, I didn't change the logic there.

The trouble is the BtAdvertise_Type() also decides whether or not to advertise at all. Anyways no worries, I will fix that shortly.

I did check the scanning logic, and there I saw no filtering based on service UUIDs, only an address based filtering, that's why I was brave enough to remove it.

I see. I like the change, but this really needs to be mentioned in a comment somewhere ;-). (Will add that.)

Actually, the compatibility is single directional.

When one ble HID is already connected we want to advertise NUS only - as we can't handle multiple ble HIDs at the same time. Unless you or @mondalaci have a better idea how to handle the situation that is.

@kareltucek If it's possible to reuse the single available BLE HID connection upon connecting a new BLE HID device, ongoing BLE HID advertising would be preferred.

kareltucek

Argh, sorry, wanted to "add a single comment".

kareltucek · 2025-02-12T11:29:15Z

device/src/bt_advertise.c

+        err = bt_le_adv_start(&adv_param, adHid, ARRAY_SIZE(adHid), sdHid, ARRAY_SIZE(sdHid));
+
+    } else if (DEVICE_IS_UHK80_LEFT) {
+        adv_param = *BT_LE_ADV_CONN_DIR_LOW_DUTY(&HostConnection(ConnectionId_NusClientRight)->bleAddress);


Btw., here should be &Peers[PeerIdRight].addr - this couldn't have worked (@mondalaci i.e., I believe the left half does never work over nus with this branch).

What's worse is that the code doesn't work when given correct addresses (they start to scan and advertise, but never actually connect). I take the above as an evidence that it wasn't ever tested.

-> I will restore it to the original NUS advertisement, but leave the subsystem in place with a comment. Feel free to try to fix it @benedekkupper .

Sorry for all the mess I made. I guess the missing line is advParam.options |= BT_LE_ADV_OPT_DIR_ADDR_RPA; before calling the bt_le_adv_start

Unfortunately still doesn't work. Left advertises, right scans, but they never connect.

Also tried this, based on comments in the code:

advParam = *BT_LE_ADV_CONN_ONE_TIME; advParam.peer = advConfig.addr;

This results in error 60 (timeout).

(Pushing a minor refactor to make the code a bit more bearable.)

kareltucek · 2025-02-12T11:47:24Z

~~@benedekkupper given your prj.conf refactor, where should following snippet be placed? (It is relevant for right half only. They are now in nrf_shared.conf)~~

Nevermind, I have added new right.conf

# Todo: place these where they belong
# config for right half host switching
CONFIG_BT_MAX_CONN=4
CONFIG_BT_CTLR_SDC_PERIPHERAL_COUNT=3

@mondalaci when testing this branch, please note that macro switchHost "name" scenarios should be (also) tested with CONFIG_BT_CTLR_SDC_PERIPHERAL_COUNT set to 1.

kareltucek · 2025-02-12T11:53:33Z

device/src/bt_advertise.c

+#define ADVERTISEMENT(TYPE) ((adv_config_t) { .advType = TYPE })
+#define ADVERTISEMENT_DIRECT_NUS(ADDR) ((adv_config_t) { .advType = ADVERTISE_DIRECTED_NUS, .addr = ADDR })
+
+adv_config_t BtAdvertise_Config() {


(An additional reason to leave this in place is that it documents the scenarios that can happen, and makes logs more explicit - nevermind what advertisement params the backend actually ends up using.)

kareltucek · 2025-02-12T12:09:31Z

Btw, 363328d broke dongle leds.

051e7ea fixes it. I assume it has to do with weak linking and its order, and I guess implies that it is not as safe and reliable as I though.

(The issue is that once we switch to nus only advertisement, the host still renames the hid connection based on it.)

benedekkupper · 2025-02-19T12:50:23Z

~~@benedekkupper given your prj.conf refactor, where should following snippet be placed? (It is relevant for right half only. They are now in nrf_shared.conf)~~

Nevermind, I have added new right.conf
# Todo: place these where they belong
# config for right half host switching
CONFIG_BT_MAX_CONN=4
CONFIG_BT_CTLR_SDC_PERIPHERAL_COUNT=3

You could put this to ble_hid.conf, since only the right side is a BLE HID device.

benedekkupper added 2 commits February 5, 2025 14:13

bugfix for cmake versions.c generation

363328d

shell: fix missing commands

a0dfb6d

cla-sign-bot bot added the cla:signed label Feb 5, 2025

benedekkupper commented Feb 5, 2025

View reviewed changes

device/src/bt_advertise.c Outdated Show resolved Hide resolved

benedekkupper force-pushed the ble-advertising-fix branch from 12452bf to e7eabdd Compare February 7, 2025 09:59

bt: left uses directed advertising, right advertises HID only

5800ef3

benedekkupper force-pushed the ble-advertising-fix branch from e7eabdd to 5800ef3 Compare February 9, 2025 19:51

kareltucek reviewed Feb 11, 2025

View reviewed changes

kareltucek reviewed Feb 12, 2025

View reviewed changes

kareltucek added 2 commits February 12, 2025 12:42

Restore the ble advertising type computation.

ee1bca1

Use normal advertising as directed doesn't work.

4e4e265

Aesthetic refactors.

1e05cf3

kareltucek reviewed Feb 12, 2025

View reviewed changes

Fix dongle leds.

051e7ea

kareltucek added 5 commits February 12, 2025 14:43

Fix left side peripheral count.

90cd19e

Name advertisements better and fix the left advertisement conditions.

4524f08

Refactor advertising code...

3a20b69

Rename the advertisements to be less misleading.

454c606

(The issue is that once we switch to nus only advertisement, the host still renames the hid connection based on it.)

Merge remote-tracking branch 'origin/master' into ble-advertising-fix

7d17948

mondalaci merged commit 7523714 into master Feb 19, 2025
2 checks passed

benedekkupper mentioned this pull request Feb 21, 2025

Make NUS use directed advertising #1141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLE advertising simplification #1122

BLE advertising simplification #1122

benedekkupper commented Feb 5, 2025

mondalaci commented Feb 6, 2025

kareltucek commented Feb 7, 2025 •

edited

Loading

mondalaci commented Feb 8, 2025

mondalaci commented Feb 9, 2025

kareltucek commented Feb 11, 2025

mondalaci commented Feb 11, 2025

kareltucek commented Feb 11, 2025

mondalaci commented Feb 11, 2025

kareltucek commented Feb 11, 2025

kareltucek Feb 11, 2025

benedekkupper Feb 11, 2025

kareltucek Feb 12, 2025

kareltucek Feb 12, 2025

mondalaci Feb 12, 2025

kareltucek left a comment

kareltucek Feb 12, 2025 •

edited

Loading

benedekkupper Feb 12, 2025

kareltucek Feb 13, 2025

kareltucek commented Feb 12, 2025 •

edited

Loading

kareltucek Feb 12, 2025

kareltucek commented Feb 12, 2025

benedekkupper commented Feb 19, 2025

BLE advertising simplification #1122

BLE advertising simplification #1122

Conversation

benedekkupper commented Feb 5, 2025

mondalaci commented Feb 6, 2025

kareltucek commented Feb 7, 2025 • edited Loading

mondalaci commented Feb 8, 2025

mondalaci commented Feb 9, 2025

kareltucek commented Feb 11, 2025

mondalaci commented Feb 11, 2025

kareltucek commented Feb 11, 2025

mondalaci commented Feb 11, 2025

kareltucek commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kareltucek left a comment

Choose a reason for hiding this comment

kareltucek Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kareltucek commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

kareltucek commented Feb 12, 2025

benedekkupper commented Feb 19, 2025

kareltucek commented Feb 7, 2025 •

edited

Loading

kareltucek Feb 12, 2025 •

edited

Loading

kareltucek commented Feb 12, 2025 •

edited

Loading