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

[Tradfri] Added support for remote controller and motion sensor devices #4373

Merged
merged 5 commits into from Nov 3, 2017

Conversation

Projects
None yet
9 participants
@cweitkamp
Copy link
Contributor

commented Oct 4, 2017

The TRÅDFRI controller and sensor devices currently cannot be observed directly using the coap protocol. We assume that they are communicating directly with the bulbs or lamps without routing their commands through the gateway. This makes it nearly impossible to trigger events or redirect any kind of information to ESH/OH2. Except using workarounds like defining TRÅDFRI groups (see PR #3799).

My approach adds the possibility to include those devices using the binding to the users environment and retrieve read-only data like presence status or battery level. I am happy to hear any feedback.

  • Moved config classes to internal package
  • Added support for remote controller and motion sensor devices (read-only, battery level and battery low channels)
  • Implement an abstract TradfriThingHandler for TradfriControllerHandler, TradfriLightHandler and TradfriSensorHandler
  • Extract classes ControllerData, LightData and SensorData from handlers, move to own package (e.g. o.e.s.b.tradfri.internal.model) and implement a reusable abstract TradfriData class
  • Documentation
  • Unit Tests
  • Find a person who owns a TRÅDFRI dimmer

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

@hreichert

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@cweitkamp Sounds great! Would be very cool to integrate the battery levels in my existing monitoring!
Also the refactoring is good idea. Maybe @S0urceror (or someone else) can re-build the group support on top of this.

@S0urceror

This comment has been minimized.

Copy link

commented Oct 6, 2017

Hopefully I have time soon to add my Group support back into this release. Great work!

@cweitkamp cweitkamp changed the title [WIP][Tradfri] Added support for remote controller and motion sensor devices [Tradfri] Added support for remote controller and motion sensor devices Oct 8, 2017

@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2017

I pushed my latest changes and I think this PR has left the [WIP] status. I am looking forward for review comments and any kind of feedback.

I own a remote controller and a motion sensor. If anybody owns a TRÅDFRI wireless dimmer, please contact me for testing it.

@sjka sjka added the Translation label Oct 9, 2017

@sjka

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

I have both, a wireless remote control and a wireless dimmer. They are both discovered successfully and added to the inbox. After having them approved, the dimmer stayed offline at first, but after a restart into debug mode it then also worked instantly. So I have no idea what initially went wrong.
In any case, they both nicely report their battery levels. Is there anything else I should have a closer look at?

@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

Sounds good. Question is which thing-type is used for each device? I assume 830 in both cases. Do you think that is correct? Imho the wireless dimmer should be discovered as thing-type 820. Currently I have no clue how to distinguish between the response results of those devices - except comparing against the json property TYPE = "5750" -> DEVICE_MODEL = "1". Wdyt?

@sjka

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

Yes, they were discovered as 830 both.

I agree, according to the ZLL standard 820 would be the better match for the wireless dimmer.

Here is what they are discovered with:

payload: {"9001":"TRADFRI remote control","9002":1497419274,"9020":1507493476,"9003":65541,"9054":0,"15009":[{"9003":0}],"5750":0,"9019":1,"3":{"0":"IKEA of Sweden","1":"TRADFRI remote control","2":"","3":"1.2.214","6":3,"9":60}} 
payload: {"9001":"TRADFRI wireless dimmer","9002":1495312356,"9020":1507525938,"9003":65536,"9054":0,"15009":[{"9003":0}],"5750":0,"9019":0,"3":{"0":"IKEA of Sweden","1":"TRADFRI wireless dimmer","2":"","3":"1.2.248","6":3,"9":74}} 

I also don't see any significant difference apart from the DEVICE_MODEL text. So there is not much choice, is it?

@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

Thanks for testing. I added a suitable check for the DEVICE_MODEL.

@openhab-bot

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/tradfri-remote-support/34695/8

@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

<<<<<<< feature-tradfri-remote-control
 * @author Christoph Weitkamp - Added support for remote controller and motion sensor devices (read-only battery level)
=======
 * @author Andre Fuechsel - fixed the results removal
>>>>>>> master

@afuechsel I definitely need some help with this merge conflict 😓. What should I do? 😄

@afuechsel

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2017

@cweitkamp What an interesting conflict. :) Looks like the only one. I leave it up to you to solve the author's comment conflict :D

cweitkamp added some commits Oct 3, 2017

Added support for remote controller and motion sensor devices (read-o…
…nly, battery level)

Restructuring and refactoring of classes
Moved some config classes to internal package
Added property for NTP server

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Distinguish discovered devices between remote control an wireless dimmer
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Removed NTP server features
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

All,

I removed the NTP features in this PR, to get this approach merged soon. I want to make space for other necessary changes.

Reading and updating the NTP server configuration from the gateway is possible using the COAP interface, but currently leads to some not very nice issues. My approach was not good enough. I will keep on working on it in the future.

@kaikreuzer
Copy link
Member

left a comment

Thanks, looks pretty good, just some minor comments.

| Colour Temperature Light | 0x0220 | 0220 |
| Extended Colour Light | 0x0210 | 0210 |
| Occupancy Sensor Unit | 0x0107 | 0107 |
| Non-Color Control Unit | 0x0820 | 0820 |

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

According to the spec referenced above, the name should be "Non-Colour Controller"

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Nov 3, 2017

Author Contributor

In relation to the above linked document we maybe can use "Dimmer Switch" (0x0104) for this device as well.

| Dimmable Light | 0x0100 | 0100 |
| Colour Temperature Light | 0x0220 | 0220 |
| Extended Colour Light | 0x0210 | 0210 |
| Occupancy Sensor Unit | 0x0107 | 0107 |

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

Where did you get the 0x0107 from? I cannot find it in the spec referenced above.

This comment has been minimized.

Copy link
@cweitkamp

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

Might be worth to update the link above then as well!

| Extended Colour Light | 0x0210 | 0210 |
| Occupancy Sensor Unit | 0x0107 | 0107 |
| Non-Color Control Unit | 0x0820 | 0820 |
| Non-Color Scene Control Unit | 0x0830 | 0830 |

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

-> "Non-Colour Scene Controller"

return;
}

logger.warn("The controller is a read-only device and cannot handle commands.");

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

reduce to debug

return;
}

logger.warn("The sensor is a read-only device and cannot handle commands.");

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

reduce to debug

super.bridgeStatusChanged(bridgeStatusInfo);

if (bridgeStatusInfo.getStatus() == ThingStatus.ONLINE) {
scheduler.schedule(() -> {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

you can use "submit" instead

@@ -105,6 +112,19 @@ public void onUpdate(String instanceId, JsonObject data) {
thingType = THING_TYPE_DIMMABLE_LIGHT;
}
thingId = new ThingUID(thingType, bridge, Integer.toString(id));
// } else if (TYPE_SWITCH.equals(type) && data.has(SWITCH)) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

This can be removed as you are handling it below, right?

Applied changes from review
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
<bridge-type-ref id="gateway" />
</supported-bridge-type-refs>

<label>Occupancy Sensor Unit</label>

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Nov 3, 2017

Member

Could you please also update the Thing types to match the README? (remove "Unit" on the new types)

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Nov 3, 2017

Author Contributor

Oh, yes. Done.

Applied changes from review
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@kaikreuzer
Copy link
Member

left a comment

Thanks!

@kaikreuzer kaikreuzer merged commit 179b202 into eclipse:master Nov 3, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ip-validation
Details

@cweitkamp cweitkamp deleted the cweitkamp:feature-tradfri-remote-control branch Nov 3, 2017

@phelicksen

This comment has been minimized.

Copy link

commented Nov 3, 2017

Good job, been looking forward to this! Thanks!

@ivivanov-bg
Copy link
Contributor

left a comment

Any particular reason the TradfriGatewayConfig.java to be moved to the internal package ?

I think it should be publicly accessible as it contains constants directly related to the Gateway thing type.

@cweitkamp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2017

Sounds reasonable, but we should move those public constants into TradfriBindingConstants.java and leave the TradfriGatewayConfig.java where it is - in an internal package.

ivivanov-bg added a commit to ivivanov-bg/smarthome that referenced this pull request Nov 9, 2017

[Tradfri] Added support for remote controller and motion sensor devic…
…es (eclipse#4373)

* Added support for remote controller and motion sensor devices (read-only, battery level)
* Restructuring and refactoring of classes
* Moved some config classes to internal package

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

ivivanov-bg added a commit to ivivanov-bg/smarthome that referenced this pull request Nov 10, 2017

<QIVICON> [Tradfri] Adapt binding to use DTLS identities eclipse#4479
Added logging for endpoint destruction (eclipse#4489)

Signed-off-by: Kai Kreuzer <kai@openhab.org>
[Tradfri] Added support for remote controller and motion sensor devices (eclipse#4373)

* Added support for remote controller and motion sensor devices (read-only, battery level)
* Restructuring and refactoring of classes
* Moved some config classes to internal package

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.