smar000 / evoGateway

Python script for listening in and responding to evohome heating control radio messages
46 stars 17 forks source link

Fix "ZoneId" bug, add MQTT autoreconnect feature #19

Closed number42net closed 3 years ago

number42net commented 3 years ago

I ran into some issues running both the 2.0.0 and master branches, mainly caused by part of the code expecting the devices dict to contain zoneId and other parts expecting zone_id. Initially I started fixing these by using the dict.get() method, but that was causing the zones to just turn to 0.

This was causing errors like these:

2020-10-15 14:13:13 |     | ERROR               | ''zoneId'' on line 725 [Command ZONE_TEMPERATURE, data: '---  I --- 01:180412 --:------ 01:180412 30C9 021 0007C901085402078A030BF804073D05073E0608BB', port: 1]
Traceback (most recent call last):
  File "evogateway.py", line 725, in process_received_message
    COMMANDS[msg.command_code](msg)
  File "evogateway.py", line 875, in zone_temperature
    zone_id = devices[msg.source_id]["zoneId"]
KeyError: 'zoneId'

I just noticed that these were also reported in issue #18

Second minor issue is that the BDR91 is being recognized as a controller instead of a relay, presumably because it's often used as a on/off boiler controller.

number42net commented 3 years ago

Also added a fix to automatically resubscribe on reconnect as requested in issue #17

smar000 commented 3 years ago

Thanks @number42net - much appreciated! I will have a quick look through both the commits later in the day before merging.

number42net commented 3 years ago

Here is a copy of my devices.json:

    "01:180412": {
        "name": "Touch screen",
        "zoneMaster": false,
        "zone_id": 7
    },
    "04:092733": {
        "name": "Bed1",
        "zoneMaster": false,
        "zone_id": 3
    },
    "04:092745": {
        "name": "Office",
        "zoneMaster": false,
        "zone_id": 4
    },
    "04:019193": {
        "name": "Bed2",
        "zoneMaster": false,
        "zone_id": 6
    },
    "04:092747": {
        "name": "Living room",
        "zoneMaster": false,
        "zone_id": 1
    },
    "13:163331": {
        "name": "Underfloor relay",
        "zoneMaster": false,
        "zone_id": 7
    },
    "10:137422":{
        "name": "Boiler",
        "zone_id": -1,
        "zoneMaster": false
    },
    "04:092741": {
        "name": "Bed3",
        "zoneMaster": false,
        "zone_id": 2
    },
    "04:092739": {
        "name": "Wardrobe",
        "zoneMaster": false,
        "zone_id": 5
    },
    "18:056026": {
        "name": "EvoGateway",
        "zoneMaster": true,
        "zone_id": 240
    },
    "04:092763": {
        "name": "Faulty",
        "zoneMaster": false,
        "zone_id": -1
    }
}

After going a bit more through the code I realise that probably the problem came from devices.json and devices_new.json using a different tag. My workflow was to let the script discover all my new devices and then just copy and paste the content from devices_new.json into devices.json. And then start making alterations to the names and zone IDs.

It would be great if you could use the same key in both files like I did in my commit, it doesn't really matter which it is, I can just change my devices.json to match.

As for the controller vs relay, if that's what most other apps expect then I'll just have to live with it as well 🙂

number42net commented 3 years ago

I've added two more commits:

The first changes datetime.now() to datetime.utcnow() when publshing time stampts to MQTT. There is a hardcodes Z at the end of the ISO time string, which implies UTC time. But now() returns local time. I ran into this while writing a client that writes the values from MQTT into InfluxDB.

The second is a follow up on issue #17. While restarting my own MQTT broker I noticed that the issue is not just that the subscriptions are not renewed on reconnect, but that there is not attempt to reconnect at all. I found that this is because there is a on_disconnect handler. After removing this handler the script automatically reconnects as expected.

smar000 commented 3 years ago

After going a bit more through the code I realise that probably the problem came from devices.json and devices_new.json using a different tag. My workflow was to let the script discover all my new devices and then just copy and paste the content from devices_new.json into devices.json. And then start making alterations to the names and zone IDs.

It would be great if you could use the same key in both files like I did in my commit, it doesn't really matter which it is, I can just change my devices.json to match.

Thanks for spotting this. On looking back through the evolution of the code, I can see that the key "zone_id" crept in into the automatically detected new devices file when I did some refactoring some months back (it was originally "zoneId", and consistent with the sample devices.json file). The cleanest option is that we keep "zoneId" in the json files and correct the bug introduced in the line writing new devices to file.

I will push an update tomorrow for this.

smar000 commented 3 years ago

I've added two more commits:

The first changes datetime.now() to datetime.utcnow() when publshing time stampts to MQTT. There is a hardcodes Z at the end of the ISO time string, which implies UTC time. But now() returns local time. I ran into this while writing a client that writes the values from MQTT into InfluxDB.

The second is a follow up on issue #17. While restarting my own MQTT broker I noticed that the issue is not just that the subscriptions are not renewed on reconnect, but that there is not attempt to reconnect at all. I found that this is because there is a on_disconnect handler. After removing this handler the script automatically reconnects as expected.

Great, thanks for both of these! I will get them merged tomorrow as well after brief testing.

smar000 commented 3 years ago

The mqtt reconnect and datetime commits have been merged. The "zoneId" matter was addressed separately by correcting the typo in the write line to the devices_new.json file.