espressif / esp-nimble

A fork of NimBLE stack, for use with ESP32 and ESP-IDF
Apache License 2.0
76 stars 50 forks source link

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

Closed h2zero closed 4 years ago

h2zero commented 4 years ago

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.

https://github.com/h2zero/NimBLE-Arduino/issues/11

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

prasad-alatkar commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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.

prasad-alatkar commented 4 years ago

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

h2zero commented 4 years ago

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

h2zero commented 4 years ago

@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 commented 4 years ago

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.

h2zero commented 4 years ago

Awesome, thanks!

prasad-alatkar commented 4 years ago

Hi @h2zero Thank you for your contributions to esp-nimble. Commit: 095c14c9 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 commented 4 years ago

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