search

jasongin / noble-uwp

Noble (Node.js Bluetooth LE) with Windows 10 UWP bindings
MIT License
83 stars 45 forks source link

Extending UWP specific methods. Format unification #51

Closed senergin closed 6 years ago

senergin commented 6 years ago
  1. Lack of signal strength filtering restricts noble-uwp itself.
  2. As is, noble-uwp is only reporting explicit scan responses and won't report passive advertisements. Setting a flag for this feature.
  3. For 1 and 2, I wanted the noble itself to have an entry point that goes beyond noble interface. Hence the .uwp extension.
  4. Some formatting unification
  5. Investigating a bug around shutdown (NodeRT subscription hanging around after object destruction), not relevant to this PR, but is also a concern. Would welcome feedback and other suggestions.
senergin commented 6 years ago

@jasongin

  1. Any other changes you'd like to see?
  2. AppVeyor build failure didn't make sense to me. Do you know what's going on?
  3. What is the path between from here to this being integrated and published for consumption?
jasongin commented 6 years ago

I'll go ahead and merge this and publish an updated noble-uwp package within a few days.

I'm still a little unsure about the scan-response filtering. Maybe there is a way to filter that at the application level? But with the default false at least that matches the behavior of noble on other platforms.

The AppVeyor failure is unrelated to this change. It was working just last week, but it looks like a dependency package recently started to emit a warning on install, and the AppVeyor script interprets it as an error. I'll fix that separately. Anyway, the primary purpose of the AppVeyor script is to publish pre-built binaries, not to validate PRs.

senergin commented 6 years ago

Thanks Jason! Looking forward for that package. ScanResponse type on Windows can be used, but it does not seem to be a field other noble bindings populate.

On Thu, Feb 22, 2018 at 10:45 AM, Jason Ginchereau <notifications@github.com

wrote:

I'll go ahead and merge this and publish an updated noble-uwp package within a few days.

I'm still a little unsure about the scan-response filtering. Maybe there is a way to filter that at the application level? But with the default false at least that matches the behavior of noble on other platforms.

The AppVeyor failure is unrelated to this change. It was working just last week, but it looks like a dependency package recently started to emit a warning on install, and the AppVeyor script interprets it as an error. I'll fix that separately. Anyway, the primary purpose of the AppVeyor script is to publish pre-built binaries, not to validate PRs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jasongin/noble-uwp/pull/51#issuecomment-367780419, or mute the thread https://github.com/notifications/unsubscribe-auth/ADowL4dvwz9bB4mC4M0HzqQadNkn08Zwks5tXbXmgaJpZM4SLurV .

-- Semih Energin

senergin commented 6 years ago

Hey Jason, just wanted to get back to you on this. Thanks

On Thu, Feb 22, 2018 at 10:57 AM, Semih Energin semihenergin@gmail.com wrote:

Thanks Jason! Looking forward for that package. ScanResponse type on Windows can be used, but it does not seem to be a field other noble bindings populate.

On Thu, Feb 22, 2018 at 10:45 AM, Jason Ginchereau < notifications@github.com> wrote:

I'll go ahead and merge this and publish an updated noble-uwp package within a few days.

I'm still a little unsure about the scan-response filtering. Maybe there is a way to filter that at the application level? But with the default false at least that matches the behavior of noble on other platforms.

The AppVeyor failure is unrelated to this change. It was working just last week, but it looks like a dependency package recently started to emit a warning on install, and the AppVeyor script interprets it as an error. I'll fix that separately. Anyway, the primary purpose of the AppVeyor script is to publish pre-built binaries, not to validate PRs.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jasongin/noble-uwp/pull/51#issuecomment-367780419, or mute the thread https://github.com/notifications/unsubscribe-auth/ADowL4dvwz9bB4mC4M0HzqQadNkn08Zwks5tXbXmgaJpZM4SLurV .

-- Semih Energin

-- Semih Energin

jasongin commented 6 years ago

Sorry for the delay. I was getting ready to merge this, and upon a second review I noticed what looks like a bug.

senergin commented 6 years ago

No problem, thank you for the review. Sorry for my oversight, sent a response.

jasongin commented 6 years ago

I published noble-uwp@0.6.0 with this change.

senergin commented 6 years ago

Thanks! Hopefully you and other consumers will find these changes useful as well.

On Mon, Mar 5, 2018 at 1:58 PM, Jason Ginchereau notifications@github.com wrote:

I published noble-uwp@0.6.0 with this change.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jasongin/noble-uwp/pull/51#issuecomment-370581756, or mute the thread https://github.com/notifications/unsubscribe-auth/ADowL1OIVz1nLochD8VbTS4N6-ZHV0u5ks5tbbUAgaJpZM4SLurV .

-- Semih Energin