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

#788 improve Eurotronic SPZB0001 support #817

Merged
merged 5 commits into from
Dec 26, 2019
Merged

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Dec 15, 2019

  • switch to one fromZigbee convertor (so we can do the system_mode override)
  • ignore hvacThermostat's system_mode
    • system_mode == heat + eurotronic_system_mode == boost => heat
    • system_mode == heat + eurotronic_system_mode == !boost => auto
    • system_mode == heat + eurotronic_system_mode == window open => off
  • rename fromZigbee.eurotronic_system_mode to fromZigbee.eurotronic_host_flags
  • parse eurotronic_host_flags into a hash with items per flag
  • add toZigbee convertors for eurotronic_host_flags as hash
  • fix eurotronic_system_mode forgets other settings

@allofmex
Copy link
Contributor

rename fromZigbee.eurotronic_system_mode to fromZigbee.eurotronic_host_flags

...will require adjustments in other projects too (like ioBroker). Would you also be ok to keep the name as it is? I agree that host_flags sounds closer to the real meaning, but since this is an already implemented device and users may have created scripts that would need to be changed as well...

@sjorge
Copy link
Contributor Author

sjorge commented Dec 15, 2019

...will require adjustments in other projects too (like ioBroker). Would you also be ok to keep the name as it is? I agree that host_flags sounds closer to the real meaning, but since this is an already implemented device and users may have created scripts that would need to be changed as well...

I'd prefer to rename it, as the idea is to make it return a hash of the bits set (if that works out at least works fine!)

{
  "linkquality": 36,
  "battery": 100,
  "current_heating_setpoint": 22,
  "local_temperature": 22,
  "occupied_heating_setpoint": 22,
  "unoccupied_heating_setpoint": 16,
  "pi_heating_demand": 94,
  "eurotronic_host_flags": {
    "mirror_display": false,
    "boost": false,
    "window_open": false,
    "child_protection": false
  },
  "system_mode": "auto",
  "eurotronic_error_status": 0
}

I wonder if there is a clean way to deprecate things so could keep a eurotronic_system_mode around for one zigbee2mqtt release or something giving people time to move.

Although IIRC this will cause issues with the toZigbee convertors if we have multiple ones, this still needs al of tweaking, as... this device is horribly implemented I just found out...

e.g. set system_mode=off, wait, read system_mode => heat, but if you read the host_flags window open is toggled to true! (but I digress)

@sjorge sjorge force-pushed the spzb0001 branch 2 times, most recently from 312f167 to 39dbdd1 Compare December 15, 2019 17:30
@allofmex
Copy link
Contributor

I'd prefer to rename it, as the idea is to make it return a hash of the bits set

And how about keeping both, eurotronic_system_mode and additionally adding a new eurotronic_host_flags?

"eurotronic_system_mode": 1, // raw bit mask of eurotronic_host_flags (like 0b00000001)
"eurotronic_host_flags": {
    "mirror_display": false,
    "boost": false,
    "window_open": false,
    "child_protection": false
  },

Then you have compatibility and it is only minimum overhead since the encoded value (0b00000001) has to be en/-decoded anyways

@sjorge
Copy link
Contributor Author

sjorge commented Dec 15, 2019

@allofmex done, at least for the fromZigbee side... will do the toZigbee once I figure out how to do the host_flags hash -> bit bits and make that code also handle a 'number' as value type, then the eurotronic_system_mode should be able to use the same convertor. was super easy, just make key an array... need to do some more testing before pushing

@sjorge
Copy link
Contributor Author

sjorge commented Dec 15, 2019

@allofmex @sti0 can you test these changes?

The 'system_mode' should now be correctly mapped, also there is a new 'eurotronic_host_flags' that is a object that can be used to set the bit field in an easy way. e.g.

mosquitto_pub -h localhost -p 1883 -t zigbee2mqtt/bedroom/radiator/set -m '{"eurotronic_host_flags": {"child_protection": true, "mirror_display": true}}'

Will set both the child protection and display mirroring!

There is still one bug in setting 'system_mode', I noticed it while correcting the auto vs heat behavior for system_mode... if you set system_mode, other bits for the host_flags get wiped.

This is because off gets mapped to window_open detection for example and it just sends bit 5 set.

@Koenkk I discovered a bug in the tz.eurotronic_system_mode while updating it... and I do not know how to fix it.

long story short, the real systemMode cluster is always heat... so we ignore it and use the eurotronic host flags specific cluster...

We map heat to the boost bit, off to the window open bit (TRV displays off on display).
Unfortunately this wipes the other set bits, for example child protection.

So we would need to read the current value before modifying it, but I am not sure how to do that in the convertSet code.

- convert bitmask to object (both fromZigbee and toZigbee)
- reintroduce eurotronic_system_mode for backwards compatibility (number)
- update eurotronic_system_mode so it follows heat = boost
@sjorge
Copy link
Contributor Author

sjorge commented Dec 15, 2019

OK, meta.state should have the data I need... will fix that too

@sti0
Copy link
Contributor

sti0 commented Dec 15, 2019

@sjorge great! I'm looking forward to test it within this week. At the moment I got some trouble on my office radiator which must repaired first (not Eurotronic related ;)) I won't like to test it on my childs room ^^

@sjorge
Copy link
Contributor Author

sjorge commented Dec 15, 2019

Alright, I fixed the setting of system_mode.... for our little device this is now entirely based on the host_flags for both reading and writing.

@sjorge sjorge changed the title [WIP] #788 improve Eurotronic SPZB0001 support [TESTING#788 improve Eurotronic SPZB0001 support Dec 15, 2019
@sjorge sjorge changed the title [TESTING#788 improve Eurotronic SPZB0001 support [TESTING] #788 improve Eurotronic SPZB0001 support Dec 15, 2019
@sti0
Copy link
Contributor

sti0 commented Dec 20, 2019

we should set the max reporting interval for the local temperature to 600sec as recommended in the docs.

await configureReporting.thermostatTemperature(endpoint);

@sti0
Copy link
Contributor

sti0 commented Dec 20, 2019

@sjorge I'm testing your implementation right now and switching from off->heat->boost->heat->boost->heat etc. works as expected. Even the pi_heating_demand attribute is reporting in notime now.

But i noticed an issue with the host flags.

Turning on child protection or mirroring the display by publishing

{"eurotronic_host_flags":{"child_protection":"true"}}
or
{"eurotronic_host_flags":{"mirror_display":"true"}}
to
zigbee2mqtt/[device]/set
works pretty good.

But turning it off by publishing the same payload with false won't be recognized and it stays on true.

@sjorge
Copy link
Contributor Author

sjorge commented Dec 21, 2019

But turning it off by publishing the same payload with false won't be recognized and it stays on true.

It should be true and false without a quotes around it.

The string "false" will actually also evaluate to true in javascript/node.

[root@amethyst ~]# mosquitto_pub -h localhost -p 1883 -t zigbee2mqtt/bedroom/radiator/set -m '{"eurotronic_host_flags":{"mirror_display":true}}'
zigbee2mqtt:info  2019-12-21 11:06:24: MQTT publish: topic 'zigbee2mqtt/bedroom/radiator', payload '{"local_temperature_calibration":0,"linkquality":44,"occupied_heating_se│tpoint":16,"local_temperature":15.5,"unoccupied_heating_setpoint":16,"pi_heating_demand":8,"eurotronic_error_status":0,"current_heating_setpoint":16,"eurotronic_host_flags"│:{"mirror_display":true,"boost":false,"window_open":false,"child_protection":false},"system_mode":"auto","eurotronic_system_mode":3,"battery":100}'
[root@amethyst ~]# mosquitto_pub -h localhost -p 1883 -t zigbee2mqtt/bedroom/radiator/set -m '{"eurotronic_host_flags":{"mirror_display":false}}'
zigbee2mqtt:info  2019-12-21 11:07:25: MQTT publish: topic 'zigbee2mqtt/bedroom/radiator', payload '{"local_temperature_calibration":0,"linkquality":44,"occupied_heating_se│tpoint":16,"local_temperature":15.5,"unoccupied_heating_setpoint":16,"pi_heating_demand":8,"eurotronic_error_status":0,"current_heating_setpoint":16,"eurotronic_host_flags"│
:{"mirror_display":false,"boost":false,"window_open":false,"child_protection":false},"system_mode":"auto","eurotronic_system_mode":1,"battery":100}'

we should set the max reporting interval for the local temperature to 600sec as recommended in the docs.
Yes, we should set the reporting to the value recomended in the docs, but I want to do that in a different PR. I want to get this chunk in first.

@sti0
Copy link
Contributor

sti0 commented Dec 21, 2019

I did publish true without quotes first. Thought this didn't work. Will test it later again.

@sjorge
Copy link
Contributor Author

sjorge commented Dec 22, 2019

@sti0 did it work 2nd try round? Would be great to get this in so I can start having a look at the reporting. (Small steps, carrying a few others diffs so want to work the diff against master away first)

Did another full round of testing and it's working as expected now for me.

@sti0
Copy link
Contributor

sti0 commented Dec 22, 2019

@sjorge sorry I was busy yesterday. Tested it with true/false without quotes again and it works as expected. Tested the system_mode setting again and it works, too.

Copy link
Contributor

@sti0 sti0 left a comment

Choose a reason for hiding this comment

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

system_mode setting works as described
setting host flags works as expected

@sjorge sjorge changed the title [TESTING] #788 improve Eurotronic SPZB0001 support #788 improve Eurotronic SPZB0001 support Dec 23, 2019
@sjorge
Copy link
Contributor Author

sjorge commented Dec 23, 2019

@Koenkk this should be ready now

@sjorge
Copy link
Contributor Author

sjorge commented Dec 23, 2019

@Koenkk anychance we can still get this in before Xmas?

@Koenkk
Copy link
Owner

Koenkk commented Dec 26, 2019

Thanks @sjorge !

@Koenkk Koenkk merged commit cdc71e7 into Koenkk:master Dec 26, 2019
@sjorge sjorge deleted the spzb0001 branch December 26, 2019 21:11
@sjorge
Copy link
Contributor Author

sjorge commented Dec 26, 2019

Thanks for merging, I will try to debug some of the other reporting and get (e.g. local_temp) issues tomorrow, will be AFK for a week after tomorrow though.

xmow49 pushed a commit to xmow49/zigbee-herdsman-converters that referenced this pull request Jun 14, 2024
* Keep requests in queue on send failure until expiration

* add missing await

* always send pending reqeuests in fastpoll, no implicit checkin on genPollCtrl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants