Use config.json for configuring sensors. #31

jannylund · 2016-10-20T23:10:01Z

This PR adds the functionality to use config file as documented in #30

"platforms": [
        {
            "name": "TellstickDuo",
            "platform": "Telldus-TD-Tool",
            "sensors": [
                {
                    "id": 114,
                    "model": "temperaturehumidity",
                    "name": "Bathroom"
                },
                {
                    "id": 142,
                    "model": "temperature",
                    "name": "Sauna"
                },
                {
                    "id": 74,
                    "model": "temperature",
                    "name": "Freezer"
                }
            ]
        }
    ]

Leaving out the sensors array or adding an empty array, it will work as before (Except the removed feature override sensortype, since that is done in config file)
Adding a sensors array brings the following features:
- No false sensors will show up.
- Setting the name in config.json ensures it's always the same (correct) name.
- Setting the proper model in config.json allows reconfiguration of false temperaturehumidity as temperature.

Each sensor need to have two mandatory members, id (to identify it uniquely) and model (since we don't know that at this stage).

jannylund · 2016-10-22T15:53:48Z

Updated PR to fix the case where the sensor() method return undefined which renders homebridge dead. This was actually a bug that could happen previously as well, if telldusd was restarted after homebridge.

pkempe · 2016-10-24T19:56:29Z

Using the below config.json, I can't get the filter to work. Sensors which are not listed in the config still show up, e.g. "Thermometer 1", even though the tdtool output for that particular sensor is

type=sensor protocol=mandolyn model=temperaturehumidity id=1 temperature=-50.0 humidity=0 time=2016-10-22 08:01:13 age=222858.

    "platforms": [
    {
     "platform" : "Telldus-TD-Tool",
     "name" : "Telldus-TD-Tool",
         "sensors": [
                {
                    "id": 14,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 21,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 22,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 13,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 238,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 11,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 12,
                    "model": "temperaturehumidity"
                },
                {
                    "id": 23,
                    "model": "temperaturehumidity"
                }
            ]
     }
     ]

PS. Do I really need to clear out the accessories and persist folders, change the MAC and pin of Homebridge in config.json in order to test this? Is there a better way?

jannylund · 2016-10-24T21:35:45Z

@pkempe strange, that should not be the case. With this PR, unnamed sensors should be named "Sensor 14", etc, and nothing related to "Termomether xx" should be visible.

I'm not 100% sure on the need to clear things out of homekit though.

Can you please paste the log you see when starting up homekit with this PR merged?

If this doesn't work, please try installing from https://github.com/jannylund/homebridge-telldus-tdtool/tree/develop (Which is similar, but with all PR's merged)

jannylund · 2016-10-24T21:55:30Z

@pkempe I confirmed there is no need to clear out anything. Just removing one of these instances from your config and restarting homebridge is enough.

You should see something like this in startup:

[10/25/2016, 12:52:06 AM] [TellstickDuo] Found 6 items of type "sensor" from "config.json".
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Bastu'...
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Garderob'...
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Badrum'...
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Utomhus'...
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Garage'...
[10/25/2016, 12:52:06 AM] [TellstickDuo] Initializing platform accessory 'Frysen'...

pkempe · 2016-10-25T04:39:24Z

Here's there log:

Oct 25 06:35:06 rpi homebridge[10776]: [2016-10-25 06:35:06] [Telldus-TD-Tool] Found 12 items of type "sensor".
Oct 25 06:35:06 rpi homebridge[10776]: [2016-10-25 06:35:06] [Telldus-TD-Tool] Model "1A2D" is not supported, try [selflearning-switch, codeswitch, selflearning-dimmer, humidity, temperature, temperaturehumidity]. If you stillhave not found what you're looking for, submit a pull at https://github.com/amlinger/homebridge-telldus-tdtool
Oct 25 06:35:06 rpi homebridge[10776]: [2016-10-25 06:35:06] [Telldus-TD-Tool] Initializing platform accessory 'Utomhusbrytare'...
Oct 25 06:35:06 rpi homebridge[10776]: [2016-10-25 06:35:06] [Telldus-TD-Tool] [Utomhusbrytare]: getServices called
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Brytare 1'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] [Brytare 1]: getServices called
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Brytare 2'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] [Brytare 2]: getServices called
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 177'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 11'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 14'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 12'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 13'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 21'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 22'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 23'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 1'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 43'...
Oct 25 06:35:07 rpi homebridge[10776]: [2016-10-25 06:35:07] [Telldus-TD-Tool] Initializing platform accessory 'Thermometer 141'...
Oct 25 06:35:07 rpi homebridge[10776]: Scan this code with your HomeKit App on your iOS device to pair with Homebridge:

pkempe · 2016-10-25T04:44:00Z

Considering the above, it seems I have not correctly installed the PR properly. Isn't git pull origin pull/31/head); npm run build and then copying the dist folder contents all that's required?

jannylund · 2016-10-25T19:07:10Z

@pkempe yes, I confirmed that it works if I do it that way. (however I run npm install instead, which then calls npm run build.

If you still have issues, maybe try to make a clean clone of this repo. (or simply clone my fork and use the develop branch).

amlinger

I very much like the implementation, although I have a few nit picks.

amlinger · 2016-10-29T19:01:26Z

src/index.js

@@ -32,28 +32,34 @@ class TelldusTDToolPlatform {
    this.log = log
    this.config = config
    this.homebridge = homebridge
+    this.senzors = config["sensors"] || [];


Why not sensors? I'm also not sure why this would need to be a instance variable, when this.devices is not.

devices are coming from the output of tdtool, since they are configured in telldus.conf. The only reason to use senzors instead of sensors is to avoid confusion with the local variable sensors used in the else clause.

I'd really like to avoid this, as sensors can be derived from the config object. Let's keep state to a minimum and rely on the config object instead.

amlinger · 2016-10-29T19:10:21Z

src/index.js

-        sensors.forEach((current, index) => {sensors[index].name = `Thermometer ${current.id}`})
-        return devices.concat(sensors)
-      })
+      if (this.senzors.length > 0) {


I'd rather rely on this.config.sensors.

amlinger · 2016-10-29T19:12:23Z

src/index.js

-        return devices.concat(sensors)
-      })
+      if (this.senzors.length > 0) {
+        this.log(foundOfTypeString('sensor', this.senzors.length, 'config.json'))


As this is user input, we should verify that the fields supplied are 'name' (optional), 'type' and 'id' and nothing else.

As of the other PR, it's also possible to provide maxAge.

I saw that, and if you do not wish to address all these constraints in this PR it's fine, we just need to track it in another issue.

Let's make it another PR as it otherwise needs changes in the other PR as well.

amlinger · 2016-10-29T19:14:52Z

src/index.js

+          }
+        })
+        return devices.concat(this.senzors);
+      } else {


The else clause here seems superfluous, but it's up to you whether you want to keep it or not.

The ideal case to me would be if we could print the path where config.json was read as well, but that might be a bit over the top for an initial implementation.

The else clause is simply there for backwards compatibility, but I agree, it could be removed if you are ok with dropping autoconfigured sensors?

Sorry for the confusion, that's not what I meant. I just meant that

if (this.senzors.length > { // ... } return ...

provides the same functionality as

if (this.senzors.length > { // ... } else { return ... }

Ok, now I understand. Yeah, that could be done, but in this case I think it's more readable to keep the else.

amlinger · 2016-10-29T19:19:23Z

src/lib/telldus-accessory.js

@@ -215,8 +215,12 @@ class TelldusHygrometer extends TelldusAccessory {
  getHumidity(callback, context) {
    this.log('Checking humidity...')
    TDtool.sensor(this.id, this.log).then(s => {
-      this.log(`Found humidity ${s.humidity}%`)
-      callback(null, parseFloat(s.humidity))
+      if (s == undefined) {


I'd like to go with === as standard.

amlinger · 2016-10-29T19:20:59Z

src/lib/telldus-accessory.js

@@ -266,8 +270,12 @@ class TelldusThermometer extends TelldusAccessory {
  getTemperature(callback, context) {
    this.log(`Checking temperature...`)
    TDtool.sensor(this.id, this.log).then(s => {
-      this.log(`Found temperature ${s.temperature}`)
-      callback(null, parseFloat(s.temperature))
+      if (s == undefined) {


I'd like to go with === as standard.

amlinger · 2016-10-29T19:21:05Z

src/lib/telldus-accessory.js

@@ -313,8 +321,12 @@ class TelldusThermometerHygrometer extends TelldusThermometer {
  getHumidity(callback, context) {
    this.log('Checking humidity...')
    TDtool.sensor(this.id, this.log).then(s => {
-      this.log(`Found humidity ${s.humidity}%`)
-      callback(null, parseFloat(s.humidity))
+      if (s == undefined) {


I'd like to go with === as standard.

jannylund · 2016-10-29T20:04:53Z

I think this one is done now. Please merge it if you agree, and I'll rebase #30

amlinger · 2016-10-29T20:17:52Z

Thanks for submitting this, looks like a good implementation and a good interface to me. Tracking the progress on feedback to config fields on #35.

jannylund mentioned this pull request Oct 20, 2016

Prevent sensors from getting stale. #30

Closed

amlinger requested changes Oct 29, 2016

View reviewed changes

Use config.json for configuring sensors.

9b60322

amlinger merged commit 99fe006 into amlinger:master Oct 29, 2016

jannylund deleted the use-config-file-for-sensors branch October 29, 2016 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use config.json for configuring sensors. #31

Use config.json for configuring sensors. #31

jannylund commented Oct 20, 2016

jannylund commented Oct 22, 2016

pkempe commented Oct 24, 2016

jannylund commented Oct 24, 2016

jannylund commented Oct 24, 2016

pkempe commented Oct 25, 2016

pkempe commented Oct 25, 2016

jannylund commented Oct 25, 2016

amlinger left a comment

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

amlinger Oct 29, 2016

jannylund Oct 29, 2016

jannylund commented Oct 29, 2016

amlinger commented Oct 29, 2016

Use config.json for configuring sensors. #31

Use config.json for configuring sensors. #31

Conversation

jannylund commented Oct 20, 2016

jannylund commented Oct 22, 2016

pkempe commented Oct 24, 2016

jannylund commented Oct 24, 2016

jannylund commented Oct 24, 2016

pkempe commented Oct 25, 2016

pkempe commented Oct 25, 2016

jannylund commented Oct 25, 2016

amlinger 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

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

jannylund commented Oct 29, 2016

amlinger commented Oct 29, 2016