Make Modbus hub name mandatory

[vzahradnik]

This pull request enforces hub name as a mandatory parameter. At the moment it's entirely optional, which introduces ambiguity.

Breaking change

Modbus hub name was optional. With this change, user has to specify a hub name
to avoid ambiguity in the configuration:

• Set name parameter for each Modbus hub
• Set hub parameter for each Modbus entity, telling which hub to use

Proposed change

Here's an example config:

modbus:
  - name: hub1
    type: tcp
    host: 127.0.0.1
    port: 5020
  - name: hub2
    type: tcp
    host: 127.0.0.1
    port: 5021

binary_sensor:
  - platform: modbus
    inputs:
      - name: Sensor1
        hub: hub1
        slave: 1
        address: 100

To better describe my point, let me show you the following several scenarios:

• If we omit the hub parameter from the binary_sensor, this parameter will be set to default. Such a hub doesn't exist. Home Assistant throws an exception at some point while trying to access a default hub. We could add a code, which will set up an alias for the first hub in a list, but I think it's better to make the parameter mandatory instead
• If we don't specify a name for hub1 and hub2, we end up with a non-working configuration. Both hubs are called default
• By making the name mandatory, user has to name each hub and reference it in each Modbus entity. This approach should help avoiding configuration errors. Home Assistant config validator doesn't catch such errors - it would be only possible with cross-validation in place to validate configs between entities (i.e., if binary_sensor doesn't specify a hub, there should at least one Modbus hub with a default name. Otherwise, the config is invalid).

All examples in the documentation already specify a hub name. Users who copy and paste the sample documentation most likely use the name parameter already. However, if they're calling Modbus services write_coil and write_register from the automation, they will need to adapt their automation rules to also specify a hub.

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)
• 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: Make Modbus hub name mandatory home-assistant.io#13920

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
• The code has been formatted using Black (black --fast 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.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[stale]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[frenck]

Not stale, awaits our review.

[frenck]

Been visiting this PR a couple of times now. I wonder why we should consider this. I get that it might have been a better idea to make the option mandatory from the get-go, but right now, it is optional and provides a default.

The only thing this PR will do, is causing a breaking change for existing users that currently DO rely on the default. So, why should we go and break this?

[vzahradnik]

There are three scenarios:

• A user defines one Modbus hub, and he doesn't assign a name to it. Home Assistant will use the name "default"

• A user defines one Modbus hub, and names it "hub1"; in the config section of sensors and switches he doesn't specify a hub name

• A user defines two or more Modbus hubs

• In the first case, everything will work. Added Modbus sensors, switches,... will use the hub with name "default".

• In the second case, Home Assistant will access a non-existing hub with name "default" to poll for sensor and switch states. Config validation will pass (hub name is missing, but it's optional), and as soon as Home Assistant tries to poll for sensor data, it throws an error into a log. User needs to fix the config by specifying a proper hub name. However, for normal users, it might not be clear from reading all the exceptions

• Third scenario throws the same errors as the second scenario.

The point of this PR is to enforce better config validation. Ideally, Home Assistant would support something like cross-validation where I could check in the sensors section the definitions of Modbus hubs, and notify users about misconfiguration. It's definitely better than throwing exceptions after validation has "passed".

Like you mentioned, this option should be probably mandatory from the start. While working on Modbus, I found several areas, where current configuration needs some improvement to avoid errors during runtime.

There are two options:

• Introducing only backwards-compatible changes. As a result, the code will always be sub-optimal, because we cannot fix design decisions made years ago
• Allowing to break backwards compatibility

While discussing in other PRs, I get your policy is to maintain backwards compatibility whenever possible. Anyway, I opened this PR to at least discuss the problem. If you think the current HA behavior is acceptable, you can close this PR.

Thanks!

[frenck]

The point of this PR is to enforce better config validation.

I'm not against that, however, breaking perfectly working existing configuration without a darn good reason to do so, is not.

I think this change would make sense if it was converted from separate platform components into an actual integration (as that would combine the breaking changes).

[vzahradnik]

Ok, in that case I suggest to close this PR. I will also try to get my other PRs merged (as time permits) in a backwards-compatible way. Then, we can start a discussion, how to start a transition into a proper integration.