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

Desired changes to use this library in noble (@abandonware/noble) #71

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nmasse-itix
Copy link

Hello,

I'm using the library node-poweredup to drive Lego gears. This library uses noble to have a cross-platform abstraction of BLE services. And noble, under Linux, is still using HCI mode that is not working on recent distributions that I tested.

I managed to craft a binding for noble, using node-ble as a way to interact with BlueZ through DBUS.

However, to get it working, I had to change a few things.

  • Expose the ServiceData, ManufacturerData and UUIDs properties of the BlueZ Device interface
  • Add the device uuid to the connect/disconnect event so that noble knows which device got connected/disconnected.
  • Fix a race condition where the first notification of the device after StartNotify might get lost.
  • Send disconnect events (do not remove listeners on disconnection). This change seems to conflict with recent commit #6611591 on this repo.

How do you feel about it ? Any suggestion ?

@nmasse-itix
Copy link
Author

@chrvadala may I ask you to have a look at it ?

@chrvadala
Copy link
Owner

Hello @nmasse-itix thanks for your feedback. I think that node-ble can easily replace noble ( I developed it for this reason).
Below an answer to your points

Expose the ServiceData, ManufacturerData and UUIDs properties of the BlueZ Device interface

Quite soon I'll' merge this PR that adds these features

Add the device uuid to the connect/disconnect event so that noble knows which device got connected/disconnected.

These events are available. Did you see the the doc https://github.com/chrvadala/node-ble/blob/main/docs/api.md#device--eventemitter

Fix a race condition where the first notification of the device after StartNotify might get lost.

Can you provide more information? An example/test case can be useful.

Send disconnect events (do not remove listeners on disconnection). This change seems to conflict with recent commit #6611591 on this repo.

I thought that this was solved. Is there some corner case?

@nmasse-itix
Copy link
Author

Hello !

Sorry for the late reply. I managed to find some time today to work on it.

Expose the ServiceData, ManufacturerData and UUIDs properties of the BlueZ Device interface

Quite soon I'll' merge this PR that adds these features

In addition to the two properties that were added, I also need the following property in Device.js :

  /**
   * List of 128-bit UUIDs that represents the available remote services.
   * @returns {string[]}
   */
  async getServiceUUIDs () {
    return this.helper.prop('UUIDs')
  }

➡️ Would it be an acceptable change for you ?

Add the device uuid to the connect/disconnect event so that noble knows which device got connected/disconnected.

These events are available. Did you see the the doc https://github.com/chrvadala/node-ble/blob/main/docs/api.md#device--eventemitter

I took a step back and found a solution that does not involve any change in this library to get the device id. 👍

Fix a race condition where the first notification of the device after StartNotify might get lost.

Can you provide more information? An example/test case can be useful.

➡️ I created #72 with a code to reproduce the issue.

Send disconnect events (do not remove listeners on disconnection). This change seems to conflict with recent commit #6611591 on this repo.

I thought that this was solved. Is there some corner case?

➡️ I created #73 with a code to reproduce the issue.

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.

2 participants