Use "device_id" instead of "slave" in modbus integration #150200

alexandrecuer · 2025-08-07T15:14:50Z

Breaking change

Proposed change

As pymodbus version has been bumped to 3.11.0, slave is not accepted as a argument and has to be replaced ,by device_id

see https://github.com/pymodbus-dev/pymodbus/blob/dev/API_changes.rst

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:
Link to developer documentation pull request:
Link to frontend pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

homeassistant/components/modbus/modbus.py

epenet

Also... tests are failing

home-assistant · 2025-08-07T15:43:55Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

tests/components/modbus/test_init.py

alexandrecuer · 2025-08-08T16:15:12Z

codecoverage is unhappy even i improved the grade adding a new test. Dont know why ?

janiversen · 2025-08-08T16:29:46Z

A good needed fix, it was fixed in my version quite a while ago (device_id have been available since Pymodbus v3.8).

Coverage probably due to the fact that you changed tests, and thus coverage starts counting.

alexandrecuer · 2025-08-08T16:34:04Z

@janiversen : i changed the tests because codecoverage asked me to 😀 so i thought it would be gratefull as i tried to cover the lines pointed as uncovered. But i maybe misunderstood

joostlek · 2025-08-08T16:40:40Z

tests/components/modbus/test_init.py

+async def test_pb_service_return_None(
+    hass: HomeAssistant,
+    do_write,
+    caplog: pytest.LogCaptureFixture,
+    mock_modbus_with_pymodbus,
+) -> None:
+    """Run test when pymodbus returns None."""
+    func_name = {


Okay so what does this actually test? Like it returns None, but what does the user notice from this?

It is called code coverage !!!

No this is touching code, I am asking what it is testing, because okay cool pymodbus returns None, but what else should it return? What behavior do we expect when it returns None or what do we expect when it does not return None?

it should return a modbus message cf https://github.com/pymodbus-dev/pymodbus/blob/9e22049439240a89f159a2aafe65137121201ce7/pymodbus/pdu/pdu.py#L12

not sure it can return None in practise, because if there is no gateway, there is an exception returned
cf https://www.simplymodbus.ca/exceptions.htm

actually it was to cover those lines :

core/homeassistant/components/modbus/modbus.py

Line 398 in bf64e11

if not result:

but I removed this "unfortunate" test

Oh I am not saying it's bad to test this, I think we have to look a bit deeper on what happens when that happens, the function you now link also returns None when this code path is taken. But what will happen if that returns None?

well, I think it never return None in fact, but I am not sure.

janiversen · 2025-08-08T17:00:30Z

@janiversen : i changed the tests because codecoverage asked me to 😀 so i thought it would be gratefull as i tried to cover the lines pointed as uncovered. But i maybe misunderstood

Codecoverage is tricky, you introduced a new attribute, and that changes the coverage calculation.

I believe the whole PR would have caused less problems if you just had changed the text in const.py, and not made a new attribute....it might not be the cleanest way, but since the modbus integration still have a lot of slave references it is also not wrong.

janiversen · 2025-08-08T17:36:25Z

ATTR_* is normally reserved for service call attributes, so the service calls should be adjusted accordingly.

janiversen · 2025-08-08T17:37:29Z

This is by far the easiest solution:

--- a/custom_components/modbus/modbus.py
+++ b/custom_components/modbus/modbus.py
@@ -377,7 +377,7 @@ class ModbusHub:
     ) -> ModbusPDU | None:
         """Call sync. pymodbus."""
         kwargs: dict[str, Any] = (
-            {ATTR_SLAVE: slave} if slave is not None else {ATTR_SLAVE: 1}
+            {"device_id": slave} if slave is not None else {"device_id": 1}
         )
         entry = self._pb_request[use_call]
``

joostlek · 2025-08-08T17:43:22Z

We can also just do DEVICE_ID, because constants are still nice

janiversen · 2025-08-08T17:51:25Z

We can also just do DEVICE_ID, because constants are still nice

Yes I actually did that, but defined the const in modbus.py in order not to "upset" coverage etc.

crug80

Ugly, but needed to have a modbus component working again, bypassing code coverage.

homeassistant/components/modbus/const.py

janiversen · 2025-08-08T21:20:42Z

Changing _ATTR_SLAVE was an ironic joke from my side !!! I am sorry that it was taken seriously !! I honestly thought people changing the code, would look at the consequences.

Clearly changing _ATTR_SLAVE is a VERY breaking change ! every service call needs to be changed !!

The right move it is to make a constant for "device_id", but ONLY for communication with the Pymodbus library, later the service calls etc could be changed, but that is a breaking change, so ABSOLUTELY not for now.

Had it not been for the disapproval of my work as a developer, I had already submitted a PR remedying the problem (which is very serious).

This repo https://github.com/pymodbus-dev/homeassistant_modbus contains a HACS version which works and currently have solved a number of the other outstanding bugs. I expect within a couple of weeks to have solved all outstanding issues, which are not related to wrong configurations.

Tests fixed

…ant#150200)

change ATTR_SLAVE to ATTR_DEVICE_ID

8dd5928

home-assistant bot added bugfix cla-signed integration: modbus small-pr PRs with less than 30 lines. Quality Scale: No score labels Aug 7, 2025

epenet reviewed Aug 7, 2025

View reviewed changes

homeassistant/components/modbus/modbus.py Outdated Show resolved Hide resolved

epenet previously requested changes Aug 7, 2025

View reviewed changes

home-assistant bot marked this pull request as draft August 7, 2025 15:43

Rename slave argument and adjust tests

196fd54

alexandrecuer marked this pull request as ready for review August 7, 2025 19:48

home-assistant bot requested a review from epenet August 7, 2025 19:48

Improve coverage

96821a1

crug80 suggested changes Aug 8, 2025

View reviewed changes

tests/components/modbus/test_init.py Outdated Show resolved Hide resolved

tests/components/modbus/test_init.py Outdated Show resolved Hide resolved

tests/components/modbus/test_init.py Outdated Show resolved Hide resolved

import ATTR_DEVICE_ID in tests to replace hardcoded strings.

615ec0a

joostlek requested changes Aug 8, 2025

View reviewed changes

home-assistant bot marked this pull request as draft August 8, 2025 16:40

rollback and change slave to device_id in const.py

f89e252

crug80 approved these changes Aug 8, 2025

View reviewed changes

joostlek requested changes Aug 8, 2025

View reviewed changes

homeassistant/components/modbus/const.py Outdated Show resolved Hide resolved

alexandrecuer marked this pull request as ready for review August 8, 2025 20:29

home-assistant bot requested review from crug80 and joostlek August 8, 2025 20:29

alexandrecuer marked this pull request as draft August 8, 2025 20:30

Use DEVICE_ID const

8e9b140

joostlek approved these changes Aug 9, 2025

View reviewed changes

alexandrecuer marked this pull request as ready for review August 9, 2025 11:56

home-assistant bot requested a review from joostlek August 9, 2025 11:56

crug80 approved these changes Aug 9, 2025

View reviewed changes

joostlek merged commit 5262cca into home-assistant:dev Aug 10, 2025
48 checks passed

Nippey pushed a commit to Nippey/home-assistant that referenced this pull request Aug 10, 2025

Use "device_id" instead of "slave" in modbus integration (home-assist…

7180b52

…ant#150200)

github-actions bot locked and limited conversation to collaborators Aug 11, 2025

Uh oh!

Use "device_id" instead of "slave" in modbus integration #150200

Use "device_id" instead of "slave" in modbus integration #150200

Uh oh!

Conversation

alexandrecuer commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Uh oh!

epenet left a comment

Choose a reason for hiding this comment

Uh oh!

home-assistant bot commented Aug 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexandrecuer commented Aug 8, 2025

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

alexandrecuer commented Aug 8, 2025

Uh oh!

joostlek Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

janiversen Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

joostlek Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

alexandrecuer Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

joostlek Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

alexandrecuer Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

joostlek commented Aug 8, 2025

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

crug80 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janiversen commented Aug 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexandrecuer commented Aug 7, 2025 •

edited

Loading