Bluetooth #1245

drache42 · 2017-06-27T17:47:41Z

Created BluetoothLEHelper and samples pages

* added minimalistic documentation

…ated properly with Sample app

Bluetooth

dnfclas · 2017-06-27T17:47:46Z

@drache42,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

…into bluetooth

dnfclas · 2017-06-27T22:26:27Z

@drache42, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

nmetulev

This is amazing, thank you for submitting. Please resolve changes and we can merge as soon as those are fixed

nmetulev · 2017-06-28T05:03:46Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/BluetoothLEHelper/BluetoothLEHelperCode.bind

+
+// Connect to a device if your choice
+ObservableBluetoothLEDevice device = 
+bluetoothLEHelper.BluetoothLeDevices[<Device you choose>].Connect()


This is a bit confusing, can we expand on this? How do I chose a device?

How do you want me to do this? bluetoothLEHelper.BluetoothLeDevices is a collection of all LE devices in the area that are connectable. The device that the user wants to connect to has to be choosen somehow, but I thought the UI and such were out of scope for these code sample files.

Maybe add that as a comment, or even add the code in a "click handler". The way it is right now, it looks one code block and I would expect to simply copy and paste for it work. However, if I run those two lines one after the other, it will probably not work as BluetoothDevices will most likely be empty. Does that make sense?

nmetulev · 2017-06-28T05:06:13Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/samples.json

+      {
+        "Name": "BluetoothLEHelper",
+        "Type": "BluetoothLEHelperPage",
+        "About": "The Bluetooth LE helper class is used to connect and interact with bluetooth LE devices.",


why are these commented out and pointing to the wrong link?

nmetulev · 2017-06-28T05:08:49Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BluetoothLEHelper.cs

+        /// </summary>
+        /// <param name="sender">The sender.</param>
+        /// <param name="args">The args.</param>
+        private void _deviceWatcher_EnumerationCompleted(DeviceWatcher sender, object args)


Must begin with Uppercase

nmetulev · 2017-06-28T05:11:48Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BluetoothLEHelper.cs

+        /// <summary>
+        /// Gets a value indicating whether the Bluetooth LE Helper is supported.
+        /// </summary>
+        public bool IsBluetoothLeSupported => ApiInformation.IsApiContractPresent("Windows.Foundation.UniversalApiContract", 4);


To continue the same naming convention, should this be IsBluetoothLESupported

Also, this should be cached and only ran once

nmetulev · 2017-06-28T05:15:51Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BluetoothLEHelper.cs

+        /// </summary>
+        public void StartEnumeration()
+        {
+            if (_advertisementWatcher?.Status == BluetoothLEAdvertisementWatcherStatus.Started)


This seems like the method could be called multiple times and subscribe to the same events all over again creating a memory leak. Should check if Status is Stopped, etc, and react appropriately

nmetulev · 2017-06-28T05:32:25Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/ObservableGattCharacteristics.cs

@@ -0,0 +1,579 @@
+// ******************************************************************


There are several stylecop warnings in this file, please resolve

nmetulev · 2017-06-28T05:34:55Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/ObservableGattDeviceService.cs

@@ -0,0 +1,175 @@
+// ******************************************************************


There are several stylecop warnings in this file, please resolve

nmetulev · 2017-06-28T05:36:20Z

Microsoft.Toolkit.Uwp.Services/Microsoft.Toolkit.Uwp.Services.csproj

    <DefaultLanguage>en-US</DefaultLanguage>
    <TargetPlatformIdentifier>UAP</TargetPlatformIdentifier>
-    <TargetPlatformVersion>10.0.10586.0</TargetPlatformVersion>
+    <TargetPlatformVersion>10.0.15063.0</TargetPlatformVersion>


This doesn't seem necessary since all the code is in the Microsoft.Toolkit.Uwp project

nmetulev · 2017-06-28T05:37:41Z

docs/helpers/BluetoothLEHelper.md

+
+## API
+
+TODO: * [BluetoothLEHelper source code]()


nmetulev · 2017-06-28T05:38:08Z

docs/helpers/BluetoothLEHelper.md

@@ -0,0 +1,32 @@
+# BluetoothLEHelper


Please add more information about public properties, events, and methods

…oolkit into bluetooth

azchohfi · 2017-06-28T22:42:53Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/BluetoothLEHelper/BluetoothLEHelperPage.xaml

+            <ComboBox.ItemTemplate>
+                <DataTemplate x:DataType="bt:ObservableGattCharacteristics" >
+                    <TextBlock >
+                            <Run Text="Service Name: " />


Shouldn't it be "Characteristic Name: "?

azchohfi · 2017-06-28T22:44:58Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/BluetoothLEHelper/BluetoothLEHelperPage.xaml.cs

+
+        private async void BluetoothLEHelper_EnumerationCompleted(object sender, EventArgs e)
+        {
+            await Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>


Should it really need a dispatcher? Shouldn't the EnumerationCompleted event be raised on the UI thread?

The EnumerationCompleted comes from a different thread than the UI thread due to how a DeviceWatcher works so this is required

azchohfi · 2017-06-28T22:47:16Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/BluetoothLEHelper/BluetoothLEHelperPage.xaml.cs

+
+            if (characteristic != null)
+            {
+                characteristic.ReadValueAsync();


This is async but not awaited? Method is not returning Task. I think it should.

azchohfi · 2017-06-28T22:48:49Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/ObservableGattCharacteristics.cs

+        /// <summary>
+        /// Reads the value of the Characteristic
+        /// </summary>
+        public async void ReadValueAsync()


If method is async, it should return a Task.

azchohfi · 2017-06-28T22:56:00Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BluetoothLEHelper.cs

+        /// <summary>
+        /// Gets the app context
+        /// </summary>
+        public static BluetoothLEHelper Context { get; private set; } = new BluetoothLEHelper();


Context is never being set again. Would be better to use a get only property, not a private set;

azchohfi · 2017-06-28T23:00:56Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BluetoothLEHelper.cs

+                if (_readerWriterLockSlim.TryEnterWriteLock(TimeSpan.FromSeconds(1)))
+                {
+                    var device = BluetoothLeDevices.FirstOrDefault(i => i.DeviceInfo.Id == deviceInfoUpdate.Id);
+                    BluetoothLeDevices?.Remove(device);


This object is never null. Remove the "?".

azchohfi · 2017-06-28T23:03:37Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/BytePadder.cs

+    /// <summary>
+    /// Helper class used to pad bytes
+    /// </summary>
+    public static class BytePadder


Why was this class included in this PR? It's not being used anywhere else.

azchohfi · 2017-06-28T23:06:10Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/GattConvert.cs

+                bytes[i / 2] = Convert.ToByte(hex.Substring(i, 2), 16);
+            }
+
+            var writer = new DataWriter();


It's this a possible leak? Why are these 3 methods (ToIBuffer, ToIBufferFromHexString, ToIBuffer) added if they are not being used? Should the toolkit expose them?

azchohfi · 2017-06-28T23:09:58Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/ObservableBluetoothLEDevice.cs

+                return _services;
+            }
+
+            private set


No need to expose the private set if it's not being used. It's the same instance all the time.

azchohfi · 2017-06-28T23:13:50Z

Microsoft.Toolkit.Uwp/Helpers/BluetoothLEHelper/ObservableBluetoothLEDevice.cs

+        /// </summary>
+        /// <returns>Task.</returns>
+        /// <exception cref="Exception">The status of the pairing.</exception>
+        public async Task DoInAppPairing()


Many methods are returning Task but doesn't end with "async".

Kiran Pathakota and others added 24 commits March 14, 2017 15:31

First Commit for BluetoothLEHelper. Files migrated and renamed

cdc45bc

Making the C# files more toolkit friendly

0220d31

Added missing files that stopped this from compiling.

57d0580

Update project to officially released SDK

0bcaecd

Updated code with the latest fixes I made in the main app

7e581a4

Added Bluetooth sample code

a34ebe4

* Renamed BluetoothHelper to BluetoothLEHelper

6aacf4a

* added minimalistic documentation

Updated sample to be able to read characteristic value

4024eb7

Merging from Microsoft dev

c5651aa

Making some changes so that it's not UI bound part 1

3af8d65

Making changes to the code to reduce the threading complexity

497084b

Bluetooth LE Helper has been tidied up, but it needs testing

f79d5e4

Making the Gatt Convert class compliant

0d65f8e

Adding Error string class

4d20d13

Getting it ready for the toolkit

4686ecb

Bluetooth LE Helper Page updated

6bc6326

Updating the Observable Gatt Device Service. Still needs to be integr…

7d5f068

…ated properly with Sample app

Merge branch 'dev' into bluetooth

b20553f

Making sure the helpers are compliant

88459a8

Cleaning up Bindable Bluetooth objects

77f9b55

Making sure the code works with sample app

51c045f

Updating documentation

7485f97

Unwiring Enumeration Completed event when stop enumeration is called

255f309

Merge pull request #1 from shenchauhan/bluetooth

c435660

Bluetooth

dnfclas added the cla-required label Jun 27, 2017

nmetulev and others added 3 commits June 27, 2017 11:07

Merge branch 'dev' into bluetooth

9dfec6d

Updating headerS

6e41569

Merge branch 'dev' of https://github.com/Microsoft/UWPCommunityToolkit …

deb56ba

…into bluetooth

dnfclas added cla-signed and removed cla-required labels Jun 27, 2017

nmetulev added this to the v1.5 milestone Jun 27, 2017

nmetulev added the reviewers wanted label Jun 27, 2017

nmetulev requested changes Jun 28, 2017

View reviewed changes

Steffen Froehlich and others added 8 commits June 28, 2017 09:21

PR Updates

955cc99

fixed build errors

4963b8e

fixed up formatting

1c7cd90

updated samples.json to add badgeupdateversion

3408def

PR: updating .bind code to make it easier to read and use

54f1e58

Merge branch 'bluetooth' of https://github.com/drache42/UWPCommunityT…

d68e343

…oolkit into bluetooth

PR added public documentation

4e3338a

changed bind encoding

7726b16

nmetulev approved these changes Jun 28, 2017

View reviewed changes

azchohfi self-requested a review June 28, 2017 22:38

formatting fixes

4543fbd

azchohfi requested changes Jun 28, 2017

View reviewed changes

nmetulev added 2 commits June 28, 2017 19:03

fixed void Tasks, renamed methods with Async, removed unused properties

6a7cc82

removed unused classes and methods

34a8272

azchohfi approved these changes Jun 29, 2017

View reviewed changes

azchohfi merged commit adb28fd into CommunityToolkit:dev Jun 29, 2017

		@@ -0,0 +1,579 @@
		// ******************************************************************

		@@ -0,0 +1,175 @@
		// ******************************************************************

Bluetooth #1245

Bluetooth #1245

Uh oh!

Conversation

drache42 commented Jun 27, 2017

Uh oh!

dnfclas commented Jun 27, 2017

Uh oh!

dnfclas commented Jun 27, 2017

Uh oh!

nmetulev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmetulev Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nmetulev Jun 29, 2017 •

edited

Loading