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

Connect rfxtrx to a device over a tcp connection #28297

Merged
merged 1 commit into from Nov 20, 2019

Conversation

@foxy82
Copy link
Contributor

foxy82 commented Oct 28, 2019

Breaking Change:

Added ability to connect to rfxtrx over TCP. The previous "device" config needs to be changed to port for local USB connection. If you want to connect to a device via TCP use host and port. See the documentation for more details.

Description:

Allow RFXtrx device to be connected to over TCP. Allowing a user to connect the device to another linux host in their house and run ser2net to expose it.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

rfxtrx:
  port: PATH_TO_SERIAL_DEVICE

OR

rfxtrx:
  host: tcp host
  port: tcp port

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

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Oct 28, 2019

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

@foxy82 foxy82 mentioned this pull request Oct 28, 2019
2 of 2 tasks complete
@Danielhiversen

This comment has been minimized.

Copy link
Member

Danielhiversen commented Oct 28, 2019

Why do you need to change to port for local USB connection?

@foxy82

This comment has been minimized.

Copy link
Contributor Author

foxy82 commented Oct 28, 2019

Then we can make the port required and works for both local and remote and the host optional. Otherwise I have to make all config optional (I think) which means a user could set it up with no host/port/device. I used the way rflink does it as motivation.

Unless there is a way with vol to say the config either has to be "device" OR ("host" and "port")?

@MartinHjelmare MartinHjelmare changed the title rfxtrx can now connect to a device over a tcp connection Connect rfxtrx to a device over a tcp connection Oct 31, 2019
@@ -84,7 +85,8 @@
{
DOMAIN: vol.Schema(
{
vol.Required(CONF_DEVICE): cv.string,
vol.Required(CONF_PORT): vol.Any(cv.port, cv.string),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 31, 2019

Member

Make a new schema and use vol.Any to wrap the two schemas.

BASE_SCHEMA = vol.Schema(
    {
        vol.Optional(CONF_DEBUG, default=False): cv.boolean,
        vol.Optional(CONF_DUMMY, default=False): cv.boolean,
    }
)

DEVICE_SCHEMA = BASE_SCHEMA.extend(
    {
        vol.Required(CONF_DEVICE): cv.string,
    }
)

PORT_SCHEMA = BASE_SCHEMA.extend(
    {
        vol.Required(CONF_PORT): cv.port,
        vol.Optional(CONF_HOST): cv.string,
    }
)

CONFIG_SCHEMA = vol.Schema(
    {
        DOMAIN: vol.Any(DEVICE_SCHEMA, PORT_SCHEMA)
    },
    extra=vol.ALLOW_EXTRA,
)
Dev automation moved this from Needs review to Review in progress Oct 31, 2019
homeassistant/components/rfxtrx/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rfxtrx/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rfxtrx/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rfxtrx/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/rfxtrx/__init__.py Outdated Show resolved Hide resolved
@foxy82

This comment has been minimized.

Copy link
Contributor Author

foxy82 commented Nov 7, 2019

This PR is ready to merge and I've changed the breaking change - although I think it would be much easier to use to make the breaking change and reduce the config options.
Although if I get time between now and before you merge it I'll try to see if I can incorporate this fix if it works: #28604

@foxy82

This comment has been minimized.

Copy link
Contributor Author

foxy82 commented Nov 7, 2019

I've updated the docs in line with the change to reintroduce device. Not sure why the docs-missing tag when that PR has been referenced here from the start? Unless it's due to the review comments making the document change out of date?

Copy link
Member

MartinHjelmare left a comment

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Nov 8, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 8, 2019

Try rebasing on latest dev branch to let the build pass.

@foxy82 foxy82 force-pushed the foxy82:rfxtrx_tcp branch from 77b579c to 9969cde Nov 10, 2019
@foxy82

This comment has been minimized.

Copy link
Contributor Author

foxy82 commented Nov 10, 2019

Rebased

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 10, 2019

The CI is still not happy. Maybe easier to start over in a fresh branch?

@foxy82 foxy82 force-pushed the foxy82:rfxtrx_tcp branch 2 times, most recently from c3175b9 to 12a01da Nov 12, 2019
@foxy82 foxy82 force-pushed the foxy82:rfxtrx_tcp branch from 12a01da to 348a9e4 Nov 12, 2019
@foxy82 foxy82 force-pushed the foxy82:rfxtrx_tcp branch from 348a9e4 to 161edc9 Nov 12, 2019
@cgarwood cgarwood merged commit 9fd6afc into home-assistant:dev Nov 20, 2019
11 checks passed
11 checks passed
CI Build #20191112.43 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 1208ab4...161edc9
Details
codecov/project 94.43% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 20, 2019
@lock lock bot locked and limited conversation to collaborators Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.