Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP]Support for Multi modbus hub #17901

Closed
wants to merge 10 commits into from
Closed

Conversation

crhan
Copy link
Contributor

@crhan crhan commented Oct 28, 2018

Description:

There is a feature request at https://community.home-assistant.io/t/ability-to-add-multiple-modbus-hubs/16365

This PR is try to implement this feature.

Now modbus components can have a config key named "hub_name". If not provided, the default value "default" is used. So this PR does not break the current config.

Related issue (if applicable):

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

modbus:
  - type: rtuovertcp
    host: 192.168.15.129
    port: 8887
    hub_name: ventilation
sensor:
- platform: modbus
  scan_interval: 5
  registers:
  - name: ventilation_operation_mode
    slave: 1
    register: 2
    register_type: holding
    count: 1
    hub_name: ventilation 
  - name: ventilation_fan_mode
    slave: 1
    register: 3
    register_type: holding
    count: 1
    hub_name: ventilation

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@frenck
Copy link
Member

frenck commented Oct 29, 2018

Could not find a related PR in our documentation repository. Adding docs-missing label.

CONF_BYTESIZE = 'bytesize'
CONF_STOPBITS = 'stopbits'
CONF_PARITY = 'parity'
CONF_BAUDRATE = "baudrate"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of unrelated changes in this PR making it very hard to see what has actually changed. I'm assuming you're using some auto formatter. Please update your PR to not contain any unrelated style changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much interested in this, so I thought I'd help out a bit with these changes.

Also, Travis CI detected a problem in components/climate/flexit.py
I updated that to the new implementation. Unfortunately I can't really test that component.

@crhan I can't directly contribute to your PR but commits are in my own fork:
style changes in: 813d077
flexit update in: 3149ec6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all and @benvm your commit is cherry-picked to my branch.

Sorry I don't have enough time to complete this PR very soon. But this feature is worked in my house since I submit this PR. So basically this feature works well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this work.
I have created a new PR #19726, so it is easier for me to make further changes.

@balloob
Copy link
Member

balloob commented Feb 6, 2019

Closing this in favor of #19726

@balloob balloob closed this Feb 6, 2019
@ghost ghost removed the in progress label Feb 6, 2019
@ghost ghost removed the platform: switch.modbus label Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants