[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-16 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r409499230
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -233,13 +233,15 @@ ble_store_util_status_rr(struct ble_store_status_event 
*event, void *arg)
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+/* Try unpairing oldest peer except current peer */
+return ble_gap_unpair_oldest_except((void *) 
>overflow.value->cccd.peer_addr);
 
 Review comment:
   nitpick: (void *) seems to be not needed here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-14 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r407949256
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +372,24 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+/* Try to remove unbonded CCCDs first */
+if ((rc = ble_store_clean_old_cccds((void *) 
>overflow.value->cccd.peer_addr)) == BLE_HS_ENOENT) {
 
 Review comment:
   @h2zero actually CCCD's do get stored when devices are bonded.  That happens 
after pairing is completed and both devices have set bonding flag.
   
   If you do see an issue that CCCDs are stored but device is not actually 
bonded, please report it as an issue.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-14 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r407949256
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +372,24 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+/* Try to remove unbonded CCCDs first */
+if ((rc = ble_store_clean_old_cccds((void *) 
>overflow.value->cccd.peer_addr)) == BLE_HS_ENOENT) {
 
 Review comment:
   @h2zero actually CCCD's do get stored when devices are bonded.  That happens 
after pairing is completed and both devices have bonding flag in set. 
   
   If you do see an issue that CCCDs are stored but device is not actually 
bonded, please report it as an issue.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-09 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r406024875
 
 

 ##
 File path: nimble/host/src/ble_gap.c
 ##
 @@ -5605,6 +5605,41 @@ ble_gap_unpair_oldest_peer(void)
 return 0;
 }
 
+int
+ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer)
+{
+ble_addr_t oldest_peer_id_addr[MYNEWT_VAL(BLE_STORE_MAX_BONDS)];
 
 Review comment:
   nitpick: please rename to `peer_id_addrs`


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-09 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r406032755
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +372,24 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+/* Try to remove unbonded CCCDs first */
+if ((rc = ble_store_clean_old_cccds((void *) 
>overflow.value->cccd.peer_addr)) == BLE_HS_ENOENT) {
 
 Review comment:
   I don't think we need to clean unbodend CCCDs as we do not keep such in the 
storage. Unless I miss something, but CCCDs are stored only for bonded devices, 
and once unbonded, we do clean CCCDs. If it does not work like this (I believe 
it does) then we have problem somewhere else.
   
   In this case we just need to call the new function and that is it. Nothing 
else is needed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-09 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r406022480
 
 

 ##
 File path: nimble/host/src/ble_gap.c
 ##
 @@ -5605,6 +5605,41 @@ ble_gap_unpair_oldest_peer(void)
 return 0;
 }
 
+int
+ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer)
+{
+ble_addr_t oldest_peer_id_addr[MYNEWT_VAL(BLE_STORE_MAX_BONDS)];
+int num_peers;
+int rc, i;
+
+rc = ble_store_util_bonded_peers(
+_peer_id_addr[0], _peers, 
MYNEWT_VAL(BLE_STORE_MAX_BONDS));
+if (rc != 0) {
+return rc;
+}
+
+if (num_peers == 0) {
+return BLE_HS_ENOENT;
+}
+
+for (i = 0; i < num_peers; i++) {
+if (memcmp(curr_peer, _peer_id_addr[i], sizeof (ble_addr_t)) != 
0) {
 
 Review comment:
   I would use here `ble_addr_cmp()`


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-09 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r406030239
 
 

 ##
 File path: nimble/host/include/host/ble_gap.h
 ##
 @@ -1896,6 +1896,20 @@ int ble_gap_unpair(const ble_addr_t *peer_addr);
  */
 int ble_gap_unpair_oldest_peer(void);
 
+/**
+ * Similar to `ble_gap_unpair_oldest_peer()`, except it makes sure that current
+ * peer is not deleted.
+ *
+ * @param peer_addr Address of the current peer (not to be deleted)
+ *
+ * @return  0 on success;
+ *  A BLE host HCI return code if the controller
+ *  rejected the request;
+ *  A BLE host core return code on unexpected
+ *  error.
+ */
+int ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer);
 
 Review comment:
   let us call it `ble_gap_unpair_oldest_except` () and change the parameter 
name to `peer_addr`. 
   This functionality is not really dedicated only to current device. You might 
want to have device which you don't want to unpair no matter what :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-09 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r406023837
 
 

 ##
 File path: nimble/host/src/ble_gap.c
 ##
 @@ -5605,6 +5605,41 @@ ble_gap_unpair_oldest_peer(void)
 return 0;
 }
 
+int
+ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer)
+{
+ble_addr_t oldest_peer_id_addr[MYNEWT_VAL(BLE_STORE_MAX_BONDS)];
+int num_peers;
+int rc, i;
+
+rc = ble_store_util_bonded_peers(
+_peer_id_addr[0], _peers, 
MYNEWT_VAL(BLE_STORE_MAX_BONDS));
+if (rc != 0) {
+return rc;
+}
+
+if (num_peers == 0) {
+return BLE_HS_ENOENT;
+}
+
+for (i = 0; i < num_peers; i++) {
+if (memcmp(curr_peer, _peer_id_addr[i], sizeof (ble_addr_t)) != 
0) {
+break;
+}
+}
+
+if (i < num_peers) {
 
 Review comment:
   
   I would do it little simpler.
   
   ```
   if (i >= num_peers) {
return BLE_HS_ENOMEM;
   }
   
   return ble_gap_unpair(_peer_id_addr[i]);
   
   ```
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-08 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r405456758
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +315,43 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
+ble_addr_t peer_id_addr;
+int num_peers;
+
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+if ((rc = ble_gap_unpair_oldest_peer()) == BLE_HS_ENOENT) {
 
 Review comment:
   I believe that `ble_store_util_delete_peer()` deletes all what we need.  


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-06 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r404129458
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +315,43 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
+ble_addr_t peer_id_addr;
+int num_peers;
+
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+if ((rc = ble_gap_unpair_oldest_peer()) == BLE_HS_ENOENT) {
 
 Review comment:
   @prasad-alatkar are you sure that `ble_gap_unpair_oldest_peer()` will not 
unpair current peer? For me it looks it will.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-04 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r403511125
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,16 +315,43 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
+ble_addr_t peer_id_addr;
+int num_peers;
+
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
-case BLE_STORE_OBJ_TYPE_OUR_SEC:
-case BLE_STORE_OBJ_TYPE_PEER_SEC:
-case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
-
-default:
-return BLE_HS_EUNKNOWN;
+case BLE_STORE_OBJ_TYPE_OUR_SEC:
+case BLE_STORE_OBJ_TYPE_PEER_SEC:
+return ble_gap_unpair_oldest_peer();
+case BLE_STORE_OBJ_TYPE_CCCD:
+if ((rc = ble_gap_unpair_oldest_peer()) == BLE_HS_ENOENT) {
 
 Review comment:
   I was thinking to call here other version of `unpair_oldest` which would 
take as a parameter `ble_addr_t ` of device which we don't want to unpair.
   If the only device which could be unpair is the one provided, then unpair is 
not done and this function returns error.
   
   In such a case we know that we cannot store CCC and there is nothing we can 
do more here. All the other code is not needed then.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-03 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r402780122
 
 

 ##
 File path: nimble/host/src/ble_store_util.c
 ##
 @@ -230,13 +299,35 @@ ble_store_util_delete_oldest_peer(void)
 int
 ble_store_util_status_rr(struct ble_store_status_event *event, void *arg)
 {
+int rc = BLE_HS_EUNKNOWN;
+ble_addr_t peer_id_addr;
+int num_peers;
+
 switch (event->event_code) {
 case BLE_STORE_EVENT_OVERFLOW:
 switch (event->overflow.obj_type) {
 case BLE_STORE_OBJ_TYPE_OUR_SEC:
 case BLE_STORE_OBJ_TYPE_PEER_SEC:
 case BLE_STORE_OBJ_TYPE_CCCD:
-return ble_gap_unpair_oldest_peer();
 
 Review comment:
   I believe that for CCCD we should have different handling and if we want to 
unpair older peer we should not unpair peer for which we store CCCD.  If there 
is nothing to unpair just return the error.
   
   In the CCCD event there is and ble_addr we should use to make sure that one 
is no unpaired.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #790: nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit

2020-04-03 Thread GitBox
rymanluk commented on a change in pull request #790: nimble/store: Fix nimble 
store behavior when CCCDs exceed static defined limit
URL: https://github.com/apache/mynewt-nimble/pull/790#discussion_r402775932
 
 

 ##
 File path: nimble/host/src/ble_gap.c
 ##
 @@ -5594,7 +5594,7 @@ ble_gap_unpair_oldest_peer(void)
 }
 
 if (num_peers == 0) {
-return 0;
+return BLE_HS_ENOENT;
 
 Review comment:
   Can we make it as separate patch?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services