Solve modbus binary slave problem #82338
Conversation
home-assistant
bot
added
bugfix
cla-signed
has-tests
integration: modbus
small-pr
|
Hey there @adamchengtkc, @vzahradnik, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
The bulk of the change is to treat result.bits as an array of bools, which matches how pymodbus works.
I think the & 1 is not necessary, but it is okay to use it for consistency between the coil/discrete input cases on one hand and the holding/input register cases on the other hand.
|
@frenck Thanks for merging my modbus PRs getting rid of bugs. Would be real nice if this one could get merged as well and be part of 2022.12. |
| self._attr_is_on = bool(result.bits[0] & 1) | ||
| else: | ||
| self._result = result.registers | ||
| self._attr_is_on = bool(result.registers[0] & 1) |
There was a problem hiding this comment.
These calculations does not match how it's done done the callback below?
There was a problem hiding this comment.
It is actually the same, let me explain:
- the primary sensor gets one of 2 arrays (bits/registers), and sets self._result which is sent to the coordinator, so that the slaves only have 1 array.
- the primary sensor always uses registers/bits index 0, whereas slaves uses index 1-n.
- the calculation itself is identical: bool( & 1)
I hope that explains things.
There was a problem hiding this comment.
Move the is_on calculation outside if the if clause and use self._result as variable instead, then code will be identical in both cases and match slave sensor.
| self._attr_is_on = bool(result.bits[0] & 1) | ||
| else: | ||
| self._result = result.registers | ||
| self._attr_is_on = bool(result.registers[0] & 1) |
There was a problem hiding this comment.
Move the is_on calculation outside if the if clause and use self._result as variable instead, then code will be identical in both cases and match slave sensor.
janiversen
force-pushed
the
issue_77029
branch
from
07e9bed to
5957077
Compare
|
All done and rebased on newest dev. |
|
thanks @elupus |
Breaking change
Proposed change
#77029 reported a problem with the slave option, but actually the problem was deeper.
Corrected binary_sensor, with/without slaves (in general indexing was wrong).
Added tests to secure it all works.
Type of change
Additional information
fixes Modbus Binary slave inputs not working #77029
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: