Extend Modbus binary sensor to support discrete inputs

[vzahradnik]

Breaking Change:

• coils array renamed to inputs
• coil configuration variable renamed to address
• Variable was renamed to better support configuration with discrete inputs. Coil would be confusing in those cases. Address is standardized term in Modbus terminology.

Backward compatibility was added to accept old configs and warn users in the logs.

Description:

This PR extends Modbus Binary Sensor to also read data from discrete inputs. New variable input_type was added to define if user wants to read from coil or discrete_input. Coil is the default option.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#12036

Example entry for configuration.yaml:

binary_sensor:
  - platform: modbus
    scan_interval: 10
    inputs:
      - name: Sensor1
        hub: hub1
        slave: 1
        address: 100
        input_type: discrete_input

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the [development checklist][dev-checklist]

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

• Documentation added/updated in home-assistant.io

[homeassistant]

Hi @vzahradnik,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[probot-home-assistant]

Hey there @adamchengtkc, mind taking a look at this pull request as its been labeled with a integration (modbus) you are listed as a codeowner for? Thanks!

[adamchengtkc]

A duplicate of #27397 ?

[vzahradnik]

I see in #27397 they try to keep the compatibility and to introduce optional parameter register_type.
I think this approach is confusing and the only benefit is keeping backwards compatibility.

discrete_inputs are not coils and both of them are not registers according to the Modbus terminology. Anyway, I will respect your decision what should be adopted.

Thanks!

[adamchengtkc]

I don't have a device that require setting discrete_input and I don't have a preference on either approach. Perhaps it would be good if you can reference this PR and raise this with the author of #27397 ?
And this not going to be my decision. It is the community (us together) to decide what is best
Thanks

[vzahradnik]

Sure, I will leave a comment there. Thanks!

[MartinHjelmare]

See #27397 (comment) for the question.

[tkaczu2]

How can I implement the above configuration in HA?

[vzahradnik]

Hi @tkaczu2, this change is not yet merged so there is no official way how to do that.
However, if you want to use this feature anyway, here's what I use.

Home Assistant supports loading custom components:
https://developers.home-assistant.io/docs/en/creating_component_loading.html

You can copy existing components there, like whole Modbus folder, and Home Assistant will override default implementation with this custom one. Not recommended approach, but it works.

If you have your custom Modbus folder (i.e. <config directory>/custom_components/Modbus), then you can override original files with my implementation.

As a last step, configure your Modbus entry according to my proposed config above. The only major change is that coil has been renamed to input and a new option input_type was added.

You can also check PR #27397, which does the same thing, however the config syntax is backwards compatible. Pick up what works best for you.

[vzahradnik]

I updated the implementation to be backward compatible as suggested by @balloob in #27397. Change was manually tested and works with different configurations without any issue. Also, I created another PR, home-assistant/home-assistant.io#12036, to update documentation for this change.

Here's an output from my log:

2020-02-10 17:56:16 WARNING (MainThread) [homeassistant.components.modbus.binary_sensor] The 'coils' option (with value '[OrderedDict([('name', 'plc_01_s8a'), ('hub', 'plc_01'), ('slave', 1), ('coil', 0)]), OrderedDict([('name', 'plc_01_s8b'), ('hub', 'plc_01'), ('slave', 1), ('address', 1)]), OrderedDict([('name', 'plc_01_s8c'), ('hub', 'plc_01'), ('slave', 1), ('coil', 2)])]') is deprecated, please replace it with 'inputs'
2020-02-10 17:56:16 WARNING (MainThread) [homeassistant.components.modbus.binary_sensor] The 'coil' option (with value '0') is deprecated, please replace it with 'address'
2020-02-10 17:56:16 WARNING (MainThread) [homeassistant.components.modbus.binary_sensor] The 'coil' option (with value '2') is deprecated, please replace it with 'address'

[MartinHjelmare]

We normally don't want sync locks in our thread pool. We shouldn't address this in this PR, but it would be something to improve for the future.

It would be ok to replace the sync lock with an asyncio lock. That would require changing the context around the affected methods from sync to async.

[vzahradnik]

Maybe I will take a look at it someday. I try to rework Modbus one step at a time. It's not the most perfect integration, but it gets better.