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

Initial commit of Bluetooth binding and Bluetooth transport bundles #3531

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@maggu2810
Contributor

maggu2810 commented May 29, 2017

The code base should be equal to: #551

  • based on the current master with all conflicts solved
  • cleaned up POM files
  • fixed license headers
  • Java is set to 1.8
  • fixed classpath
  • applied clean up rules
  • re-formatted ESH-INF XML files
  • renamed yeelight to yeelightblue

IMHO the code itself needs further work.

  • don't use printStackTrace
  • don't catch exceptions and ignore them
  • apply changes to work with the current Bluez versions
  • etc.

BUT it should be enough to create a CQ and merge it, so further improvements could be done using a central place and code base.

We should ensure that someone is working (again) on that cleanup.

maggu2810 added some commits May 29, 2017

Initial commit of Bluetooth binding and Bluetooth transport bundles
This is a rebased version of #551

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
fix POM files and remove obsolete header text file
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
bluetooth: fix license headers
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
bluetooth: use Java 1.8
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
bluetooth: fix classpath
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

maggu2810 added some commits May 29, 2017

bluetooth: run clean up rules
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
bluetooth: format ESH-INF XML files
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@cdjackson

This comment has been minimized.

Show comment
Hide comment
@cdjackson

cdjackson May 29, 2017

Contributor
Contributor

cdjackson commented May 29, 2017

@maggu2810

This comment has been minimized.

Show comment
Hide comment
@maggu2810

maggu2810 May 29, 2017

Contributor

I created #3533 for the failed integration build.

@cdjackson I don't care if the openHAB binding changes its name to yeelightwifi or we change the ESH binding to yeelightblue (perhaps yeelight-wifi and yeelight-blue would make sense, so both state the transport it is using).

Should I change yeelight to yeelightblue or yeelight-blue?

Contributor

maggu2810 commented May 29, 2017

I created #3533 for the failed integration build.

@cdjackson I don't care if the openHAB binding changes its name to yeelightwifi or we change the ESH binding to yeelightblue (perhaps yeelight-wifi and yeelight-blue would make sense, so both state the transport it is using).

Should I change yeelight to yeelightblue or yeelight-blue?

@cdjackson

This comment has been minimized.

Show comment
Hide comment
@cdjackson

cdjackson May 29, 2017

Contributor
Contributor

cdjackson commented May 29, 2017

@kaikreuzer

This comment has been minimized.

Show comment
Hide comment
@kaikreuzer

kaikreuzer May 29, 2017

Member

Should I change yeelight to yeelightblue or yeelight-blue?

We so far do not use any hyphens in binding ids, so I would clearly vote for yeelightblue.

Member

kaikreuzer commented May 29, 2017

Should I change yeelight to yeelightblue or yeelight-blue?

We so far do not use any hyphens in binding ids, so I would clearly vote for yeelightblue.

rename yeelight to yeelightblue
Related to: #3531 (comment)
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@maggu2810

This comment has been minimized.

Show comment
Hide comment
@maggu2810

maggu2810 May 29, 2017

Contributor

renamed yeelight to yeelightblue

Contributor

maggu2810 commented May 29, 2017

renamed yeelight to yeelightblue

fix version in manifest
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
timerTask = null;
}
public float[] RGBtoHSB(int r, int g, int b, float[] hsbvals) {

This comment has been minimized.

@svilenvul

svilenvul May 31, 2017

Contributor

I understand that the purpose of this PR is to apply only the most important changes, but I just want to mention that this method seems to me redundant, as it is already implemented in ESH - see HSBType.fromRGB()

@svilenvul

svilenvul May 31, 2017

Contributor

I understand that the purpose of this PR is to apply only the most important changes, but I just want to mention that this method seems to me redundant, as it is already implemented in ESH - see HSBType.fromRGB()

This comment has been minimized.

@maggu2810

maggu2810 May 31, 2017

Contributor

What do you think about create an issue to collect stuff that should be cleaned up?

@maggu2810

maggu2810 May 31, 2017

Contributor

What do you think about create an issue to collect stuff that should be cleaned up?

This comment has been minimized.

@kaikreuzer

kaikreuzer May 31, 2017

Member

@svilenvul Or alternatively feel free to directly commit fixes/clean-ups on a personal branch in your fork and we could pull that into this PR before it is going to be merged.

@kaikreuzer

kaikreuzer May 31, 2017

Member

@svilenvul Or alternatively feel free to directly commit fixes/clean-ups on a personal branch in your fork and we could pull that into this PR before it is going to be merged.

This comment has been minimized.

@maggu2810

maggu2810 May 31, 2017

Contributor

So, when do we want to create a CQ? Doesn't every change of the code base in this PR result in a delay of the initial CQ?
Wouldn't it be better to create a CQ for the initial code base and then allow us all the other changes (as long as they consists of <1k LOC) to be merged immediately without a CQ?

@maggu2810

maggu2810 May 31, 2017

Contributor

So, when do we want to create a CQ? Doesn't every change of the code base in this PR result in a delay of the initial CQ?
Wouldn't it be better to create a CQ for the initial code base and then allow us all the other changes (as long as they consists of <1k LOC) to be merged immediately without a CQ?

This comment has been minimized.

@kaikreuzer

kaikreuzer May 31, 2017

Member

I wanted to at least spend an hour this week to roughly go over the code to see if it is ok to create a CQ (and thus merge as is) or if we (I) should do any changes to it upfront. In the meantime, I think it is ok for @svilenvul to work on improvements in his own branch - those can then result in a follow-up PR once this one is merged, right?

@kaikreuzer

kaikreuzer May 31, 2017

Member

I wanted to at least spend an hour this week to roughly go over the code to see if it is ok to create a CQ (and thus merge as is) or if we (I) should do any changes to it upfront. In the meantime, I think it is ok for @svilenvul to work on improvements in his own branch - those can then result in a follow-up PR once this one is merged, right?

This comment has been minimized.

@maggu2810

maggu2810 May 31, 2017

Contributor

in a follow-up PR once this one is merged

Sure, I fully agree with that approach. 👍

I wanted to at least spend an hour this week to roughly go over the code to see if it is ok to create a CQ (and thus merge as is) or if we (I) should do any changes to it upfront.

Great 😉

@maggu2810

maggu2810 May 31, 2017

Contributor

in a follow-up PR once this one is merged

Sure, I fully agree with that approach. 👍

I wanted to at least spend an hour this week to roughly go over the code to see if it is ok to create a CQ (and thus merge as is) or if we (I) should do any changes to it upfront.

Great 😉

This comment has been minimized.

@svilenvul

svilenvul Jun 2, 2017

Contributor

Thanks for giving me green light to apply some changes, I have put this on my TODO list :)

@svilenvul

svilenvul Jun 2, 2017

Contributor

Thanks for giving me green light to apply some changes, I have put this on my TODO list :)

This comment has been minimized.

@kaikreuzer

kaikreuzer Jun 2, 2017

Member

I have put this on my TODO list :)

Better refrain from it for the moment - I just had a call with Chris and I will write a small summary soon; we agreed on some substantial refactoring of the code :-/

@kaikreuzer

kaikreuzer Jun 2, 2017

Member

I have put this on my TODO list :)

Better refrain from it for the moment - I just had a call with Chris and I will write a small summary soon; we agreed on some substantial refactoring of the code :-/

This comment has been minimized.

@svilenvul

svilenvul Jun 2, 2017

Contributor

OK, I haven't read you comment below before :)

@svilenvul

svilenvul Jun 2, 2017

Contributor

OK, I haven't read you comment below before :)

@kaikreuzer

This comment has been minimized.

Show comment
Hide comment
@kaikreuzer

kaikreuzer Jun 1, 2017

Member

Having had a closer look at the code, I am not really too keen to merge it as is anymore.

Especially the Yeelight binding shows that there are huge issues in the architecture:

  • It implements a discovery service that triggers general BT device discovery. Imho we should have a BTDiscoveryService with a Participant interface, similar to UPnP and mDNS.
  • The whole code depends on the BluetoothAdapter, its correct configuration, etc and has to handle all potential issues. I think it would be much better to model the BluetoothAdapter as a Bridge - this would easily allow adding other Bridges like the BlueGiga dongle.
  • Imho, we SHOULD after all have a "BLE Binding", which defines the bridge and provides the connectivity (i.e. parts what is in the transport.bluetooth.bluez) and additional bundles that then come with specific thing-types and handler implementations.

Additionally, the Yeelight binding is in a bad shape:

  • The binding has switch/dimmer/color channels side by side, which is wrong.
  • It defines channels like Temperature & Luminance, which are not reflected anywhere in the code.
  • Its config parameters are all device management actions, which is something we do not really officially support (and where I do not even see how they are used).
  • The unique ID is added as a property, although this needs to be a config parameter for the handler.

I tried to refactor the binding, but ended up with hardly any code left in place. As the binding isn't much code anyhow, I would suggest to completely remove it from the PR, so that it does not accidentally serve as a template for others.

For the other two transport bundles, I would create a CQ then, unless @cdjackson has again time for helping on that code, so that we could discuss potential refactorings wrt the BlueGiga dongle.

Member

kaikreuzer commented Jun 1, 2017

Having had a closer look at the code, I am not really too keen to merge it as is anymore.

Especially the Yeelight binding shows that there are huge issues in the architecture:

  • It implements a discovery service that triggers general BT device discovery. Imho we should have a BTDiscoveryService with a Participant interface, similar to UPnP and mDNS.
  • The whole code depends on the BluetoothAdapter, its correct configuration, etc and has to handle all potential issues. I think it would be much better to model the BluetoothAdapter as a Bridge - this would easily allow adding other Bridges like the BlueGiga dongle.
  • Imho, we SHOULD after all have a "BLE Binding", which defines the bridge and provides the connectivity (i.e. parts what is in the transport.bluetooth.bluez) and additional bundles that then come with specific thing-types and handler implementations.

Additionally, the Yeelight binding is in a bad shape:

  • The binding has switch/dimmer/color channels side by side, which is wrong.
  • It defines channels like Temperature & Luminance, which are not reflected anywhere in the code.
  • Its config parameters are all device management actions, which is something we do not really officially support (and where I do not even see how they are used).
  • The unique ID is added as a property, although this needs to be a config parameter for the handler.

I tried to refactor the binding, but ended up with hardly any code left in place. As the binding isn't much code anyhow, I would suggest to completely remove it from the PR, so that it does not accidentally serve as a template for others.

For the other two transport bundles, I would create a CQ then, unless @cdjackson has again time for helping on that code, so that we could discuss potential refactorings wrt the BlueGiga dongle.

@cdjackson

This comment has been minimized.

Show comment
Hide comment
@cdjackson

cdjackson Jun 2, 2017

Contributor
Contributor

cdjackson commented Jun 2, 2017

@kaikreuzer

This comment has been minimized.

Show comment
Hide comment
@kaikreuzer

kaikreuzer Jun 2, 2017

Member

@cdjackson I tried to put our discussion in a diagram:

ble

The general scheme is that we should not think of the "BLE Binding" to be a single bundle, but rather a feature (i.e. a set of bundles, not all of which need to be installed).

  • The "base" BLE bundle is the central one that is always required.
    • It has the "ble" binding.xml
    • It contains an implementation of a discovery service (which requires the presence of a BLEBridge)
    • It defines a (yet to be designed) high level BLE-API interface for specific device support bundles as the current Android-like API is too low level.
  • BLEBridge implementations can be added as separate bundles.
    • They come with a bridge-type (which defines their required configuration parameters) and an according handler implementations
    • Their handlers implement the BLE-API interface.
    • We can have one bundle that brings a handler that uses BlueZ over DBUS and another bundle bringing support for BlueGiga USB dongle.
  • Specific device support can as well be added via separate bundles.
    • These bundles register a BLEDiscoveryParticipant and thus can provide DiscoveryResults for specific devices.
  • They define a specific thing-type and come with a specific handler implementation.
  • Their handlers have access to the bridge, which by definition implements the BLE-API interface, which can hence be used by those handlers.

This should all nicely fit into the current framework architecture (actually, the binding API had been designed specifically in a way to support that, so I think it is time to actually use it that way :-)).
There is one issue that comes to my mind, though: All thing-types will have to reference all possible bridge-types in their XML, which is not nice. We should check whether it is possible to also allow some kind of placeholder, so that specific bundles do not need to be aware of all potential BLEBridge implementations.

Chris, does that match our conversation? Do you have anything to add?
If you have time to work on such refactorings, I would suggest to leave this PR open (after all) for now.
As @svilenvul is also eagerly waiting to get his hands into BLE, please let us discuss how the work could be possibly shared. Could e.g. @svilenvul already start with creating the suggested bundle structure?

Member

kaikreuzer commented Jun 2, 2017

@cdjackson I tried to put our discussion in a diagram:

ble

The general scheme is that we should not think of the "BLE Binding" to be a single bundle, but rather a feature (i.e. a set of bundles, not all of which need to be installed).

  • The "base" BLE bundle is the central one that is always required.
    • It has the "ble" binding.xml
    • It contains an implementation of a discovery service (which requires the presence of a BLEBridge)
    • It defines a (yet to be designed) high level BLE-API interface for specific device support bundles as the current Android-like API is too low level.
  • BLEBridge implementations can be added as separate bundles.
    • They come with a bridge-type (which defines their required configuration parameters) and an according handler implementations
    • Their handlers implement the BLE-API interface.
    • We can have one bundle that brings a handler that uses BlueZ over DBUS and another bundle bringing support for BlueGiga USB dongle.
  • Specific device support can as well be added via separate bundles.
    • These bundles register a BLEDiscoveryParticipant and thus can provide DiscoveryResults for specific devices.
  • They define a specific thing-type and come with a specific handler implementation.
  • Their handlers have access to the bridge, which by definition implements the BLE-API interface, which can hence be used by those handlers.

This should all nicely fit into the current framework architecture (actually, the binding API had been designed specifically in a way to support that, so I think it is time to actually use it that way :-)).
There is one issue that comes to my mind, though: All thing-types will have to reference all possible bridge-types in their XML, which is not nice. We should check whether it is possible to also allow some kind of placeholder, so that specific bundles do not need to be aware of all potential BLEBridge implementations.

Chris, does that match our conversation? Do you have anything to add?
If you have time to work on such refactorings, I would suggest to leave this PR open (after all) for now.
As @svilenvul is also eagerly waiting to get his hands into BLE, please let us discuss how the work could be possibly shared. Could e.g. @svilenvul already start with creating the suggested bundle structure?

@cdjackson

This comment has been minimized.

Show comment
Hide comment
@cdjackson

cdjackson Jun 2, 2017

Contributor
Contributor

cdjackson commented Jun 2, 2017

@kaikreuzer

This comment has been minimized.

Show comment
Hide comment
@kaikreuzer

kaikreuzer Jul 24, 2017

Member

If I am not mistaken, this is superseded by #3633 - unless anyone does not agree with the suggested refactoring of the architecture.

Member

kaikreuzer commented Jul 24, 2017

If I am not mistaken, this is superseded by #3633 - unless anyone does not agree with the suggested refactoring of the architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment