home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
73.42k stars 30.66k forks source link

Modbus climate won't accept target temperature value #54146

Closed Nicomagno closed 3 years ago

Nicomagno commented 3 years ago

The problem

Climate using a Modbus TCP server fails with a ("error al llamar al servicio climate/set_temperature. required argument is not an integer") when setting a new target temperature. The climate device uses an integer but divided by 10 to have one decimal point, I thought that might be the problem so I deleted the Scale 0.1 and replaced it with scale 1. So I see the target temperature in integer without decimals. But the issue persists

What is version of Home Assistant Core has the issue?

2021.8

What was the last working version of Home Assistant Core?

2021.6

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Modbus Climate

Link to integration documentation on our website

https://www.home-assistant.io/integrations/modbus/

Example YAML snippet

climates:
    - name: Chimenea Horno
      slave: 1
      data_type: int
      count: 1
# I changed scale from 0.1 to 1 but same error persists
      scale: 1
      max_temp: 200
      min_temp: 0
# I changed temp_step from 0.1 to 1 but same error persists
      temp_step: 1
      target_temp_register: 1505
      address: 1213

Anything in the logs that might be useful for us?

Logger: homeassistant.components.homeassistant.triggers.numeric_state
Source: components/homeassistant/triggers/numeric_state.py:115
Integration: Home Assistant Core Integration (documentation, issues)
First occurred: 6:39:45 (2 occurrences)
Last logged: 6:39:45

Error initializing 'Chimenea Horno' trigger: In 'numeric_state' condition: entity climate.chimenea_horno state 'None' cannot be processed as a number
Error initializing 'Chimenea Horno (Duplicate)' trigger: In 'numeric_state' condition: entity climate.chimenea_horno state 'None' cannot be processed as a number

Additional information

No response

probot-home-assistant[bot] commented 3 years ago

modbus documentation modbus source (message by IssueLinks)

probot-home-assistant[bot] commented 3 years ago

Hey there @adamchengtkc, @janiversen, @vzahradnik, mind taking a look at this issue as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks! (message by CodeOwnersMention)

PimDoos commented 3 years ago

Can confirm this issue also exists when using Modbus RTU over TCP. When changing the value on my Modbus Climate thermostat in the frontend, I get this error: image

Config:

- name: EHST20D Tank
  slave: 4
  address: 105
  target_temp_register: 30
  data_type: int
  count: 1
  precision: 1
  scale: 0.01
  max_temp: 55
  min_temp: 45
  temp_step: 0.5
janiversen commented 3 years ago

Both cases are in HA core and not the modbus integration. Seems you are making the service call with a non integer.

Nicomagno commented 3 years ago

I thought that so I changed the Temp step and scale to 1. Might it be a problem with the front end passing a non-integer value or the climate entity? I used the Modbus write service (to the target temp address) and it works fine. Only the Climate integration returns the non-integer error. But this used to work up to 2021.6 even with the 0.1 scale factor. But that shouldn't be a problem since the value passed to the Modbus server should be an int after the conversion. or maybe it is an overzealous data type check.

janiversen commented 3 years ago

There are no indication that this error comes from the modbus integration, in contrary it looks as if the error comes long before. How about changing your setup to write a fixed value.

the error "trigger: In 'numeric_state' condition:" is from before modbus is called.

Apart from that having just controlled the code in modbus, we do not reject calls:

async def async_set_temperature(self, **kwargs):
        """Set new target temperature."""
        if ATTR_TEMPERATURE not in kwargs:
            return
        target_temperature = (
            float(kwargs.get(ATTR_TEMPERATURE)) - self._offset
        ) / self._scale
        if self._data_type in (
            DATA_TYPE_INT16,
            DATA_TYPE_INT32,
            DATA_TYPE_INT64,
            DATA_TYPE_UINT16,
            DATA_TYPE_UINT32,
            DATA_TYPE_UINT64,
        ):
            target_temperature = int(target_temperature)
        as_bytes = struct.pack(self._structure, target_temperature)
        raw_regs = [
            int.from_bytes(as_bytes[i : i + 2], "big")
            for i in range(0, len(as_bytes), 2)
        ]
        registers = self._swap_registers(raw_regs)
        result = await self._hub.async_pymodbus_call(
            self._slave,
            self._target_temperature_register,
            registers,
            CALL_TYPE_WRITE_REGISTERS,
        )
        self._attr_available = result is not None
        await self.async_update()

So I believe the title of this issue is wrong. And changes to the modbus configuration including scale factor will not affect this error.

chodki commented 3 years ago

I have the same bug, in version 2021.7.x everything worked fine ;/

PimDoos commented 3 years ago

When I call the service climate.set_temperature from the devtools with an integer "53" it also returns an error: image

Calling any other entity from another integration in the same way (ex. my airconditioning from the Tuya integration) the change goes through OK. The modbus climate service used to work fine for me up until the 2021.8.0 release.

Debug log:

Logger: homeassistant.components.websocket_api.http.connection
Source: components/modbus/climate.py:126
Integration: Home Assistant WebSocket API (documentation, issues)
First occurred: 14:55:14 (4 occurrences)
Last logged: 19:07:03

[140236675276512] required argument is not an integer
[140236608107136] Error handling message: Unknown error
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/websocket_api/commands.py", line 185, in handle_call_service
    await hass.services.async_call(
  File "/usr/src/homeassistant/homeassistant/core.py", line 1491, in async_call
    task.result()
  File "/usr/src/homeassistant/homeassistant/core.py", line 1526, in _execute_service
    await handler.job.target(service_call)
  File "/usr/src/homeassistant/homeassistant/helpers/entity_component.py", line 213, in handle_service
    await self.hass.helpers.service.entity_service_call(
  File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 658, in entity_service_call
    future.result()  # pop exception if have
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 856, in async_request_call
    await coro
  File "/usr/src/homeassistant/homeassistant/helpers/service.py", line 695, in _handle_entity_call
    await result
  File "/usr/src/homeassistant/homeassistant/components/climate/__init__.py", line 588, in async_service_temperature_set
    await entity.async_set_temperature(**kwargs)
  File "/usr/src/homeassistant/homeassistant/components/modbus/climate.py", line 126, in async_set_temperature
    as_bytes = struct.pack(self._structure, target_temperature)
struct.error: required argument is not an integer

I'll have a look and see if I can dig up an explanation to which piece of code is making this break. My theory is that modbus doesn't cast to an integer due to self._data_type somehow not being an integer. https://github.com/home-assistant/core/blob/13e7cd237eacfa2d9b33e4d225de90236b12882c/homeassistant/components/modbus/climate.py#L117-L125

PimDoos commented 3 years ago

I think I found it. @janiversen shouldn't the list also include these DATA_TYPE constants? https://github.com/home-assistant/core/blob/13e7cd237eacfa2d9b33e4d225de90236b12882c/homeassistant/components/modbus/const.py#L78-L79

janiversen commented 3 years ago

Super de debug log, thanks I will submit a patch later.

janiversen commented 3 years ago

I think I found it. @janiversen shouldn't the list also include these DATA_TYPE constants?

https://github.com/home-assistant/core/blob/13e7cd237eacfa2d9b33e4d225de90236b12882c/homeassistant/components/modbus/const.py#L78-L79

I do not know what you think you found, but that has nothing to do with the error, and are correct code. int and uint are legacy parameters accepted but deprecated (look in validator.py). If you want to solve the problem be my quest, solve it, test it and submit it. Please do not add sugestionas to an issue.

PimDoos commented 3 years ago

Apologies for not following proper procedure. I was trying to understand the code, but you're right this might not be the right place to speculate. I'll look into how to do this properly.

After some more investigation, the issue seems to be a configuration issue. After replacing data_type: int with data_type: int16 in my config it is working properly again. Seeing as @Nicomagno also defined data_type: int in his config, replacing it with data_type: int16 would likely fix the issue.

What I don't understand is why that would cause this issue, as the documentation states they should result in the same behaviour:

int/uint are silently converted to int16/uint16

[Edited this comment as I mistakenly noted the removing data_type would default to int16]

chodki commented 3 years ago

@PimDoos, yes you are right, adding int16 fixes it! :) Thank you!

Nicomagno commented 3 years ago

data_type: int16 does fix my problem, thank you. It didn't occur to me to test data_type: int16. I read the line that said int/uint are silently converted to int16/uint16. I don't remember if that was deprecated in any release along with the recent improvements to Modbus.

janiversen commented 3 years ago

This issue should not have been closed, it still exist! There is a slight difference in the code whether you do not specify 'data_type' or specify 'data_type: int', that is an error and will be patched.

janiversen commented 3 years ago

Added patch that correct the problem, thanks for bringing it to my attention.