Modbus bit_switch and bit_sensor implementation#47240
Modbus bit_switch and bit_sensor implementation#47240yury-sannikov wants to merge 1 commit intohome-assistant:devfrom
Conversation
|
Hey there @adamchengtkc, @janiversen, @vzahradnik, mind taking a look at this pull request as its been labeled with an integration ( |
|
cc @nagyrobi |
|
Look good from a first view, I will go into detail a bit later |
|
Cool, then I will start working on the documentation PR. lmk if you feel we can use better names |
|
Remark I have an open PR for documentation as well, yours should extend my PR. |
|
Documentation PR has been added based on the home-assistant/home-assistant.io#16678 PR |
|
Hi I wanted to do a proper review, but for some reason I cannot exclude my PR so I review only your changes. As soon as my PR is merged (just waiting for a maintainer to do it), then I swiftly review your PR. I have downloaded it too, and it works on my Mac. |
janiversen
left a comment
There was a problem hiding this comment.
Your changes in sensor/switch needs some more thinking. I like the idea of a base class.
|
|
||
|
|
||
| @lru_cache(maxsize=32) | ||
| def _read_cached(client, what, unit, address, count, ttl_bucket): |
There was a problem hiding this comment.
I really do not see the need for this added complexity, it does not seem to make your bit implementation easier, and it complicates the rest. I suggest to remove the lru_cache part (unless you have some good arguments, why not).
There was a problem hiding this comment.
If I have a configuration where each sensor/switch has the same address (in the case below it's 514) I have N read calls of the same Modbus register to the Modbus Client. If I have 16 sensors attached to the same address, the integration will end up with 16 reads of the same value through the Modbus.
This read cache holds the first read value for one second. This helps avoid making sequential calls. We can remove the cache, however, then we will be making the same read calls multiple times. This is not a big deal, however, may slow down updates.
The cache protects from multiple reads only during one update cycle and should be empty on the next cycle, which ensures by the 1 second expiration time.
bit_sensors:
- name: pwrmain_main_feed_under_over_voltage
slave: 16
address: 514
bit_number: 1
- name: pwrmain_controller_fault_watchdog_took_over
slave: 16
address: 514
bit_number: 2
- name: pwrmain_main_feed_phase_order_error
slave: 16
address: 514
bit_number: 6
Since Modbus device is a black box, we can't assume that the cache will be valid for a longer period of time. That's why I picked one second.
Also, any write to the Modbus invalidates the cache. This might be unnecessary though because the next read cycle will start not earlier than a second later and the cache won't hit anyway.
There was a problem hiding this comment.
Your use case is a bit different than the normal ones, normally you would just read the 16bits in one go, and then split them. What I do not like here is that you force the cache on all read, and I am not so sure it is a good idea.
What you could easily do is make a bit_sensor.py, and in that file have special handling for the update call. E.g. you group the sensors (according to the bit pattern), the update call to the first sensor reads physically and either stores the value for the other sensors or updates all sensors. Update call to all other sensors are dummies.
There was a problem hiding this comment.
Good idea. Let me think about how to do that. I will revert _read_cached changes
| read_result = ( | ||
| ReadResult(register_words) | ||
| if register_words | ||
| else ModbusException("Modbus error") |
There was a problem hiding this comment.
you are testing the test setup ?
There was a problem hiding this comment.
Nope, I just want to emulate a disconnected state, when the read_* call throws an exception. I found a bug in my code and decided to add a test for that as well.
There was a problem hiding this comment.
Ahhh that is a good idea, I like that.
janiversen
left a comment
There was a problem hiding this comment.
Second part of review. I have downloaded your PR, to isolate your changes, making it a bit harder to review, but I wanted to give you some feedback.
| ), mock.patch( | ||
| "homeassistant.components.modbus.modbus.ModbusUdpClient", return_value=mock_sync | ||
| ), mock.patch( | ||
| "homeassistant.components.modbus.modbus._read_cached", return_value=read_result |
There was a problem hiding this comment.
But your cache is in our source, I am patching pymodbus, because I want to test the code we have in the read functions.
There was a problem hiding this comment.
hm, not sure why you need to patch pymodbus to make it work. read_cached basically wraps all read* calls, that's why I removed the code with the ReadResult below.
| check_config_only=False, | ||
| config_modbus=None, | ||
| scan_interval=None, | ||
| **kvargs, |
There was a problem hiding this comment.
I like that you have added test for write, but it should be done properly, with named parameters.
There was a problem hiding this comment.
Ok, make sense
| entity_id = f"{entity_domain}.{device_name}" | ||
|
|
||
| # Call an arbitrary service | ||
| if "call_service" in kvargs: |
There was a problem hiding this comment.
Looks like you are allowing both a read and a write test in the same call, the idea of this function is one call pr test.
There was a problem hiding this comment.
Yeah, this test design might be confusing. Let me rethink it based on the cache decision
| # check write_register call | ||
| if "write_register" in kvargs: | ||
| args, kvargs = kvargs["write_register"] | ||
| mock_sync.write_register.assert_called_once_with(*args, **kvargs) |
There was a problem hiding this comment.
We should check that the values sent to pymodbus (bytes) correspond the to the parameters in the original call (which could be e.g. STATE_ON).
| STATE_OFF, | ||
| SERVICE_TURN_ON, | ||
| # 0x40 yields OFF state due to the status bit 5 | ||
| # however, SERVICE_TURN_ON enables bit 6 which make bit untouched |
There was a problem hiding this comment.
yeah, except 0x40 and 0x41, lol
|
@janiversen thank you for the review. I will make changes a bit later this week. The main takeaways are:
|
|
I think those are the big thing, once that is done I will look at it all again Your idea of having a base sensor class is very good, please keep that. |
d84828a to
e0eb8d0
Compare
|
Hi @janiversen. I think I addressed all the issues except extracting bit_sensor and bit_switch into their own files. That will require extracting the base classes into their own separate files which may increase the complexity of understanding the code. |
|
ok, I will take a look, but it might be a couple of days until I get some free time |
|
The modbus integration have changed on dev, so you need to do:
|
b2bd5e9 to
1ec7bb4
Compare
| ) | ||
| except ConnectionException: | ||
| self._available = False | ||
| self.schedule_update_ha_state() |
There was a problem hiding this comment.
w/o this, the state would be STATE_UNKNOWN instead of STATE_UNAVAILABLE. Added tests as well.
There was a problem hiding this comment.
Please be aware your PR is about bit_sensor, so please only add changes to that respect.
| ): | ||
| """Initialize the modbus bit sensor.""" | ||
| super().__init__( | ||
| ModbusReadCache(hub), |
There was a problem hiding this comment.
Only bit switch/sensor uses read cache as a wrapper. Regular sensors/switches would not use read cache.
There was a problem hiding this comment.
OK. Before you start doing changes, please make sure your branch is rebased against dev, so that this PR only contain your changes (also no merge commits), in order to facilitate review.
Regarding the cache, we are currently thinking about a different more general way. You instanciate an object of each bit_sensor configured, but an alternative it is modbus.py to have 2 classes one with the real read and one with a virtual read (reusing the first read). That way you would instanciate the first bit_sensor with the real class (reading modbus) and the following with the second class. The reason we are thinking of this, is that we often see a configuration that read address, +1, +2 ..... and that like your bit_sensor is overload.
janiversen
left a comment
There was a problem hiding this comment.
You have a ton of good code, but there are 2 general issues:
- it seems (maybe wrongly) that you have moved code around, that is tricky
- a number of your changes are not related to bit_sensor/bit_switch
Avoiding those 2 would make review a lot easier and you would have approval a lot faster.
| vol.Optional(CONF_STATE_OFF): cv.positive_int, | ||
| vol.Optional(CONF_STATE_ON): cv.positive_int, | ||
| vol.Optional(CONF_VERIFY_REGISTER): cv.positive_int, | ||
| vol.Optional(CONF_VERIFY_STATE, default=True): cv.boolean, |
There was a problem hiding this comment.
Why are you adding this, is that needed for your bit_sensor implementation ?
There was a problem hiding this comment.
Yep, I will remove it
| @@ -0,0 +1,26 @@ | |||
| """Read cache wrapper for the Modbus Hub methods.""" | |||
There was a problem hiding this comment.
This is something local to bit_sensor / bit_switch so at least the file name is wrong. I would prefer you put this content in bit_sensor, and imported it in bit_switch.
| CALL_TYPE_REGISTER_HOLDING, | ||
| CALL_TYPE_REGISTER_INPUT, | ||
| CONF_BIT_NUMBER, | ||
| CONF_BIT_SENSORS, |
There was a problem hiding this comment.
This seems to belong in bit_sensor.py
| config = None | ||
|
|
||
| for entry in discovery_info[CONF_SENSORS]: | ||
| for entry in discovery_info.get(CONF_BIT_SENSORS, []): |
There was a problem hiding this comment.
Please do not mix sensor with bit_sensor !
There was a problem hiding this comment.
Please see my comment https://github.com/home-assistant/core/pull/47240/files#r604390837
|
|
||
| class ModbusRegisterSensor(RestoreEntity, SensorEntity): | ||
| """Modbus register sensor.""" | ||
| class ModbusSensorBase(RestoreEntity, SensorEntity): |
There was a problem hiding this comment.
Are you sure that we should not have 2 base on for register and one for coil ?
| sensor_name = "test_sensor" | ||
| config_sensor = {CONF_NAME: sensor_name, CONF_ADDRESS: 51, CONF_BIT_NUMBER: 5} | ||
| if do_options: | ||
| config_sensor.update( |
There was a problem hiding this comment.
Please see in the other test files. if/else in the test cases are to be avoided
There was a problem hiding this comment.
There was a problem hiding this comment.
That is old standard, as I noted somewhere else...when you present new code, maintainers look with the present standards. I was asked to remove specifically the “if do_options” in my last PR (and it do make sense). This is a good example why not to move old code.
When I get time, I will change all tests to follow this new pattern.
There was a problem hiding this comment.
Oh, cool. Sorry, that's my first PR to the HA, I might not know all the rules
| array_type = None | ||
| device_config[CONF_ADDRESS] = 1234 | ||
|
|
||
| if do_options: |
| @@ -0,0 +1,91 @@ | |||
| """Test modbus read cache.""" | |||
There was a problem hiding this comment.
This should be part of the bit_sensor / bit_switch test, we do not want to caching generally.
|
Will this affect the coils as switches in any aspect? |
|
hi @janiversen |
|
you are basically missing to update modbus.py (the loop that calls all platforms under modbus). I have added some review comments. |
|
Thanks for pointing out it. I added suggested changes to the test PR and I'm getting the error I had before: This happens when I add If I do That's why I think there are only 2 ways to hook into:
I'm looking for the third approach, which I'm struggling to find out. |
|
have a look at pr #48558 a fan is also no standard. |
|
@janiversen, unfortunately, a fan is a standard domain https://github.com/home-assistant/core/blob/dev/homeassistant/components/fan/manifest.json |
|
Any suggestions on how can I go ahead with this PR for the non-standard domain (senor and switch)? |
|
For a non-standard domain you need to ask in the developer forum or use discord. But I still do not see why you continue trying to make a non-standard domain. What you try to do looks like a specialised version of binary_sensor and switch (as I have written earlier). If you cannot trix HA to call you directly (load_component) then 1 single "if" in setup() of binary_sensor.py and switch.py is all that are needed to call you setup and do what you like (well and of course import a function from your file(s)), no need to make things more complicated. One of the problems I have had reviewing your PR is all your changes in the existing code, causing me to wonder why they are need in order to add a specialised binary_sensor/switch. A clear implementation without all the other stuff is easier to review and easier to guide you on. Apart from that, your changes in existing code are in conflict with dev, please rebase without merge. |
|
Specific example: from <yourfile> import <your setup func>
def async_setup_platform(
...
for entry in ......
if <your_key> in entry
<your setup func>
returnyou can do it this simple, and without touching other parts of the existing code. |
|
Oh, cool, that makes sense now. I thought I'm not allowed to do any changes in the binary_sensor.py and switch.py. If I can hook into it like #47240 (comment) I think I'm good now. Thank you! |
7026a65 to
9952081
Compare
|
are you aware that you have changes in 12 files? that sound like overkill for what you want to implement. may I suggest you do “git rebase” and squash the commit so you have only a few where you easier check which files are changed. When your PR get merged the maintainer will squash all commits into one, but keep the commit message from every commit. |
43d23c3 to
e95cb0b
Compare
|
Squashed into one commit. I would appreciate it if you can guide me on how can I reduce the number of changed files. I might be missing something. |
|
If you say it is all needed changes, then you cannot reduce the number and at least you have removed 2 files. It looks to as if your changes to conftest.py does not really matter. I see you are still using "sensor" as base, but as I understand it bit_sensor can return true/false, that is the definition of binary_sensor. I wonder if you will get problems with Lovelace when forcing a sensor (not binary_sensor) to only be true/false. I see you have introduced your own setup which is good, but I think your code have an unintended sideeffect. For a bit_sensor your setup is called to add the sensor, but then the sensor code continues with the same discovery_info, meaning it is added a second time. (this was just a quick look, I might have overlooked something). |
Right, forgot about you mentioned it earlier. Let me fix it a bit later.
|
5df256f to
c9a3b6f
Compare
|
Hi @janiversen |
|
2 general items:
|
janiversen
left a comment
There was a problem hiding this comment.
I have not looked too closely at your test cases, it seems to me you are redefining the test harness we use for other platforms, and that makes the code a lot harder to read.
| config = None | ||
|
|
||
| for entry in discovery_info[CONF_BINARY_SENSORS]: | ||
| sensors.extend(setup_bit_sensors(hass, discovery_info)) |
There was a problem hiding this comment.
This needs at very least a comment. Reading this code, it looks as if you first extend sensors with bit-sensors and the continue with the normal sensors.
| for entry in discovery_info[CONF_BINARY_SENSORS]: | ||
| sensors.extend(setup_bit_sensors(hass, discovery_info)) | ||
|
|
||
| for entry in discovery_info.get(CONF_BINARY_SENSORS, []): |
There was a problem hiding this comment.
Why change to info.get ? That is not a needed change.
| import logging | ||
| import time | ||
|
|
||
| from pymodbus.exceptions import ConnectionException, ModbusException |
There was a problem hiding this comment.
There are no need to check for ConnectionException it is also a ModbusException.
| import time | ||
|
|
||
| from pymodbus.exceptions import ConnectionException, ModbusException | ||
| from pymodbus.pdu import ExceptionResponse |
There was a problem hiding this comment.
You only need to check for ModbusException. Which btw is something which are currently being removed in all platforms, I have a PR pending merge for that.
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @lru_cache(maxsize=32) |
There was a problem hiding this comment.
If you limit to 32, then you should also check that no more than 32 are used.
| array_name_discovery, | ||
| array_name_old_config, | ||
| register_words, | ||
| register_words_or_exception, |
There was a problem hiding this comment.
Please do not change the test methods in a PR where you implement new functionality. In case of problems it is quite hard to determine whether it is the new functionaliy or the test changes.
| } | ||
|
|
||
| mock_sync = mock.MagicMock() | ||
| # Setup inputs for the sensor |
There was a problem hiding this comment.
?? This is not just for a sensor.
| register_words_or_exception | ||
| if isinstance(register_words_or_exception, Exception) | ||
| else ReadResult(register_words_or_exception) | ||
| ) |
There was a problem hiding this comment.
This is a good idea, but we do not want to test pymodbus in every platform, that is the responsibiliy and thus testing of modbus.py.
There was a problem hiding this comment.
It's not a Modbus test, it's a negative case testing wish Modbus integration lack of, unfortuately
| async_fire_time_changed(hass, now) | ||
| await hass.async_block_till_done() | ||
|
|
||
| # Check state |
| ), | ||
| ], | ||
| ) | ||
| @mock.patch.object(ModbusHub, "read_holding_registers") |
There was a problem hiding this comment.
There are a standard function in conftest you should use.
|
Most of the review comments do not make much sense to me. Not sure I will ever be able to make it to conform to the controversial suggestions here. Feel free to use some portions of the code for further development of the platform. I will focus my energy on more meaningful things. Thanks for your time. |
Breaking change
This PR adds new functionality and should not introduce any breaking changes.
❗ This PR is based on the @janiversen PR #46591
Proposed change
bit_switchallows mapping Modbus holding register bits to a HomeAssistant Switch. All switch operations such as on, off, and toggle works against individual bits of a Modbus holding register. You may use the Modbus input register as well, however, a switch will remain read-only.bit_sensoris pretty similar to thebit_switch. However, with thebit_sensoryou are not limited to 16bit Modbus register length. You may specify any arbitrary count and specify the bit number of the interest.Since multiple sensors or switches can read from the same Modbus register, a
functools.lru_cachecaching was added to cache Modbus register for one second. This should be enough to avoid re-reads. Any write operation to any of the registers reset the cache.Type of change
Example entry for
configuration.yaml:Additional information
Modbus bit sensor switch home-assistant.io#16831
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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: