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

Allow host to resolve peer RPA without using local RPA. #7

Closed

Conversation

h2zero
Copy link

@h2zero h2zero commented Apr 16, 2020

This solves an issue of not being able to resolve a bonded peer's address without the host RPA being enabled. I.E when a phone bonds with a peripheral with a static address it will not resolve and correctly identify the phone peer as bonded upon reconnection. This PR solves that issue.

h2zero/NimBLE-Arduino#11

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2020

CLA assistant check
All committers have signed the CLA.

h2zero pushed a commit to h2zero/esp-nimble that referenced this pull request Apr 17, 2020
It is possible that scheduler already fired and aux data is NULL so
we should unref it only if scheduler item was removed.

ble_ll_scan_aux_data_unref (aux_data=<optimized out>) at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll_scan.c:1079
1079        BLE_LL_ASSERT(aux_data);
(gdb) bt
 #0  ble_ll_scan_aux_data_unref (aux_data=<optimized out>) at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll_scan.c:1079
 espressif#1  0x0001f732 in ble_ll_scan_rx_pkt_in_on_aux (pdu_type=<optimized out>, om=0x2000d6f8 <os_msys_1_data>, hdr=0x2000d710 <os_msys_1_data+24>, addrd=addrd@entry=0x20004ecc <g_ble_ll_stack+404>)
    at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll_scan.c:3156
 espressif#2  0x0002010e in ble_ll_scan_rx_pkt_in (ptype=ptype@entry=7 '\a', om=om@entry=0x2000d6f8 <os_msys_1_data>, hdr=hdr@entry=0x2000d710 <os_msys_1_data+24>)
    at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll_scan.c:3198
 espressif#3  0x0001299e in ble_ll_rx_pkt_in () at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll.c:837
 espressif#4  0x000129de in ble_ll_event_rx_pkt (ev=<optimized out>) at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll.c:1164
 espressif#5  0x00012830 in ble_npl_event_run (ev=<optimized out>) at repos/apache-mynewt-nimble/porting/npl/mynewt/include/nimble/nimble_npl_os.h:116
 espressif#6  ble_ll_task (arg=<optimized out>) at repos/apache-mynewt-nimble/nimble/controller/src/ble_ll.c:1214
 espressif#7  0x00038e9c in nrf52_clock_hfxo_release () at repos/apache-mynewt-core/hw/mcu/nordic/nrf52xxx/include/mcu/cortex_m4.h:37
 espressif#8  0x00000000 in ?? ()
@prasad-alatkar
Copy link

Hi @h2zero , ble_host_rpa_enabled() call/check is necessary to resolve or add peer device in resolving list. Though I get your point that local device doesn't need to advertise/scan using RPA to resolve peer's RPA. This case can be addressed with setting own_addr_type = BLE_ADDR_TYPE_PUBLIC and still configuring privacy with ble_hs_pvcy_rpa_config.

@h2zero
Copy link
Author

h2zero commented Apr 20, 2020

Hi @prasad-alatkar, thanks, I'll give it a try that way and get back to you. I should note that this patch still adds the peer to the resolving list, unless I missed something?

@h2zero
Copy link
Author

h2zero commented Apr 21, 2020

@prasad-alatkar I have tested as you suggested with own_addr_type = BLE_OWN_ADDR_PUBLIC; and ble_hs_pvcy_rpa_config(1);. I was quickly reminded why this patch was necessary. Using nRF connect on iPhone I would not be able to pair/bond, the error message I would see on the phone is encryption is insufficent. If I changed to BLE_OWN_ADDR_RANDOM it works fine. I can provide many more details and logs but that is probably better done in the issue thread #8 if you would like.

@prasad-alatkar
Copy link

@h2zero Yes, just own_addr_type = BLE_OWN_ADDR_PUBLIC; won't help, realized after posting my comment. You can basically get the above setup working with commenting out this line . However it will give error for own_addr_type = BLE_OWN_ADDR_RANDOM;. I am kind of not comfortable with the approach you have suggested, reason being it will add extra overhead for cases other than RPA. Maybe we can handle this situation with calling ble_hs_is_rpa to classify peer_addr as RPA. Another approach I was thinking of was through ble_hs_pvcy_rpa_config itself i.e. input parameter 0 to disable; 1 to enable local RPA & resolving peer RPA; 2 for local public address & resolving peer RPA. Let me know your thoughts on this.

@h2zero
Copy link
Author

h2zero commented Apr 21, 2020

@prasad-alatkar If I understand you correctly, you want to avoid using cycles looking up peers in the resolve list etc if RPA is not enabled by the app, I agree. However I would, as a user, suggest that we should default to using the RPA; 2 parameter in your post as that seems to be the expected outcome from users and I believe is the default behavior of the bluedroid library (not sure?).

If that is not desired then this function should be very well documented or there may be many issues posted with answers that look like: "Change line x to ble_hs_pvcy_rpa_config(2);" 😃

@h2zero
Copy link
Author

h2zero commented Apr 25, 2020

@prasad-alatkar I'm going to put some work into this over the weekend, hopefully the ble_hs_is_rpa approach works well as it would be the easiest to implement.

@prasad-alatkar
Copy link

@h2zero sorry for not responding earlier, caught up with some work. Before working on ble_hs_is_rpa, I would suggest try to finalize/refine your current approach in this PR. I will work to allocate some good amount of time in coming week around this PR/feature enhancement :)

@h2zero
Copy link
Author

h2zero commented Apr 26, 2020

@prasad-alatkar No worries 😃, seeing some good initial results already and identified a couple spots for further refinement. I'm enjoying the learning process, I didn't even know ble_hs_is_rpa existed before, but seems quite useful in this application. I'm sure we can come up with a satisfactory solution 🥂.

@h2zero
Copy link
Author

h2zero commented Apr 27, 2020

@prasad-alatkar Updated code and refined a bit, works well but I found a new bug that is related to but not created by this PR. I'll post an issue for it.

@h2zero h2zero force-pushed the resolve-peer-rpa-no-local-rpa branch from a554101 to b6fda46 Compare April 27, 2020 04:33
nimble/host/src/ble_hs_hci_evt.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_hci_evt.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_conn.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_conn.c Show resolved Hide resolved
nimble/host/src/ble_hs_conn.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_conn.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_conn.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_hci_evt.c Outdated Show resolved Hide resolved
nimble/host/src/ble_hs_hci_evt.c Outdated Show resolved Hide resolved
@prasad-alatkar
Copy link

@h2zero I have left a few comments. Apart from that looks good !!

@h2zero
Copy link
Author

h2zero commented Apr 29, 2020

@prasad-alatkar, I see now what you're looking for, I'll go through it when I get a chance.

@h2zero h2zero force-pushed the resolve-peer-rpa-no-local-rpa branch from b6fda46 to 42fef9c Compare May 2, 2020 03:47
@h2zero
Copy link
Author

h2zero commented May 2, 2020

@prasad-alatkar Sorry for the confusion, I guess I wasn't sure of your direction but I think I got it now😃. Updated with a commit to resolve #10 as requested.

@prasad-alatkar
Copy link

Thank you @h2zero, looks good to me !! @dhrishi Can you please take a quick look ? It adds support when peer is RPA but local device's address is not RPA.

Copy link
Collaborator

@dhrishi dhrishi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Have reviewed and approved. Thanks @h2zero!

@h2zero
Copy link
Author

h2zero commented May 7, 2020

Awesome, thanks!

dhrishi pushed a commit that referenced this pull request May 20, 2020
dhrishi pushed a commit that referenced this pull request May 20, 2020
dhrishi pushed a commit that referenced this pull request May 20, 2020
dhrishi pushed a commit that referenced this pull request May 20, 2020
dhrishi pushed a commit that referenced this pull request May 28, 2020
dhrishi pushed a commit that referenced this pull request May 28, 2020
@prasad-alatkar
Copy link

Hi @h2zero Thank you for your contributions to esp-nimble. Commit: 095c14c reflects your changes. I incorporated your changes to my existing MR by cherry-picking, that caused the commit SHA change, and that may be the reason for not having Merged tag for this MR. Sorry for that !!

@h2zero
Copy link
Author

h2zero commented May 29, 2020

Thanks @prasad-alatkar, no problem, I'm just glad to see it got included 😄

mahavirj pushed a commit to mahavirj/esp-afr-sdk that referenced this pull request Jun 1, 2020
… (backport v3.3)

Change list:
- Reduces the size of the compiled binary, PR: espressif/esp-nimble#6
- Null pointer check, PR: apache/mynewt-nimble#701
- Pairing procedure abort on unexpected req: apache/mynewt-nimble#710
- Fix conn flags after pairing: apache/mynewt-nimble#730
- Remove notification for update process timeout (Vol 6, Part B, section 5.2 ):
  apache/mynewt-nimble#782
- CCCD fix : apache/mynewt-nimble#790 and
  apache/mynewt-nimble#804
- Host based Privacy (RPA) fix: espressif/esp-nimble#7

 Closes espressif/esp-nimble#10

 Closes espressif/esp-idf#4413
espressif-bot pushed a commit to espressif/esp-idf that referenced this pull request Jun 15, 2020
… (backport v4.0)

Change list:
- Reduces the size of the compiled binary, PR: espressif/esp-nimble#6
- Null pointer check, PR: apache/mynewt-nimble#701
- Pairing procedure abort on unexpected req: apache/mynewt-nimble#710
- Fix conn flags after pairing: apache/mynewt-nimble#730
- Remove notification for update process timeout (Vol 6, Part B, section 5.2 ):
  apache/mynewt-nimble#782
- CCCD fix : apache/mynewt-nimble#790 and
  apache/mynewt-nimble#804
- Host based Privacy (RPA) fix: espressif/esp-nimble#7

 Closes espressif/esp-nimble#10

 Closes #4413
projectgus pushed a commit to espressif/esp-idf that referenced this pull request Jul 22, 2020
… (backport v4.1)

Change list:
- Reduces the size of the compiled binary, PR: espressif/esp-nimble#6
- Null pointer check, PR: apache/mynewt-nimble#701
- Pairing procedure abort on unexpected req: apache/mynewt-nimble#710
- Fix conn flags after pairing: apache/mynewt-nimble#730
- Remove notification for update process timeout (Vol 6, Part B, section 5.2 ):
  apache/mynewt-nimble#782
- CCCD fix : apache/mynewt-nimble#790 and
  apache/mynewt-nimble#804
- Host based Privacy (RPA) fix: espressif/esp-nimble#7

 Closes espressif/esp-nimble#10

 Closes #4413
@h2zero h2zero deleted the resolve-peer-rpa-no-local-rpa branch March 6, 2021 00:47
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.

4 participants