abandonware / noble

A Node.js BLE (Bluetooth Low Energy) central module : Community maintained
https://libraries.io/npm/@abandonware%2Fnoble
MIT License
547 stars 161 forks source link

Revert the change for reseting service data on non scan hci events #297

Closed lordthorzonus closed 1 year ago

lordthorzonus commented 1 year ago

See the background discussion and logs what are happening here: https://github.com/abandonware/noble/issues/296

This commit https://github.com/abandonware/noble/commit/b67eea246f719947fc45b1b52b856e61637a8a8e changed the logic where the service data is reset from the meta event type scan to ever having a scan response discovered, and so leading to ever growing serviceData arrays in advertisements.

I reverted the logic to the one that was existing before this commit and introduced a test for catching the problem. I'm not so familiar with this codebase so this could use a another pair of eyes too 😅.

Apollon77 commented 1 year ago

@kele6ra ... please have a look too. Your PR introduced the issue. Please advice.

kele6ra commented 1 year ago

Hello, I can't understand what you have changed. Because we have:

  if (type === LE_META_EVENT_TYPE_SCAN_RESPONSE) {
    hasScanResponse = true;
  }

And you haven't changed logic. Maybe in another file, but not in the gap.js. It was very long ago, and I don't remember what I have done there )) But in the 276 row you have leMetaEventType instead of type. It could be a reason.

lordthorzonus commented 1 year ago

@kele6ra 👋 the hasScanResponse is already set true above it: https://github.com/abandonware/noble/blob/67e3936f8560f62d753866286a4368d26b32d82a/lib/hci-socket/gap.js#L131-L133

So in case we receive an event 0x04 (scan) and then 0x01 the serviceData array is not reset cause because of hasScanResponse is then true. One can reproduce this with the test that I added, if we rely on the hasScanResponse the test shows that the serviceData array just keeps growing on new events 🤔

flowchart TD
 event1[Event 0x04] --> advertisement1[This peripheral is now previously discovered and its hasScanResponse is true]
 advertisement1 --> event2[Some other event received]
 event2--> nextStep[The service data should be reset, but it's not as hasScanResponse is true as this is discovered already]
 nextStep--> loop[This loops and service data just gets appended]

It looks like this in real life:

2023-02-21T19:06:01.383Z gap advertisement = {"localName":"Flower care","serviceData":[{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,23,156,6,110,141,124,196,13,7,16,3,115,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,24,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,25,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,26,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,27,156,6,110,141,124,196,13,7,16,3,134,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,27,156,6,110,141,124,196,13,7,16,3,134,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,27,156,6,110,141,124,196,13,7,16,3,134,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,27,156,6,110,141,124,196,13,7,16,3,134,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,28,156,6,110,141,124,196,13,8,16,1,35]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,29,156,6,110,141,124,196,13,9,16,2,197,1]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,30,156,6,110,141,124,196,13,4,16,2,234,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,31,156,6,110,141,124,196,13,7,16,3,97,0,0]}},{"uuid":"fe95","data":{"type":"Buffer","data":[113,32,152,0,31,156,6,110,141,124,196,13,7,16,3,97,0,0]}}],"serviceUuids":["fe95"],"solicitationServiceUuids":[],"serviceSolicitationUuids":[]}

But reverting this logic to rely just on the event seems to fix this

kele6ra commented 1 year ago

Sorry, but I can't check it now. Looks like you are right.

Apollon77 commented 1 year ago

I will also look into the PR later today hopefully ... but seems legit to me - and yes the ever growing servicedata is a bad bug right noe ... @rzr release work incoming ;-))