Skip to content
This repository has been archived by the owner. It is now read-only.

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

Merged
merged 5 commits into from Nov 3, 2017
Merged

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

merged 5 commits into from Nov 3, 2017

Conversation

@cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp 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
Copy link
Contributor

@hreichert hreichert 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
Copy link

@S0urceror S0urceror 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
Copy link
Contributor Author

@cweitkamp cweitkamp 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.

@sjsf
Copy link
Contributor

@sjsf sjsf 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
Copy link
Contributor Author

@cweitkamp cweitkamp 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?

@sjsf
Copy link
Contributor

@sjsf sjsf 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
Copy link
Contributor Author

@cweitkamp cweitkamp commented Oct 9, 2017

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

@openhab-bot
Copy link
Contributor

@openhab-bot openhab-bot 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
Copy link
Contributor Author

@cweitkamp cweitkamp 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
Copy link
Contributor

@afuechsel afuechsel 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 3 commits Oct 17, 2017
…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>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp
Copy link
Contributor Author

@cweitkamp cweitkamp 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.

Copy link
Contributor

@kaikreuzer kaikreuzer 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 |
Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

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

Copy link
Contributor Author

@cweitkamp cweitkamp Nov 3, 2017

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 |
Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

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

Copy link
Contributor Author

@cweitkamp cweitkamp Nov 3, 2017

Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

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 |
Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

-> "Non-Colour Scene Controller"

return;
}

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

@kaikreuzer kaikreuzer Nov 3, 2017

reduce to debug

return;
}

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

@kaikreuzer kaikreuzer Nov 3, 2017

reduce to debug

super.bridgeStatusChanged(bridgeStatusInfo);

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

@kaikreuzer kaikreuzer Nov 3, 2017

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)) {
Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

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

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

<label>Occupancy Sensor Unit</label>
Copy link
Contributor

@kaikreuzer kaikreuzer Nov 3, 2017

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

Copy link
Contributor Author

@cweitkamp cweitkamp Nov 3, 2017

Oh, yes. Done.

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

@kaikreuzer kaikreuzer left a comment

Thanks!

@kaikreuzer kaikreuzer merged commit 179b202 into eclipse-archived:master Nov 3, 2017
1 of 2 checks passed
@cweitkamp cweitkamp deleted the feature-tradfri-remote-control branch Nov 3, 2017
@felixhall
Copy link

@felixhall felixhall commented Nov 3, 2017

Good job, been looking forward to this! Thanks!

Copy link
Contributor

@ivivanov-bg ivivanov-bg 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
Copy link
Contributor Author

@cweitkamp cweitkamp 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.

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants