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.

Sign up for GitHub

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

Add separate scale for current temp for modbus climates #135848

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
+92 −1
Open

Add separate scale for current temp for modbus climates #135848

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2919e6d
Added separate scale for current temp for climates
illia-piskurov Jan 17, 2025
63f9762
Fix bug
illia-piskurov Jan 22, 2025
f382c68
Added test for scale of current_temp attr
illia-piskurov Jan 31, 2025
e27fef2
fix bug with scale
illia-piskurov Feb 6, 2025
5e0edf5
fix offseting
illia-piskurov Feb 7, 2025
93c63f6
Replace multiline ternary with if-else for better readability
illia-piskurov Feb 10, 2025
6bc6e78
Merge branch 'dev' into separate_scales
illia-piskurov Feb 10, 2025
bcad747
Refactor test name
illia-piskurov Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions homeassistant/components/modbus/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
CONF_BAUDRATE,
CONF_BYTESIZE,
CONF_CLIMATES,
CONF_CURRENT_TEMP_SCALE,
CONF_DATA_TYPE,
CONF_DEVICE_ADDRESS,
CONF_FAN_MODE_AUTO,
Expand Down Expand Up @@ -199,6 +200,7 @@
),
vol.Optional(CONF_STRUCTURE): cv.string,
vol.Optional(CONF_SCALE, default=1): vol.Coerce(float),
vol.Optional(CONF_CURRENT_TEMP_SCALE, default=1): vol.Coerce(float),
vol.Optional(CONF_OFFSET, default=0): vol.Coerce(float),
vol.Optional(CONF_PRECISION): cv.positive_int,
vol.Optional(
Expand Down
13 changes: 12 additions & 1 deletion homeassistant/components/modbus/climate.py
View file
Open in desktop
Copy link

@simonmarwitz simonmarwitz Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am mistaken, but wouldn't it have to be
int((register_value - self._offset ) / self._scale * self._current_temperature_scale)
?
This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is considered good coding practice after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am mistaken, but wouldn't it have to be int((register_value - self._offset ) / self._scale * self._current_temperature_scale) ? This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is consider good coding practice after all.

Yeah, I think you're right. As for the method, I'm willing to do whatever is accepted and considered good practice.

Copy link

Choose a reason for hiding this comment

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

A quick search turned out, that overriding variables should be OK from a coding pratice point of view. I could not find any further guidelines related to the home-assistant coding style. Maybe some experienced developer could comment here? Personally, I this option cleaner. I suspect it should be executed in order even with asyncio (await).

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually current and target temp are 2 different sensors which share 6 different parameters at least: scale, precision, limits (min-max) and offset. In my opinion, the choice is between an ugly change limited to solve the only problem behind this PR (to have a different scale) or to rewrite a little bit the component to make current and target temperature 2 distinct sensors. I asked for an help and probably we can talk also on discord, under the modbus thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am mistaken, but wouldn't it have to be int((register_value - self._offset ) / self._scale * self._current_temperature_scale) ? This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is considered good coding practice after all.

Thanks

Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
CALL_TYPE_WRITE_REGISTER,
CALL_TYPE_WRITE_REGISTERS,
CONF_CLIMATES,
CONF_CURRENT_TEMP_SCALE,
CONF_FAN_MODE_AUTO,
CONF_FAN_MODE_DIFFUSE,
CONF_FAN_MODE_FOCUS,
Expand Down Expand Up @@ -144,6 +145,7 @@ def __init__(
]
self._unit = config[CONF_TEMPERATURE_UNIT]
self._attr_current_temperature = None
self._current_temperature_scale = config[CONF_CURRENT_TEMP_SCALE]
self._attr_target_temperature = None
self._attr_temperature_unit = (
UnitOfTemperature.FAHRENHEIT
Expand Down Expand Up @@ -440,9 +442,18 @@ async def _async_update(self) -> None:
],
)

self._attr_current_temperature = await self._async_read_register(
register_value = await self._async_read_register(
self._input_type, self._address
)
if register_value:
self._attr_current_temperature = int(
(register_value - self._offset)
/ self._scale
* self._current_temperature_scale
)
else:
self._attr_current_temperature = None

# Read the HVAC mode register if defined
if self._hvac_mode_register is not None:
hvac_mode = await self._async_read_register(
Expand Down
1 change: 1 addition & 0 deletions homeassistant/components/modbus/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
CONF_PARITY = "parity"
CONF_PRECISION = "precision"
CONF_SCALE = "scale"
CONF_CURRENT_TEMP_SCALE = "current_temp_scale"
CONF_SLAVE_COUNT = "slave_count"
CONF_STATE_CLOSED = "state_closed"
CONF_STATE_CLOSING = "state_closing"
Expand Down
77 changes: 77 additions & 0 deletions tests/components/modbus/test_climate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from homeassistant.components.homeassistant import SERVICE_UPDATE_ENTITY
from homeassistant.components.modbus.const import (
CONF_CLIMATES,
CONF_CURRENT_TEMP_SCALE,
CONF_DATA_TYPE,
CONF_DEVICE_ADDRESS,
CONF_FAN_MODE_AUTO,
Expand All @@ -62,6 +63,7 @@
CONF_HVAC_ONOFF_REGISTER,
CONF_MAX_TEMP,
CONF_MIN_TEMP,
CONF_SCALE,
CONF_SWING_MODE_REGISTER,
CONF_SWING_MODE_SWING_BOTH,
CONF_SWING_MODE_SWING_HORIZ,
Expand All @@ -80,6 +82,7 @@
ATTR_TEMPERATURE,
CONF_ADDRESS,
CONF_NAME,
CONF_OFFSET,
CONF_PLATFORM,
CONF_SCAN_INTERVAL,
CONF_SLAVE,
Expand Down Expand Up @@ -745,6 +748,80 @@
assert state.state == result


@pytest.mark.parametrize(
("do_config", "result", "register_words"),
[
(
{
CONF_CLIMATES: [
{
CONF_NAME: TEST_ENTITY_NAME,
CONF_TARGET_TEMP: 120,
CONF_ADDRESS: 117,
CONF_SLAVE: 10,
CONF_SCAN_INTERVAL: 0,
CONF_DATA_TYPE: DataType.INT32,
},
]
},
17,
[0, 17],
),
(
{
CONF_CLIMATES: [
{
CONF_NAME: TEST_ENTITY_NAME,
CONF_TARGET_TEMP: 120,
CONF_ADDRESS: 117,
CONF_SLAVE: 10,
CONF_SCAN_INTERVAL: 0,
CONF_CURRENT_TEMP_SCALE: 0.01,
CONF_SCALE: 10,
CONF_OFFSET: 20,
},
]
},
25,
[2520],
),
(
{
CONF_CLIMATES: [
{
CONF_NAME: TEST_ENTITY_NAME,
CONF_TARGET_TEMP: 120,
CONF_ADDRESS: 117,
CONF_SLAVE: 10,
CONF_SCAN_INTERVAL: 0,
CONF_CURRENT_TEMP_SCALE: 0.01,
CONF_SCALE: 10,
},
]
},
19,
[1900],
),
],
)
async def test_hvac_onoff_coil_update(

Check failure on line 807 in tests/components/modbus/test_climate.py

View workflow job for this annotation

GitHub Actions / Check ruff

Ruff (F811) 

tests/components/modbus/test_climate.py:807:11: F811 Redefinition of unused `test_hvac_onoff_coil_update` from line 732
hass: HomeAssistant, mock_modbus_ha, result, register_words
) -> None:
"""Test climate update based on On/Off coil values."""
mock_modbus_ha.read_holding_registers.return_value = ReadResult(register_words)

await hass.services.async_call(
HOMEASSISTANT_DOMAIN,
SERVICE_UPDATE_ENTITY,
{ATTR_ENTITY_ID: ENTITY_ID},
blocking=True,
)
await hass.async_block_till_done()

state = hass.states.get(ENTITY_ID)
assert state.attributes.get("current_temperature") == result


@pytest.mark.parametrize(
("do_config", "result", "register_words"),
[
Expand Down
Loading