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

Convert Bosch BTH-RA to modernExtend #7498

Merged
merged 37 commits into from
May 10, 2024
Merged

Conversation

burmistrzak
Copy link
Contributor

@burmistrzak burmistrzak commented May 8, 2024

Reimplementation of the Bosch TRV using modernExtend, and includes the following additional changes:

  • Add Bosch-specific operating_mode
  • Add genPollCtrl cluster
  • Expose battery_low
  • Remove off & auto from system_mode

Even though the TRV supports a cooling mode (yup..), it's currently not implemented here.

⚠️ Contains breaking changes for some setups!

ℹ️ Fresh off the press, may require hotfixes.

@burmistrzak
Copy link
Contributor Author

@Koenkk What should we do regarding system_mode? The TRV does not support off or auto, my tests and packet captures seem to confirm this. And according to ZCL spec, auto would suggest the device is capable of switching between a heating and cooling mode.

Interestingly enough, it does support cool for system_mode, as well as thermostat_occupied_cooling_setpoint???

@burmistrzak burmistrzak changed the title Convert Bosch BTH_RA to modernExtend Convert Bosch BTH-RA to modernExtend May 8, 2024
@Koenkk
Copy link
Owner

Koenkk commented May 8, 2024

What should we do regarding system_mode?

What can we do here? I guess add the system modes which the device supports?

@burmistrzak
Copy link
Contributor Author

What should we do regarding system_mode?

What can we do here? I guess add the system modes which the device supports?

I agree. This mean off and auto are gone for good. #6995
However, good news for folks with Home Assistant: I've added the Bosch-specific operating_mode (schedule | manual | pause) here, so they can use this new attribute as a drop-in replacement for their system_mode mappings.

@burmistrzak burmistrzak marked this pull request as ready for review May 8, 2024 21:13
@Koenkk
Copy link
Owner

Koenkk commented May 8, 2024

Great, if you can fix the failing CI I can merge this.

src/devices/bosch.ts Outdated Show resolved Hide resolved
@burmistrzak burmistrzak marked this pull request as draft May 9, 2024 04:55
@dierochade
Copy link
Contributor

dierochade commented May 20, 2024

@burmistrzak

I see your point too. Firstly, what speaks for your approach is that the device only publishes heat (and cool) states (0x04 and 0x03). So this is a formal approach.

On the other hand, as you stated yourself, the manufacturer did not comply with the specification. Therefore, I do not see the benefit of trying to comply at all costs, especially if this does not improve the user experience but rather results in a setback: The implementation cannot map the state to the relevant and intended key but to some proprietary attribute.
Consequently, the device just does not support the modes out of the box, and Home Assistant will not be able to read the mode at all.

I see your point, but it's better to use the Bosch-specific....

You stated that this was better, but you did not say why. I just do not see the real benefit besides formal compliance.

I personally could not find the definition that "auto" means auto switch from heat to cool. As far as I was able to see something related, it just states that if the temperature is above the cool setpoint, the temperature shall be considered as above target (line 10116). If no cool setpoint is present, this statement would not be true/without consequences. This would be true at least if there is no cooling setpoint set.

I cannot find anything or anyone speaking against this understanding of "auto." So, I believe this point is valid and would still advocate for the implementation of pause and auto. It would align best with the specs and usability from a general perspective.

Therefore, I kindly ask you to reconsider this decision and revert the change for the benefit of the normal user.

I see that you provide the code and have made all the effort. So it seems natural that eventually, you decide on the way to go – the commit is merged anyway, and I am in no way technically capable of making an alternative PR....

So I tried to go on and implement the new situation in my setup. I made quite an effort (no IT professional here...). Unfortunately, I am facing some difficulties and am unsure how to proceed sensibly. Maybe I am misunderstanding it in the first place:
Maybe I am on the wrong track from the beginning?:

Why would you need to duplicate a device? You'll continue to see the TRV in Home Assistant as a single device, just with no off or auto mode by default, at least for the moment.

Yes, but as I understand it, the device will not work properly (out of the box) because it cannot read/set the operation_mode. So I have a rather big self-made heating control logic that depends on these states to determine if it should use automatic commands (auto/schedule) or manual override from the UI or even from the device itself, very handy (heat/manual). So this is not only (but also, @l3rdy) about the standard climate card not working anymore...
If I want it to work fully, what would be the best approach? I did not find any way to alter the existing device with the template. Is there a way?

So I do not see an option but to manually add it in MQTT. If this could be done in auto discovery, this would really be the BEST! and it would avoid the downsides and keep the new structure and benefits!?

Thank you very much for the template you provided! I am not totally sure where this is meant to be implemented, though.

I did set up an MQTT climate device, mostly derived from the autodiscovery message (without the template so far because I do not run the new converter):
I did setup a mqtt climate device, mostly derived from autodiscovery message (without the template so far, cause I do not run the new converter):

#mqtt.yaml

climate:
  - name: 
    unique_id: "virtual_device_4a"
    action_template: >
      {% set values = {None: None, 'idle': 'idle', 'heat': 'heating', 'cool': 'cooling', 'fan_only': 'fan'} %}
      {{ values[value_json.running_state] }}
    action_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad"
    availability:
      - topic: "zigbee2mqtt/bridge/state"
        value_template: "{{ value_json.state }}"
      - topic: "zigbee2mqtt/Thermostat Bosch 4 Bad/availability"
        value_template: "{{ value_json.state }}"
    availability_mode: "all"
    current_temperature_template: "{{ value_json.local_temperature }}"
    current_temperature_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad"
    device:
      identifiers:
        - "virtual_device_4a"
      manufacturer: "Bosch"
      model: "Radiator thermostat II (BTH-RA)"
      name: "virtual device 4 Bad"      
    max_temp: "30"
    min_temp: "5"
    mode_command_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad/set/system_mode"
    mode_state_template: "{{ value_json.system_mode }}"
    mode_state_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad"
    modes:
      - "off"
      - "heat"
      - "auto"
    temp_step: "0.5"
    temperature_command_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad/set/occupied_heating_setpoint"
    temperature_state_template: "{{ value_json.occupied_heating_setpoint }}"
    temperature_state_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad"
    temperature_unit: "C"
    json_attributes_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad"

This leads to a virtual device that shows mode (old version) and temperature, setpoint, etc. But it does not include the secondary entities.
It just creates a single entity with all additional information as attributes.
So there is no way to control things like Boost, display orientation, brightness, etc., registering them to a given device....

Is there an easy way to do this? Is the template meant to be used in this way?
I still hope it is just a little tweak and I just miss the place to apply it.

If there is no solution by reverting or auto-discovery, any guidance is much appreciated!

@burmistrzak
Copy link
Contributor Author

On the other hand, as you stated yourself, the manufacturer did not comply with the specification. Therefore, I do not see the benefit of trying to comply at all costs, especially if this does not improve the user experience but rather results in a setback: The implementation cannot map the state to the relevant and intended key but to some proprietary attribute. Consequently, the device just does not support the modes out of the box, and Home Assistant will not be able to read the mode at all.

We'll have to unfortunately draw a line somewhere.
Z2M really shouldn't make such decisions, we're just passing on what's coming from Zigbee, that's why operating_mode exists.
We can't just build converters around the specific needs of a single platform (e.g. HA). The idea of Z2M is to translate Zigbee to MQTT as faithfully as possible, regardless of smart home system used, that's why ZCL is king. 👑

I see your point, but it's better to use the Bosch-specific....

You stated that this was better, but you did not say why. I just do not see the real benefit besides formal compliance.

Again, please see above. 👆

I personally could not find the definition that "auto" means auto switch from heat to cool. As far as I was able to see something related, it just states that if the temperature is above the cool setpoint, the temperature shall be considered as above target (line 10116). If no cool setpoint is present, this statement would not be true/without consequences. This would be true at least if there is no cooling setpoint set.
I cannot find anything or anyone speaking against this understanding of "auto." So, I believe this point is valid and would still advocate for the implementation of pause and auto. It would align best with the specs and usability from a general perspective.
Therefore, I kindly ask you to reconsider this decision and revert the change for the benefit of the normal user.

That's your interpretation of the spec, which is fine, but I disagree based upon my knowledge of similar specs and how HVAC modes are handled there.
Quoting the HomeKit Accessory Protocol Specification: "Auto. Turn on heating or cooling to maintain temperature within the heating and cooling threshold of the target temperature."
However, Home Assistant seems to be an outlier, according to their developer docs, they define Auto quite differently.
AFAICT, their HVACMode.HEAT_COOL is analogous to auto in other specs.

I see that you provide the code and have made all the effort. So it seems natural that eventually, you decide on the way to go – the commit is merged anyway, and I am in no way technically capable of making an alternative PR....

I went this way because we have to adhere to something, ZCL in this case. We cannot start implementing converters just to cater to a single ecosystem and its idiosyncrasies. ZHC just isn't the right place for such specific stuff.

So I have a rather big self-made heating control logic that depends on these states to determine if it should use automatic commands (auto/schedule) or manual override from the UI or even from the device itself, very handy (heat/manual).

You can't (at least currently) alter the schedule on the TRV, you know this, right?

If there is no solution by reverting or auto-discovery, any guidance is much appreciated!

Now, what should we do? 🤔
Well, as I explained, we can't alter/fake what/how system_mode is exposed by Z2M, simply to please a single ecosystem.
But, there's the HA integration! Adding a special mapping/fallback to auto-discovery (for just a single device at this point) should nevertheless be possible.
The main Z2M repo seems to be the appropriate place for this.

Thank you very much for the template you provided! I am not totally sure where this is meant to be implemented, though.

Just add them to your mqtt.yaml? Replace the current mode_state_template and change the command topic to: mode_command_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad/set/operating_mode"
FYI, you'll need the latest dev release, tho...

So there is no way to control things like Boost, display orientation, brightness, etc., registering them to a given device....

Well, seems to be a HA integration issue as well? I can't really help with these things here, sorry.

@l3rdy
Copy link

l3rdy commented May 20, 2024

I have just seen I didn't post my response I wrote a few hours ago, but I guess I might as well do it now. 😄

Consequently, the device just does not support the modes out of the box, and Home Assistant will not be able to read the mode at all.

I think there's a big misunderstanding here. Everything should work out of the box of course. (Even in HA)
Otherwise, what really would be the point of this PR right?

Also gotta mention regardless that this Z2MQTT and not Home Assistant. So there might be situations where things (maybe?) could be better for Z2MQTT and not for HA in theory (tho I can't really think of one). There's People who use Z2M and not HA and the other way around.

Should issues occur it should be within Z2M (where most definitely everything works) for HA issues we should switch to the HA community and look at it there, if there's one.

As far as I can tell you and what I've seen, there's not really an Issue in HA. Everything gets exposed and works. Just slightly different. A mod to your cards and/or Automations and you should be good.
Because with this PR in the end the functionality is the same and all the same things are available and exposed just well.. Differently. (as mentioned the Climate Card missing the Mode now but easily controllable with another card etc. but as said functionality is the same)
Also, Auto currently doesn't really work anyway or at least isn't controllable in the current state, so theres actually not really a point using it currently

So of course feel free to test the dev release and keep everyone updated if you run into issues within Z2MQTT.
Otherwise, for HA stuff, let's switch to their community. I'll happily help you with stuff there too :) (feel free to link HA Community link here if you start one)

@burmistrzak
Copy link
Contributor Author

I have just seen I didn't post my response I wrote a few hours ago, but I guess I might as well do it now. 😄

Happens to the best of us. 😅

Consequently, the device just does not support the modes out of the box, and Home Assistant will not be able to read the mode at all.

I think there's a big misunderstanding here. Everything should work out of the box of course. (Even in HA) Otherwise, what really would be the point of this PR right?

Correct.

Also gotta mention regardless that this Z2MQTT and not Home Assistant. So there might be situations where things (maybe?) could be better for Z2MQTT and not for HA in theory (tho I can't really think of one). There's People who use Z2M and not HA and the other way around.

Preach! 🙏
Converters should simply just convert, and not cater to a particular ecosystem. It's up to the smart home system (e.g. Homebridge, HA, etc.) to make the necessary changes to deal with what's coming from Z2M.

I personally don't use HA because another "layer" adds to much latency for my taste. It's Z2M straight to HomeKit, with Node-RED tagged on for automations. HomeKit is more or less just the frontend and handles notifications, scenes, and home/away stuff.

Should issues occur it should be within Z2M (where most definitely everything works) for HA issues we should switch to the HA community and look at it there, if there's one.

Yes, support for Boost Mode, etc. is up to HA.
For the native HA extension, the main Z2M repo is the right place.

As far as I can tell you and what I've seen, there's not really an Issue in HA. Everything gets exposed and works. Just slightly different. A mod to your cards and/or Automations and you should be good. Because with this PR in the end the functionality is the same and all the same things are available and exposed just well.. Differently. (as mentioned the Climate Card missing the Mode now but easily controllable with another card etc. but as said functionality is the same) Also, Auto currently doesn't really work anyway or at least isn't controllable in the current state, so theres actually not really a point using it currently

The key here is currently. 😉

So of course feel free to test the dev release and keep everyone updated if you run into issues within Z2MQTT. Otherwise, for HA stuff, let's switch to their community. I'll happily help you with stuff there too :) (feel free to link HA Community link here if you start one)

✌️

@dierochade
Copy link
Contributor

dierochade commented May 21, 2024

We'll have to unfortunately draw a line somewhere.
Z2M really shouldn't make such decisions, we're just passing on what's coming from Zigbee, that's why operating_mode exists.

As stated before, I can understand your point and can accept the decision. I just want to point out that my point of view is in no way related to home assistant, but only what should be done if the implementation of the manufacturer does not comply to standard. In my view, it then makes little sense to state that zcl is king, cause it was not in the first place. This is clearly true for pause/off, what is an operating mode and will be exposed as heat, whats just not accurate.
This is where other applications (like ha or whoever) will look up the values - imo this interoperability is what specification all is about.
For schedule/auto, this might be different though, after reading your explanation I have to admit that it is more like heat with proprietary addition than auto...

You can't (at least currently) alter the schedule on the TRV, you know this, right?

Please, dont be silly.
I have a sequence in node red that catches the preset changes (It also calls the preset temp every time the trv is switched to auto/scheduled from another mode, not just at the scheduled times) and just revert them. Works good in my experience.

Thank you very much for the template you provided! I am not totally sure where this is meant to be implemented, though.

Just add them to your mqtt.yaml? Replace the current mode_state_template and change the command topic to: mode_command_topic: "zigbee2mqtt/Thermostat Bosch 4 Bad/set/operating_mode" FYI, you'll need the latest dev release, tho...

I did create the device as mqtt template entity and I was just wondering if there is another place just for the template in home assistant/auto discovery to specifically alter this property of the auto discovered device.
I think this means no, then.

Maybe I could write the whole config in mqtt.yaml (additional entities, registering to each device etc), but it will be clumsy, its really much copy and paste, cause scope of variables is too limited. Moreover it probably will just be unmanageable when future changes occur...

Now, what should we do? 🤔

  • Would be much appreciated:

I might be able to tweak the auto-discovery so that's no longer necessary for this specific device, but no promises.

Looking forward to see this:

Yes, support for editing the internal schedule is (hopefully) coming next. 🤞

Thanks for your commitment in trying to improve the device support!

I personally don't use HA because another "layer" adds to much latency for my taste. It's Z2M straight to HomeKit, with Node-RED tagged on for automations.
In my personal experience the noticable part of latency comes from the message relay in zigbee, and the home assistant part is very fast, but if your kinda atomic clock.. :-D

Best regards

@burmistrzak
Copy link
Contributor Author

We'll have to unfortunately draw a line somewhere.
Z2M really shouldn't make such decisions, we're just passing on what's coming from Zigbee, that's why operating_mode exists.

As stated before, I can understand your point and can accept the decision. I just want to point out that my point of view is in no way related to home assistant, but only what should be done if the implementation of the manufacturer does not comply to standard. In my view, it then makes little sense to state that zcl is king, cause it was not in the first place. This is clearly true for pause/off, what is an operating mode and will be exposed as heat, whats just not accurate. This is where other applications (like ha or whoever) will look up the values - imo this interoperability is what specification all is about. For schedule/auto, this might be different though, after reading your explanation I have to admit that it is more like heat with proprietary addition than auto...

Thanks for understanding.
Yes, I see your point regarding pause. Really sucks that Bosch went about it this way... Maybe it's because the TRV is still opening the valve regularly for maintenance?

You can't (at least currently) alter the schedule on the TRV, you know this, right?

Please, dont be silly. I have a sequence in node red that catches the preset changes (It also calls the preset temp every time the trv is switched to auto/scheduled from another mode, not just at the scheduled times) and just revert them. Works good in my experience.

Interesting solution. I had some issues with the manual setpoint being randomly overridden by the schedule. The TRV firmware is still kinda messy.

Now, what should we do? 🤔

  • Would be much appreciated:

Feel free to open a PR on the main Z2M repo, I unfortunately don't have the right setup to develop/verify such a patch. 🙈

I personally don't use HA because another "layer" adds to much latency for my taste. It's Z2M straight to HomeKit, with Node-RED tagged on for automations.
In my personal experience the noticable part of latency comes from the message relay in zigbee, and the home assistant part is very fast, but if your kinda atomic clock.. :-D

Not an atomic clock (yet), but I run a GNSS-based time server. 😉

@dierochade
Copy link
Contributor

dierochade commented May 23, 2024

I tested the dev converter in my setup and it works nice. 😄

Just two minor things:

  • For preconfigured devices (old converter) I have the values "boost" and "boost heating" exposed?
    For a newly paired device (did work as expected :-) I have only "boost heating". "boost" doesnt get updated anymore.
    On first glance this seems like a change in name only in the new converter, but will break old setups?

  • I get:

    "battery_low": null
    Is this intentional or should the value be a boolean?

Implemenation in home assistant:

Feel free to open a PR on the main Z2M repo, I unfortunately don't have the right setup to develop/verify such a patch. 🙈

I do not feel like I was able to accomplish that cause I have to admit my knowledge of these things is just derived from trial and error and thus very limited. I do not understand the structure of the code in the auto-discovery and it would be way to risky to break everything.

However, I checked the documentation and found out that i just can update the rendered values in auto-discovery with some mqtt commands. It seems to work for me, though I have not tested it thorouhgly.

Its in basis just expanding and mapping of some keys, based on these values and templates:

modes: 
	["off", "heat", "auto"]
mode_state_template: 
	{% set values = {'schedule':'auto','manual':'heat','pause':'off'} %}
	{% set value = value_json.operating_mode %}
	{{ values[value] if value in values.keys() else 'off' }}
mode_command_topic: 
	zigbee2mqtt/<friendly name>/set
mode_command_template: 
	{% set values = { 'auto':'schedule','heat':'manual','off':'pause'} %}
	{"operating_mode": "{{ values[value] if value in values.keys() else 'pause' }}"}

And this is the corresponding example flow in node-red for one trv

[{"id":"7dafe2e04952bcd3","type":"mqtt in","z":"d2598a95479ad0a1","d":true,"name":"<friendly name> auto discovery","topic":"homeassistant/climate/<IEEE-address>/climate/config/#","qos":"2","datatype":"auto-detect","broker":"","nl":false,"rap":true,"rh":0,"inputs":0,"x":190,"y":660,"wires":[["37212c8da9adf843"]]},{"id":"2b80c33d334c6f6b","type":"delay","z":"d2598a95479ad0a1","name":"","pauseType":"rate","timeout":"2","timeoutUnits":"seconds","rate":"1","nbRateUnits":"2","rateUnits":"second","randomFirst":"1","randomLast":"5","randomUnits":"seconds","drop":true,"allowrate":false,"outputs":1,"x":640,"y":660,"wires":[["1830a589f11f9c9c"]]},{"id":"37212c8da9adf843","type":"change","z":"d2598a95479ad0a1","name":"","rules":[{"t":"set","p":"payload.modes","pt":"msg","to":"[\"off\",\"heat\",\"auto\"]","tot":"json"},{"t":"set","p":"payload.mode_state_template","pt":"msg","to":"{% set values = {'schedule':'auto','manual':'heat','pause':'off'} %}{% set value = value_json.operating_mode %}{{ values[value] if value in values.keys() else 'off' }}","tot":"str"},{"t":"set","p":"payload.mode_command_topic","pt":"msg","to":"zigbee2mqtt/<friendly name>/set","tot":"str"},{"t":"set","p":"payload.mode_command_template","pt":"msg","to":"{% set values = { 'auto':'schedule','heat':'manual','off':'pause'} %} \t{\"operating_mode\": \"{{ values[value] if value in values.keys() else 'pause' }}\"}","tot":"str"}],"action":"","property":"","from":"","to":"","reg":false,"x":430,"y":660,"wires":[["2b80c33d334c6f6b"]]},{"id":"1830a589f11f9c9c","type":"mqtt out","z":"d2598a95479ad0a1","d":true,"name":"","topic":"","qos":"","retain":"","respTopic":"","contentType":"","userProps":"","correl":"","expiry":"","broker":"fcdd3890675d3f77","x":830,"y":660,"wires":[]},{"id":"fcdd3890675d3f77","type":"mqtt-broker","name":"mqtt node","broker":"192.168.100.56","port":"1883","clientid":"","autoConnect":true,"usetls":false,"protocolVersion":"5","keepalive":"60","cleansession":true,"autoUnsubscribe":true,"birthTopic":"","birthQos":"0","birthPayload":"","birthMsg":{},"closeTopic":"","closeQos":"0","closePayload":"","closeMsg":{},"willTopic":"","willQos":"0","willPayload":"","willMsg":{},"userProps":"","sessionExpiry":""}][{"id":"7dafe2e04952bcd3","type":"mqtt in","z":"d2598a95479ad0a1","d":true,"name":"<friendly name> auto discovery","topic":"homeassistant/climate/<IEEE-address>/climate/config/#","qos":"2","datatype":"auto-detect","broker":"","nl":false,"rap":true,"rh":0,"inputs":0,"x":190,"y":660,"wires":[["37212c8da9adf843"]]},{"id":"2b80c33d334c6f6b","type":"delay","z":"d2598a95479ad0a1","name":"","pauseType":"rate","timeout":"2","timeoutUnits":"seconds","rate":"1","nbRateUnits":"2","rateUnits":"second","randomFirst":"1","randomLast":"5","randomUnits":"seconds","drop":true,"allowrate":false,"outputs":1,"x":640,"y":660,"wires":[["1830a589f11f9c9c"]]},{"id":"37212c8da9adf843","type":"change","z":"d2598a95479ad0a1","name":"","rules":[{"t":"set","p":"payload.modes","pt":"msg","to":"[\"off\",\"heat\",\"auto\"]","tot":"json"},{"t":"set","p":"payload.mode_state_template","pt":"msg","to":"{% set values = {'schedule':'auto','manual':'heat','pause':'off'} %}{% set value = value_json.operating_mode %}{{ values[value] if value in values.keys() else 'off' }}","tot":"str"},{"t":"set","p":"payload.mode_command_topic","pt":"msg","to":"zigbee2mqtt/<friendly name>/set","tot":"str"},{"t":"set","p":"payload.mode_command_template","pt":"msg","to":"{% set values = { 'auto':'schedule','heat':'manual','off':'pause'} %} \t{\"operating_mode\": \"{{ values[value] if value in values.keys() else 'pause' }}\"}","tot":"str"}],"action":"","property":"","from":"","to":"","reg":false,"x":430,"y":660,"wires":[["2b80c33d334c6f6b"]]},{"id":"1830a589f11f9c9c","type":"mqtt out","z":"d2598a95479ad0a1","d":true,"name":"","topic":"","qos":"","retain":"","respTopic":"","contentType":"","userProps":"","correl":"","expiry":"","broker":"fcdd3890675d3f77","x":830,"y":660,"wires":[]},{"id":"fcdd3890675d3f77","type":"mqtt-broker","name":"mqtt node","broker":"192.168.100.56","port":"1883","clientid":"","autoConnect":true,"usetls":false,"protocolVersion":"5","keepalive":"60","cleansession":true,"autoUnsubscribe":true,"birthTopic":"","birthQos":"0","birthPayload":"","birthMsg":{},"closeTopic":"","closeQos":"0","closePayload":"","closeMsg":{},"willTopic":"","willQos":"0","willPayload":"","willMsg":{},"userProps":"","sessionExpiry":""}]

Maybe this will be useful as a blueprint for others

I am willing to do the update on the device documentation, but i am abroad for the coming 10d and this will have to wait.

@burmistrzak
Copy link
Contributor Author

I tested the dev converter in my setup and it works nice. 😄

Just two minor things:

  • For preconfigured devices (old converter) I have the values "boost" and "boost heating" exposed?
    For a newly paired device (did work as expected :-) I have only "boost heating". "boost" doesnt get updated anymore.
    On first glance this seems like a change in name only in the new converter, but will break old setups?

Yep, just a change in name. boost_heating is already used for other devices, and is more descriptive.

  • I get:
    "battery_low": null
    Is this intentional or should the value be a boolean?

Just wait a few hours. It takes some time for the first report to be sent.
I might be able to improve the UX here by reading the current battery level during configuration.

I do not feel like I was able to accomplish that cause I have to admit my knowledge of these things is just derived from trial and error and thus very limited. I do not understand the structure of the code in the auto-discovery and it would be way to risky to break everything.

That's how we learn. 😉

However, I checked the documentation and found out that i just can update the rendered values in auto-discovery with some mqtt commands. It seems to work for me, though I have not tested it thorouhgly.

Looks good! Should work just as well as before.

Maybe this will be useful as a blueprint for others

Thanks for sharing!

I am willing to do the update on the device documentation, but i am abroad for the coming 10d and this will have to wait.

Cool, take your time. 👌

@dierochade
Copy link
Contributor

  • I get:
    "battery_low": null
    Is this intentional or should the value be a boolean?

Just wait a few hours. It takes some time for the first report to be sent.

Unfortunately, I still get the null value.

@burmistrzak
Copy link
Contributor Author

  • I get:
    "battery_low": null
    Is this intentional or should the value be a boolean?

Just wait a few hours. It takes some time for the first report to be sent.

Unfortunately, I still get the null value.

Hmm, can you do me a quick favor and issue a read on batteryAlarmState from genPowerCfg using Dev Console on the device page?
I have a hunch what's going on here, but we first have to verify that your device is actually returning any usable battery data. 👀

@burmistrzak burmistrzak deleted the improve-bth-ra branch May 29, 2024 17:47
@dierochade
Copy link
Contributor

Hmm, can you do me a quick favor and issue a read on batteryAlarmState from genPowerCfg using Dev Console on the device page? I have a hunch what's going on here, but we first have to verify that your device is actually returning any usable battery data. 👀

Sorry for the delay, but i was not at home some days...

I did log the readings on the cluster "powerConfig" → AlarmState. it is not the same on the devices:

Device #2
Info 2024-06-02 12:08:10Read result of 'genPowerCfg': {"batteryAlarmState":0}
"battery": 100,
"battery_low": false,

Device #4
Info 2024-06-02 12:09:51Read result of 'genPowerCfg': {"batteryAlarmState":8}
"battery": 64,
"battery_low": true, (??)

Device #5,
Info 2024-06-02 12:11:59Read result of 'genPowerCfg': {"batteryAlarmState":8}
"battery": 39,
does not expose the battery low key in status?

Device #6,
2024-06-02 12:14:30Read result of 'genPowerCfg': {"batteryAlarmState":8}
"battery": 70,
does not expose the battery low key in status?

@burmistrzak
Copy link
Contributor Author

@dierochade I've removed battery_low after confirming your report myself.
That's probably why some devices still expose it and others don't. Just some cached values from before the update. 😅

We'll have to figure out which devices actually report batteryAlarmState (or comparable in IAS Zone) on their own, without having to setup additional reporting.
See #7239 (comment)

@dierochade
Copy link
Contributor

We'll have to figure out which devices actually report batteryAlarmState (or comparable in IAS Zone) on their own, without having to setup additional reporting.
See #7239 (comment)

@burmistrzak
In my opinion its rather not worth the effort cause we already can read battery state value as percentage and this therefore would provide no real additional information.

It would be of much more interest to be able to edit the builtin schedule. Have you already had the time and inclination to take a look at it?

I did lookup the documentation to for the device to update it, but, that's already done... 😉

One other thing: I did the upgrade in my normal setup and noticed that I had made an error in the template provided ("operating_mode" in command topic and command payload), so it did not work. I did edit the original post and fixed the code.

@burmistrzak
Copy link
Contributor Author

In my opinion its rather not worth the effort cause we already can read battery state value as percentage and this therefore would provide no real additional information.

True, that's why it's gone now. 😉

It would be of much more interest to be able to edit the builtin schedule. Have you already had the time and inclination to take a look at it?

I played around very briefly, but it I'll have to dig through my packet captures to get something that works.
With a bit of luck, we'll have something ready for the next heating period.

One other thing: I did the upgrade in my normal setup and noticed that I had made an error in the template provided ("operating_mode" in command topic and command payload), so it did not work. I did edit the original post and fixed the code.

Great, thanks! 🙏

dierochade added a commit to dierochade/zigbee2mqtt.io that referenced this pull request Jun 8, 2024
Clarified new control of system_mode since [Convert Bosch BTH-RA to modernExtend #7498] (Koenkk/zigbee-herdsman-converters#7498)
dierochade added a commit to dierochade/zigbee2mqtt.io that referenced this pull request Jun 11, 2024
Clarified new control of system_mode since [Convert Bosch BTH-RA to modernExtend #7498] (Koenkk/zigbee-herdsman-converters#7498)

Reworked the commit so that it only affects the 'Notes' section. I hope everything is fine now. Thanks for your patience, I'm still trying to find my way around...
dierochade added a commit to dierochade/zigbee2mqtt.io that referenced this pull request Jun 11, 2024
Clarified new control of system_mode since [Convert Bosch BTH-RA to modernExtend #7498] (Koenkk/zigbee-herdsman-converters#7498)

Reworked the commit so that it only affects the 'Notes' section. I hope everything is fine now. Thanks for your patience, I'm still trying to find my way around...
@gpayer
Copy link

gpayer commented Jun 12, 2024

Just to add my 2 cents, from a technical perspective this change makes perfect sense and needed to be merged.

However as it completely breaks existing setups (Bosch BTH-RA is now simply not usable anymore in HA via the usual service calls), this merge and its release should really have been coordinated with an equivalent change to the auto detection template generation code in the HA integration. Just to say "this must be solved in the main Z2M repo and is therefor not our problem" is very shortsighted.

This breaks a lot of trust, as really no normal user makes a distinction between the converters repo and the main repo. Z2M has a reputation of just working and this is the opposite of that.

During my research yesterday into the reasons why my thermostats are not usable anymore I already stumbled upon a thread with the theme "do not use Z2M, use ZHA, as it just works. Noone needs the extra features of Z2M."

For me personally, I can fix this locally by creating my own device configs inside mqtt for HA to detect, but in the future I will go through each change log of Z2M like a paranoid mad man to avoid issues like this. Downgrading to 1.37 might be another option and to never upgrade again.

Shoutouts to @dierochade for creating the necessary templates, these will make my fixup work much easier!

@burmistrzak
Copy link
Contributor Author

@gpayer I agree that a corresponding change to auto-discovery would have made for a much smoother transition.

Btw. I even had a Z2M fork to implement this, however I wasn't really satisfied with my solution, and abandoned it shortly thereafter. Main reason being, that I didn't want to force a specific mode mapping onto HA users.
My idea was to make auto-discovery for this device configurable, e.g. a legacy or accurate mode.
Unfortunately, I'm still unsure about how to do this properly, so any suggestions and/or contributions are welcome!

Anyone who noticed the change and actually needs the legacy mapping, is free to use the provided template until we find a long-term solution.

Also, you absolutely should read every changelog before updating any software!
That's the reason why it exists.

Lastly, please keep in mind that we're all volunteers, doing this on our own time, for no compensation other than enjoyment. Users of FOSS aren't entitled to anything more than what's stated in the license.
Everything beyond that is community. Give & take. ✌️

@gpayer
Copy link

gpayer commented Jun 13, 2024

@burmistrzak What stopped you from just providing the legacy mapping in the beginning and adding other modes later on? I also do not really understand the usecase for different mappings. Most often you just want your thermostat to work with your existing automations. And for a climate device to be considered a working climate device the service climate.set_hvac_mode simply must work. Otherwise it's no use.

I mean, this is the whole point of the templating functionality in the MQTT integration. And if Z2M provides a auto discovery config for a MQTT HVAC device then this config should actually work. And in this case the provided config is simply wrong. That in of itself is a bug, there can be no discussion about it.

Yes, you could set the operation_mode attribute manually, but then you would have to touch every existing automation regarding climate devices! And what if you have different models from different vendors? That's why you (and every existing third-party blueprint and custom integrations like versatile thermostat) use climate.set_hvac_mode.

And yes, enjoyment is one currency in FOSS. However, especially in big and widely used projects, reputation and trust are currencies as well and I very much assume these are part of the enjoyment.

tl;dr I understand your motivation to make the created mapping more flexible, but I do not understand your opinion that only a tiny minority actually uses climate devices in HA in the default way.

And yes, I'm salty that I had to put in a few hours of work to get everything back on track again. Thankfully it's summer around here, in the winter I would have been royally screwed. I probably would have reverted back to Z2M 1.37 and never upgraded again.

@burmistrzak
Copy link
Contributor Author

What stopped you from just providing the legacy mapping in the beginning and adding other modes later on? I also do not really understand the usecase for different mappings. Most often you just want your thermostat to work with your existing automations. And for a climate device to be considered a working climate device the service climate.set_hvac_mode simply must work. Otherwise it's no use.

Well, it's a Bosch issue. The TRV simply doesn't support auto or off as system_mode.
The custom cluster attribute operating_mode is used instead, and auto-discovery doesn't know how to deal with it yet.
These things take time, man...

And in this case the provided config is simply wrong. That in of itself is a bug, there can be no discussion about it.

We're just passing along what's actually supported by the device.
Also, HA isn't the only ecosystem out there. Other systems handle a single mode just fine.

And yes, enjoyment is one currency in FOSS. However, especially in big and widely used projects, reputation and trust are currencies as well and I very much assume these are part of the enjoyment.

Guess, no currency for me then! 📉

And yes, I'm salty that I had to put in a few hours of work to get everything back on track again.

Not reading release notes and then chirping at contributors is overall a solid way to get things done. 👏👏👏
Why not be constructive and provide some ideas or a draft PR instead of just complaining?

Thankfully it's summer around here, in the winter I would have been royally screwed.

Indeed, very tragic.

I probably would have reverted back to Z2M 1.37 and never upgraded again.

And..?

@gpayer
Copy link

gpayer commented Jun 13, 2024

First of all, I don't want to put down your work. Again, this change was necessary to be able to control this TRV precisely.
Just the fact that the necessary conversion templates were not integrated into homeassistant.js is kind of annoying. I hope you can understand that.

Well, it's a Bosch issue. The TRV simply doesn't support auto or off as system_mode. The custom cluster attribute operating_mode is used instead, and auto-discovery doesn't know how to deal with it yet. These things take time, man...

I know that, I was talking about the home assistant integration, the mqtt payload sent to a specific homeassistant/climate topic and used only by home assistant. The one that takes care of this mismatch by providing the proper templates. An example what has to be changed is actually in this very thread.

I've sent an adapted config manually into a new homeassistant/climate subtopic and got a working climate device this way. Of course it would be much nicer if homeassistant.js would do this automatically.

And in this case the provided config is simply wrong. That in of itself is a bug, there can be no discussion about it.

We're just passing along what's actually supported by the device. Also, HA isn't the only ecosystem out there. Other systems handle a single mode just fine.

Again, I was talking about the home assistant specific config sent to the homeassistant/climate/... topic. There is now a mismatch between this config and the actual data.

I really don't get how the home assistant specific integration (homeassistant.js) is relevant to other ecosystems.

Not reading release notes and then chirping at contributors is overall a solid way to get things done. 👏👏👏 Why not be constructive and provide some ideas or a draft PR instead of just complaining?

I already provided a constructive idea in the actual bug issue. You dismissed it because you want something more complicated. And I simply don't have the time atm to dig into yet another FOSS project. Furthermore I can't develop locally, I don't have a second zigbee usb stick. Is there a way to test with a mockup?

Btw still waiting for the use case of having an "accurate mode" in HA!

@burmistrzak
Copy link
Contributor Author

Just the fact that the necessary conversion templates were not integrated into homeassistant.js is kind of annoying. I hope you can understand that.

I do, but these things can (and will) happen in relatively fast paced projects.
Another reason why reading the release notes is so important!

Well, it's a Bosch issue. The TRV simply doesn't support auto or off as system_mode. The custom cluster attribute operating_mode is used instead, and auto-discovery doesn't know how to deal with it yet. These things take time, man...

I know that, I was talking about the home assistant integration, the mqtt payload sent to a specific homeassistant/climate topic and used only by home assistant. The one that takes care of this mismatch by providing the proper templates. An example what has to be changed is actually in this very thread.

I've sent an adapted config manually into a new homeassistant/climate subtopic and got a working climate device this way. Of course it would be much nicer if homeassistant.js would do this automatically.

Respectfully, no. You don't understand.
Let me explain it one more time:

There're a few things that make auto-discovery & Co. rather tricky:
First, this special workaround (i.e. template) only applies to a single device. We'll have to take this into account when updating the HA integration, so not to break climate control for 99.99% of other users.
Second, operating_mode shouldn't be the default because system_mode is being exposed (albeit only with a single mode).
And lastly, operating_mode is not a native feature of climate, so that's another factor to consider when implementing the workaround.

TL;DR
We can't just put a template into homeassistant.ts, there's more to it.

I already provided a constructive idea in the actual bug issue. You dismissed it because you want something more complicated.

Sigh... See above.

And I simply don't have the time atm to dig into yet another FOSS project.

Well, too bad. Guess you'll have to wait then, or find some time.

Furthermore I can't develop locally, I don't have a second zigbee usb stick. Is there a way to test with a mockup?

You can write e.g. a unit test, and develop against it.
Also, you don't need a separate setup for this kind of issue.

Btw still waiting for the use case of having an "accurate mode" in HA!

...

@mmattel
Copy link
Contributor

mmattel commented Jun 15, 2024

@burmistrzak, first of all, thanks for taking care ❤️ I think that there is a lot of work in the background we cant even imagine...

There are two hearts beating in me. As I am a professional doc writer, I read release notes but I have to admit I did not got the impact of the changes. Thanks to @dierochade for preparing some readme updates to BOSCH-BTH. The culprit of all that is as far I understand that Bosch does not stick to specs which is a shame to a central European, in particular German company.

I think that a lot of things could have been relaxed if the old vs. new situation would have been described in a more user consumable way than in a dev view.

On the other hand side, as a user, I got "zillions" of HA log entries telling WARNING (MainThread) [homeassistant.components.mqtt.climate] Invalid modes mode: off respectively mode: manual I was not able to deal with. By chance, I only have two Bosch BTH devices, it was "relatively" easy to identify that the climate entry belongs to them using the MQTT explorer and finding the threads here.

Reading the posts, I did the following to silence the HA logs: In Zigbee2MQTT (runs as container outside HA), I switched settings back and forth in Operating mode fields respectively clicked on the System mode field on both devices. Now, I have no warning log entries anymore.

@burmistrzak
Copy link
Contributor Author

@mmattel Thank you for your kind words. 🫰
I agree that these changes should have been more clearly communicated in the release notes, e.g. like we did recently with the BMCT-SLZ. So there already have been some lessons learned.

You are correct, Bosch is indeed not following spec. I personally suspect that this is deliberate to limit interoperability, because 99% of features can easily be implemented with standard ZCL, no custom clusters necessary. In fact, it's more effort to do custom stuff! 🥸

Btw. I already have a rough proof-of-concept for a possible solution that, while certainly not perfect, should automatically restore the previous behavior for HA users.

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.

None yet

6 participants