Added support for passkey-entry and made some usability improvements #632
Conversation
…and a few helpers
|
thanks for submitting PR, I am currently in middle of other works. I will review this when switching back to nrf52. Please be patient in the meanwhile |
|
No problem! Thanks for the update. |
|
Hi @hathach, are there any updates as to when this will be able to be reviewed? |
There was a problem hiding this comment.
Thank for the PR and your patient, following are my review
- the passkey request should require user to fill the passkey instead of having it to call another API
- changes to mouse buttons are not really necessary in my opinion, application should scan the button and send it. Therefore please revert those changes.
- exposing bond list is useful, though I would like to put more thoughts on the API. Since the list is maintained by the bluefruit lib, I could only think of 2 usage, one is for display (mac, name retrieval), and another is removing specific device. Is there any usage that you would add. For my opinion, it is much better to be on its own PR.
| bool mouseButtonPressSpecific(uint8_t buttons); | ||
| bool mouseButtonReleaseSpecific(uint8_t buttons); |
There was a problem hiding this comment.
It is simple, but I am not quite sure if it would be useful. Mouse application should really keep an copy of its own active buttons or scaned its gpio for active buttons to use with. So I think it is better to revert this change.
There was a problem hiding this comment.
Sounds fair. Reverted.
| @@ -50,6 +51,7 @@ class BLEPeriph | |||
| bool connected(uint16_t conn_hdl); // Connected as prph to this connection | |||
| uint8_t connected(void); // Number of connected as peripherals | |||
|
|
|||
| bool getBonds(bonded_device_info* bondedPeers, uint32_t bondedPeersSize, uint32_t* actualBondedPeersSize); | |||
There was a problem hiding this comment.
yeah, I would also want to expose the bond list, however I would prefer to retrieve one by one as an array than returning the whole list which can be long and application probably will process one by one anyway afterwards. Or I am missing something, could you tell me about your usage.
There was a problem hiding this comment.
I took your suggestion and removed this portion of this PR. I'll consider the addition a bit more and if it still makes sense to add I will create a separate PR later to focus on that change by itself.
There was a problem hiding this comment.
Yeah, it make sense to retrieve data from bond list, there is bond management service which can benefit from this as well, I planed to do this but haven’t got time to do so.
the separate PR makes sense. It is best to focus on each feature per PR to make it easier to back-tracking when fixing bugs. We could piggy-pack multiple features if they are related though.
|
Hi @hathach. Apologies for the long hiatus. I just updated the PR, taking your feedback and suggestions. I left the comments open so you can resolve the conversations as you ensure the right modifications were made with the new change. Also, do let me know if you find it necessary to rebase on top of |
There was a problem hiding this comment.
Great PR, thank you and sorry for late response. There is no need to apology, I have my own issue with time management as well. The PR is much more focused on the passkey-entry, though I think we should change the name and return type of getConnectedHandles() per review. Let me know what you think, I am open to all suggestions.
There was a problem hiding this comment.
@ArturA-MS since we are both busy and the request is fairly simple. I have made the changes myself and push the update to your fork so that we could finally merge this. Thank you very much for your PR and patient.
|
@hathach thank you! I greatly appreciate your help. |
|
Thank you for the PR :) |
Our use cases required some additional functionality to exercise passkey-entry pairing and query current connections.
These are meant to be some spot-changes that don't modify any existing behavior but can be used to unblock certain uses. Hopefully they can be useful to others as well.
Quick summary:
Passkey Entrypairing by handlingBLE_GAP_EVT_AUTH_KEY_REQUESTinBLESecurityclass.AdafruitBluefruitto get all currently connected connection handles.