Modbus Switch toggle doesn't work with latest code

[vzahradnik]

The problem

With latest (nightly) Modbus code I am no longer able to toggle the Modbus switch.

I believe the error is related to PR #49910. I will investigate further whether I have a non-standard configuration, or if this is really an issue with our Modbus integration.

What is version of Home Assistant Core has the issue?

core-2021.6.0.dev0

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Core

Integration causing the issue

Modbus

Link to integration documentation on our website

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

Example YAML snippet

modbus:
  - name: plc_01
    type: tcp
    host: 127.0.0.1
    port: 5020

    switches:
      - name: plc_01_in_switch
        address: 8
        write_type: coil
        slave: 1
        scan_interval: 1

Anything in the logs that might be useful for us?

2021-05-03 11:30:19 ERROR (SyncWorker_0) [homeassistant.components.modbus.modbus] Pymodbus: WriteCoilResponse(8) => 1

Additional information

No response

[probot-home-assistant]

modbus documentation
modbus source
_{^{(message by IssueLinks)}}

[probot-home-assistant]

Hey there @adamchengtkc, @janiversen, mind taking a look at this issue as its been labeled with an integration (modbus) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[janiversen]

no you are missing “verify:” without that it will not read from your device but just set the state directly.

[janiversen]

Let me know what you find, #49910 should not affect toogling but could affect verifying.

[vzahradnik]

I will check... I created this issue to let you know about this. Maybe I will just need to update my configuration.

[janiversen]

Thanks for telling me, let me know if you find anything.

[vzahradnik]

I will troubleshoot during this week. All my findings will go here. You'll get an email :)

[rostislav-palivoda]

no you are missing “verify:” without that it will not read from your device but just set the state directly.

I always wondering why it's mandatory to continuously very state of relay if there is only one master on the line. Isn't it possible to do make state verification only after the command is send during configure N seconds time-out period?

Yes, I understand both use cases are possible - switch can be triggered manually. Still in installations where you need just to confirm execution of the command a request loop is an overhead - it just bring a lot of waste traffic in the modbus network.

Let me explain what I mean by use case. Relay board can receive command and send it's state. Its configured as switch.

- name: foo_relay
  slave: 8
  input_type: holding
  address: 5
  command_on: 0x0100
  command_off: 0x0200
  state_on: 0x0001
  state_off: 0x0000

I see the state of relay requested in a loop with configured interval. The log is full of such requests:

2021-05-04 12:29:08 DEBUG (SyncWorker_1) [pymodbus.transaction] SEND: 0x8 0x3 0x0 0x5 0x0 0x1 0x94 0x92
2021-05-04 12:29:08 DEBUG (SyncWorker_1) [pymodbus.transaction] Changing transaction state from 'SENDING' to 'WAITING FOR REPLY'
2021-05-04 12:29:08 DEBUG (SyncWorker_1) [pymodbus.transaction] Changing transaction state from 'WAITING FOR REPLY' to 'PROCESSING REPLY'
2021-05-04 12:29:08 DEBUG (SyncWorker_1) [pymodbus.transaction] RECV: 0x8 0x3 0x2 0x0 0x0 0x64 0x45

Instead of this I propose to request state during N seconds after the change state command is sent.

For example a parameter

  #scan_interval: 15 <- if not defined, then no rescans
  verify_state: 3
  verify_timeout: 15

This configuration will tell modbus component to request state of the switch 3 time each 15 seconds after the switch command send. If no response after 45 seconds then switch becomes unavailable. Stop request on first successful response.

[janiversen]

@rostislav-palivoda It looks to me as if you are requesting 2 new features
a) scan_interval meaning coil will only be read at startup and after a write. I think thats makes a lot of sense.
b) configurable scan_atop and interval between write and read. I do not understand the need for that but I do not see a problem.
For both cases please raise new issues enabling us to discuss them separately from this potential error.

@vzahradnik new switch doc. just went online, maybe it helps you :-)

[rostislav-palivoda]

Oh, you reply while I was fulfilling my comment with log data and example, See my updated comment above. And yes a minor change (a) will have a great effect. Change (b) is not required - scan_interval: 120 per sensor if perfectly fine.

[janiversen]

please raise a new issue....we do not like to have multiple issues mixed.

[rostislav-palivoda]

Done - https://community.home-assistant.io/t/improve-efficiency-of-modbus-switch-component-for-relays/305098

Do you think its doable in the nearest future?

I'm thinking about making changes in the component myself but not sure how to start. I read on forum about custom component but not sure what it means and is it possible to make changes from VS addon right inside the HA UI? Can someone share links to the proper guidelines?

[janiversen]

?? I am not following the community. Issues with core are raised here.
Your questions seems to be community nature not related to this issue....please try to only write things directly related to the issue at hand. Here we are solving problems/adding features not providing general help.

[rostislav-palivoda]

Then I should create issue here on github?

[janiversen]

I am not sure what the right way is, might be that feature requests in the community are automatically copied here (the text there suggest it).

I only look at “pull requests” and “issues”.

[rostislav-palivoda]

Sorry for off topic. I'm asking because there is big warning BUGS ONLY if I try to report issue for the HA core project. :)

I really appreciate if you can take care of this change. Then I'll create issue in core and delete post from community.
Else I'll leave it in community and maybe someday will try to fix it by myself.

Not sure about community feature requests propagation in to the git, looks like not.

[vzahradnik]

new switch doc. just went online, maybe it helps you :-)

I'll check, maybe I just missed something.

[vzahradnik]

OK, first problem I see is that as far as I know you can't turn on verify without giving at least one optional parameter. So if I want to verify the state and keep all optional subattributes as-is, it is not possible.

2021-05-05 13:44:43 ERROR (MainThread) [homeassistant.config] Invalid config for [modbus]: expected a dictionary for dictionary value @ data['modbus'][0]['switches'][0]['verify']. Got None.

Perhaps we could modify the scheme so that it will create an empty dict if nothing is supplied.

The documentation for switch contains this:

switches:
...
      - name: Switch2
        ...
        verify:

So officialy such a config (verify without optional parameters) should be supported. Or it is a typo in the documentation.

Second problem is this:

        # switch.py, line 129
        if result is False:
            self._available = False
            self.schedule_update_ha_state()

If turn_on() or turn_off() fails, we notify UI that the switch is unavailable. But because verify is turned off, the switch will always stay unavailable. There is no update() call. In case that verify is turned off we should probably ignore the failure so that users can still toggle the switch.

Also, I found out that reading values in my setup is also a problem. When I turn on verify, I will get the same error while reading a coil.

I found the following in my logs but I don't know for sure if it's related:

AttributeError: 'NoneType' object has no attribute 'read_coils'
2021-05-05 13:39:12 ERROR (MainThread) [homeassistant] Error doing job: Future exception was never retrieved
Traceback (most recent call last):
  File "/usr/lib/python3.9/concurrent/futures/thread.py", line 52, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/srv/git/Lutemi/home-assistant-core/homeassistant/components/modbus/switch.py", line 102, in <lambda>
    self.hass, lambda arg: self.update(), self._scan_interval
  File "/srv/git/Lutemi/home-assistant-core/homeassistant/components/modbus/switch.py", line 159, in update
    result = self._read_func(self._slave, self._verify_address, 1)
  File "/srv/git/Lutemi/home-assistant-core/homeassistant/components/modbus/modbus.py", line 236, in read_coils
    result = self._client.read_coils(address, count, **kwargs)

[janiversen]

Hmm you have some nice issues.

1. VERIFY:
   Actually I tried pretty hard to convince vol, that it should accept "verify:" but did not succeed. I am not sure how you would do the "{}", but if possible it is a very good idea. The other is to have a dummy attribute with a default value. Let me play a bit with that, but suggestions are more than welcome.

2. turn_off / turn_on
   Another corner case, but easy to fix. My only problem is that I wanted to signal that turn_on/turn_off failed. Anyhow I am happy to remove these lines.

3. Stack trace.
   I wonder how you managed to provoke that, it means the modbus connection is closed (_client is None), but let us safeguard it.

THANKS for the analysis, I will add a PR you can test either later today or tomorrow.

[vzahradnik]

1. I might take a look at it too... Let me know if you make any progress
2. Your approach is correct but I think it is better to keep the switch in a working state. Imagine initializing Home Assistant at the time the Modbus server is unavailable. Then, as the server becomes available again, the component will always stay unavailable. But as you write, it is a corner case.
3. The stack trace was printed after I terminated Home Assistant with Ctrl+C. So it might be a false positive.

[janiversen]

1. Please please any ideas are welcome !.
   My idea right now is to correct the documentation and require that a attribute is present, because schema changes are NOT allowed in patch releases, and I do not want for 2021.6 before we solve this.

If you or I find a way that will be in 2021.6.

about 2: what if I leave the code in turn_on/turn_off and add this to update:

   if not self._verify:
      self._available = True

(code example, not real!), because the update is called independent of verify or not...with this the switch would be unavailable for SCAN_INTERVAL seconds and then go back to Available. WDYT ?

3. Could you please dig a bit, I have a bad feeling that you have seen a racing problem.
   Independent I will add a guard to secure against this in the future.

Thanks for your help.

[janiversen]

Breaking news: The new Switch will NOT be in 2021.5 (as I asked for), the documentation (New modbus switch. #17516) is only on Next !!

So we have time, meaning lets do this right ! I really need help how to define the schema, and would really appreciate to see an idea from you.

[janiversen]

It also means that you HOPEFULLY do not have problems with 2021.5.0b7 ?

[vzahradnik]

Breaking news: The new Switch will NOT be in 2021.5 (as I asked for), the documentation

Makes sense. Today I checked the documentation, and your changes were not there.

Let's start with the schema. I will try to look into it tomorrow.

[janiversen]

I have submitted a PR solving issue 2) and 3), but I need your opinion (see #500117).

Thanks for helping. I have also raised the question on discord.

Btw. are you on discord ? I am janGranada.

[vzahradnik]

I was on Discord a long time ago. But if it helps with our communication, I might re-join :)