Skip to content
This repository has been archived by the owner on May 7, 2020. 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 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

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

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 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

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 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

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

@openhab-bot
Copy link
Contributor

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

<<<<<<< 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

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

…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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> "Non-Colour Scene Controller"

return;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reduce to debug

return;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reduce to debug

super.bridgeStatusChanged(bridgeStatusInfo);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. Done.

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

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kaikreuzer kaikreuzer merged commit 179b202 into eclipse-archived:master Nov 3, 2017
@cweitkamp cweitkamp deleted the feature-tradfri-remote-control branch November 3, 2017 14:33
@felixhall
Copy link

Good job, been looking forward to this! Thanks!

Copy link
Contributor

@ivivanov-bg ivivanov-bg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants