[Bluetooth] Added BlueGiga USB dongle support #5885

kaikreuzer · 2018-07-09T19:50:40Z

This is a follow-up on #4535, which had BlueGiga dongle support, while I had removed it from the merged #4997.

I have meanwhile cleaned up the code, fixed a few further bugs that I came across, added a test fragment, adapted the code to EPLv2, updated the license headers and added a README file.

Also-By: Chris Jackson [email protected]
Also-By: Vlad Kolotov [email protected]
Signed-off-by: Kai Kreuzer [email protected]

htreu · 2018-07-10T07:15:54Z

Unfortunately karaf feature verification fails:

missing requirement [org.eclipse.smarthome.binding.bluetooth.bluegiga/0.10.0.201807092041] osgi.wiring.package; filter:="(osgi.wiring.package=gnu.io)"

kaikreuzer · 2018-07-10T07:30:53Z

This makes me think that it might be a good idea to also adapt the code to use the new ESH serial API. Will try to do so tonight!

kaikreuzer · 2018-07-10T21:39:11Z

Done.

@maggu2810, when doing this change, I noticed that the ESH Serial API is lacking the methods getName(), disableReceiveTimeout(), setFlowControlMode(), enableReceiveThreshold() on SerialPort - the previous code used all of those on the RXTX version. They all seem to exist on javax.comm.SerialPort as well, so I am wondering, why you didn't add them to the ESH API?

maggu2810 · 2018-07-11T07:29:37Z

I added the methods that are currently used.
I didn't see any reason to add all methods to our abstraction layer if there is no usage.

If we need that methods now it should be checked if every serial provider supports them and added.

triller-telekom

Thank you for adding the bluegiga dongle support.

What surprises me is the size of this PR. 15k LOC for "just another bluetooth dongle"? I see that Chris has auto-generated quite a lot of packages and responses, but are these really specific to bluegiga or can we move them over to the generic bluetooth binding and export them there?

I left a few comments in the code, which I stumbled across, please have a look.

triller-telekom · 2018-07-11T14:04:14Z

extensions/binding/org.eclipse.smarthome.binding.bluetooth.bluegiga.test/pom.xml

+      </plugin>
+    </plugins>
+  </build>
+


left over blank line

triller-telekom · 2018-07-11T14:04:33Z

...c/test/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/eir/EirRecordTest.java

+        assertTrue(serviceData.containsKey(dataServiceUUID));
+        assertArrayEquals(new int[] { 0x74, 0x01, 0x0d, 0x01, (byte) 0xec }, serviceData.get(dataServiceUUID));
+    }
+}


new line missing at the end

triller-telekom · 2018-07-11T14:06:26Z

extensions/binding/org.eclipse.smarthome.binding.bluetooth.bluegiga/cfg/ble.bluegiga.cfg

@@ -0,0 +1 @@
+# Ble.BlueGiga Binding Default Configuration


If there is no configuration, please remove this file.

Is there a reason for not removing this empty configuration file, or did you just overlook it?

triller-telekom · 2018-07-11T14:15:29Z

...ain/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/handler/BlueGigaBridgeHandler.java

+import java.util.concurrent.CopyOnWriteArraySet;
+
+import org.eclipse.jdt.annotation.DefaultLocation;
+import org.eclipse.jdt.annotation.NonNull;


Please remove references to @NonNull

triller-telekom · 2018-07-11T14:17:25Z

...ain/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/handler/BlueGigaBridgeHandler.java

+ * @author Chris Jackson - Initial contribution
+ * @author Kai Kreuzer - Made handler implement BlueGigaHandlerListener
+ */
+@NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE, DefaultLocation.ARRAY_CONTENTS,


What is the reason why you restrict the annotation, instead of using it without arguments?

triller-telekom · 2018-07-11T15:08:53Z

...ain/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/handler/BlueGigaBridgeHandler.java

+        command.setConnection(connectionHandle);
+        command.setStart(1);
+        command.setEnd(65535);
+        command.setUuid(UUID.fromString("00002800-0000-0000-0000-000000000000"));


This looks like a perfect candidate for a constant so this magic string gets a human readable meaning, doesn't it?

Did you see this comment?

triller-telekom · 2018-07-11T15:14:07Z

...in/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/BlueGigaEventListener.java

+package org.eclipse.smarthome.binding.bluetooth.bluegiga.internal;
+
+/**
+ * 


Please add a description for this interface

triller-telekom · 2018-07-11T15:17:49Z

.../src/main/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/BlueGigaPacket.java

+ * @author Chris Jackson
+ *
+ */
+public class BlueGigaPacket {


Given the size of this PR and having a packet format specific to this dongle, I am wondering whether each bluetooth dongle sends different packets? If not, can't we move these definitions into the common bluetooth binding and export them there for all dongles to use?

triller-telekom · 2018-07-11T15:19:35Z

...in/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/BlueGigaSerialHandler.java

+    private boolean notifyTransactionComplete(final BlueGigaResponse response) {
+        boolean processed = false;
+
+        // logger.debug("NODE {}: notifyTransactionResponse {}",


left over commented out code

triller-telekom · 2018-07-11T15:20:58Z

...in/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/BlueGigaSerialHandler.java

+        }
+    }
+
+    // TODO: Add a timeout mechanism


Seeing the TRANSACTION_TIMEOUT_PERIOD parameter I guess this comment is now obsolete?

htreu · 2018-07-12T10:30:44Z

.../java/org/eclipse/smarthome/binding/bluetooth/bluegiga/internal/BlueGigaResponsePackets.java

+/**
+ * Helper class to create BlueGiga BLE Response and Event packets (ie packets that we will receive).
+ * <p>
+ * Note that this code is autogenerated. Manual changes may be overwritten.


I noticed several classes to have this note. Why do we check in autogenerated code? What was the source of this files? Wouldn't it be more future prove to add the original source and the generator to the project?

maggu2810 · 2018-07-12T11:39:11Z

extensions/binding/org.eclipse.smarthome.binding.bluetooth.bluegiga/META-INF/MANIFEST.MF

+Service-Component: OSGI-INF/*.xml
+Export-Package: org.eclipse.smarthome.binding.bluetooth.bluegiga,
+ org.eclipse.smarthome.binding.bluetooth.bluegiga.handler
+Bundle-ActivationPolicy: lazy


Remove that line and add Automatic-Module-Name

kaikreuzer · 2018-07-12T21:34:00Z

Thanks @maggu2810, @triller-telekom & @htreu, I have incorporated all your review comments.
Wrt the generated code: I believe that this was mainly a one-time generation to avoid a lot of typing; I do not see that this is required as a regular step for maintaining this code. Nonetheless, I kept the warning in the JavaDoc in place as touching code that has once been generated usually adheres to a strict structure where manual changes are likely to break things unintentionally.

@triller-telekom I do not know whether this code is only applicable for BlueGiga or also to other dongles / BT support. But as long as nobody expresses the need to reuse it at some other place, I very much prefer to keep it internal here. I myself have never missed it in the generic bluetooth support.

@maggu2810 I have added the additional methods in the ESH Serial API within this PR, I hope this is ok.

In general, I'd like to mention that this code might not in all places be fully stable, tested and production-ready - this is why I kept is so long in my own fork. But as it is immensely useful when developing bluetooth stuff on a non-Linux machine, I think it should be made available in ESH to all developers. In a typical production scenario, almost everyone will probably rather use Linux with BlueZ.

I've kept the PR as a single commit. If you do not find any severe issues with it anymore, it would be great if a CQ could be filed for it.

triller-telekom

Thank you for your changes. You seem to have forgotten one emtpy config file and there is a magic UUID as a string in the code. Wouldn't it be nicer to have it as a constant with a nice name?

Otherwise the code looks good to me now.

triller-telekom · 2018-07-13T07:54:50Z

extensions/binding/org.eclipse.smarthome.binding.bluetooth.bluegiga/cfg/ble.bluegiga.cfg

@@ -0,0 +1 @@
+# Ble.BlueGiga Binding Default Configuration


Is there a reason for not removing this empty configuration file, or did you just overlook it?

triller-telekom · 2018-07-13T08:00:14Z

...ain/java/org/eclipse/smarthome/binding/bluetooth/bluegiga/handler/BlueGigaBridgeHandler.java

+        command.setConnection(connectionHandle);
+        command.setStart(1);
+        command.setEnd(65535);
+        command.setUuid(UUID.fromString("00002800-0000-0000-0000-000000000000"));


Did you see this comment?

maggu2810 · 2018-07-13T15:54:38Z

@maggu2810 I have added the additional methods in the ESH Serial API within this PR, I hope this is ok.

As there also exists #5560 that adds another serial implementation have you already checked if this implementation supports the implementation of the new interface methods, too?
I assume that the first merge breaks the second one.
If #5560 is merged, you have to add the interface implementation to the new serial implementation.
If this one is merged, @msteigenberger has to add the interface implementation to fit to the new serial interface.

maggu2810 · 2018-07-14T08:54:35Z

Didn't we agree that new classes / interfaces should already contain the @NonNullByDefault annotation?

kaikreuzer · 2018-07-14T20:19:27Z

You seem to have forgotten one emtpy config file

Indeed, that got lost in the commit, sorry. Now it is removed.

there is a magic UUID as a string in the code

The reason I left it there is that it is a magic string for me, too; I do not know, what a good constant name would be for it.

Didn't we agree that new classes / interfaces should already contain the @NonNullByDefault annotation?

I do not remember such a strict decision. If so, this would be mainly for framework interfaces, but I do not think we enforce it for any internal class of any extension. We only expect that no @NonNull annotations are used and this is what I have already fixed after @triller-telekom's remark.
Note that this code is also already a year old and I didn't write it - I merely think it should be added to the repo, but I don't want to take the effort to do huge refactorings to it.

have you already checked if this implementation supports the implementation of the new interface methods, too?

Good point. I just checked: Yes, the TelnetSerialPort also has those methods, so it should be easy to adapt #5560 accordingly. To avoid dependencies between this PR and #5560, I have now created the separate PR #5907, which should be merged before any of the other two. This PR here will now hence fail to compile until #5907 is merged - but it imho should not prevent us from already creating a CQ for it.

cdjackson · 2018-07-15T09:41:30Z

I just noticed that this PR copies the source from https://github.com/zsmartsystems/com.zsmartsystems.bluetooth.bluegiga. Can you add a note to this effect in the NOTICE file please?

kaikreuzer · 2018-07-15T21:37:21Z

The code actually originates from extracting the sources that were included in the zip in #3633, which had then been modified in #4535 to match the ESH bluetooth API.
You are right, this effectively means they are a modified version of the code at https://github.com/zsmartsystems/com.zsmartsystems.bluetooth.bluegiga.

The NOTICE file is usually used for referencing third party code that is included as a binary in the distribution (so that it is clear where to find the source code). But if you wish to also add some note wrt zsmartsystems, please suggest a text and I will add it.

Also-By: Chris Jackson <[email protected]> Also-By: Vlad Kolotov <[email protected]> Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer · 2018-07-18T07:43:53Z

@maggu2810 & @htreu: I have fixed the travis build issue, so imho this is ready for a CQ - any further changes can be done through separate follow-up PRs.

htreu · 2018-07-18T14:00:05Z

FTR: CQ16983 created.

maggu2810 · 2018-07-25T09:13:12Z

CQ16983 has been approved.

cdjackson · 2018-07-28T19:14:58Z

@kaikreuzer sorry - I didn't see your response to the above. Note that if you're using the original code from 18 months ago, then there were a number of bug fixes since then.

Since this has been merged now I will create a separate PR to reference the library if that's ok?

kaikreuzer added the Binding-Bluetooth label Jul 9, 2018

kaikreuzer force-pushed the bluegiga branch from 7d6cce2 to ca9a89f Compare July 9, 2018 19:51

kaikreuzer force-pushed the bluegiga branch from ca9a89f to ec584ef Compare July 10, 2018 21:34

triller-telekom reviewed Jul 11, 2018

View reviewed changes

htreu reviewed Jul 12, 2018

View reviewed changes

maggu2810 reviewed Jul 12, 2018

View reviewed changes

kaikreuzer force-pushed the bluegiga branch from ec584ef to ceb99bf Compare July 12, 2018 21:23

triller-telekom reviewed Jul 13, 2018

View reviewed changes

kaikreuzer force-pushed the bluegiga branch from ceb99bf to 48e12f8 Compare July 14, 2018 16:35

kaikreuzer mentioned this pull request Jul 14, 2018

added further methods to SerialPort #5907

Merged

kaikreuzer force-pushed the bluegiga branch from 48e12f8 to 5dbd817 Compare July 14, 2018 20:19

maggu2810 mentioned this pull request Jul 15, 2018

Add Serial-over-IP (rfc2217) support in ESH Serial API #5560

Merged

kaikreuzer force-pushed the bluegiga branch from 5dbd817 to e5243f6 Compare July 15, 2018 21:55

Added BlueGiga USB dongle support for Bluetooth binding

4108b98

Also-By: Chris Jackson <[email protected]> Also-By: Vlad Kolotov <[email protected]> Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer force-pushed the bluegiga branch from e5243f6 to 4108b98 Compare July 17, 2018 21:48

htreu added the CQ label Jul 18, 2018

maggu2810 merged commit d5954e0 into eclipse-archived:master Jul 25, 2018

cdjackson mentioned this pull request Aug 7, 2018

Add reference to original source for Bluegiga library #6017

Merged

kaikreuzer removed the CQ label Aug 27, 2018

kaikreuzer deleted the bluegiga branch August 27, 2018 09:14

htreu added this to the 0.10.0 milestone Oct 30, 2018

kaikreuzer changed the title ~~Added BlueGiga USB dongle support for Bluetooth binding~~ [Bluetooth] Added BlueGiga USB dongle support Dec 13, 2018

kaikreuzer added the enhancement label Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bluetooth] Added BlueGiga USB dongle support #5885

[Bluetooth] Added BlueGiga USB dongle support #5885

kaikreuzer commented Jul 9, 2018

htreu commented Jul 10, 2018

kaikreuzer commented Jul 10, 2018

kaikreuzer commented Jul 10, 2018

maggu2810 commented Jul 11, 2018

triller-telekom left a comment

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 13, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 13, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

triller-telekom Jul 11, 2018

htreu Jul 12, 2018

maggu2810 Jul 12, 2018

kaikreuzer commented Jul 12, 2018

triller-telekom left a comment

triller-telekom Jul 13, 2018

triller-telekom Jul 13, 2018

maggu2810 commented Jul 13, 2018

maggu2810 commented Jul 14, 2018

kaikreuzer commented Jul 14, 2018 •

edited

Loading

cdjackson commented Jul 15, 2018

kaikreuzer commented Jul 15, 2018

kaikreuzer commented Jul 18, 2018 •

edited

Loading

htreu commented Jul 18, 2018

maggu2810 commented Jul 25, 2018

cdjackson commented Jul 28, 2018

[Bluetooth] Added BlueGiga USB dongle support #5885

[Bluetooth] Added BlueGiga USB dongle support #5885

Conversation

kaikreuzer commented Jul 9, 2018

htreu commented Jul 10, 2018

kaikreuzer commented Jul 10, 2018

kaikreuzer commented Jul 10, 2018

maggu2810 commented Jul 11, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Jul 12, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 commented Jul 13, 2018

maggu2810 commented Jul 14, 2018

kaikreuzer commented Jul 14, 2018 • edited Loading

cdjackson commented Jul 15, 2018

kaikreuzer commented Jul 15, 2018

kaikreuzer commented Jul 18, 2018 • edited Loading

htreu commented Jul 18, 2018

maggu2810 commented Jul 25, 2018

cdjackson commented Jul 28, 2018

kaikreuzer commented Jul 14, 2018 •

edited

Loading

kaikreuzer commented Jul 18, 2018 •

edited

Loading