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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow rfxtrx network usage #25378

Closed
wants to merge 4 commits into from
Closed

Conversation

barrystaes
Copy link

@barrystaes barrystaes commented Jul 21, 2019

WIP asking for feedback 馃挕

Description:

I wanted to use rfxcom device via network. Started with this HA forum thread, then a PR to pyRFXtrx, and now this (concept) PR to Home Assistant.

Pull request with documentation for home-assistant.io: todo on checklist

Example entry for configuration.yaml:

rfxtrx:
  host: '192.168.0.123'
  port: 10001

So changed:

  • previously only device: /dev/ttyUSB0 was supported, the above is an alternative.
  • added voluptuous indication that only 1 connection config is allowed (device / host / dummy)

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.
  • Make a PR with updated documentation
  • 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.

@homeassistant
Copy link
Contributor

Hi @barrystaes,

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!

@ghost
Copy link

ghost commented Jul 21, 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!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@homeassistant
Copy link
Contributor

Hi @barrystaes,

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!

@barrystaes
Copy link
Author

Code is UNTESTED. Also beware i'm new to Python, feedback is very welcome.

Questions for owner:

  • Is YAML ok? If networkport is provided (optional) then device is interpreted as hostname.

@elupus
Copy link
Contributor

elupus commented Jul 22, 2019

Just add new fields instead "host"/"port"

@barrystaes
Copy link
Author

Ok wil do.

I was not sure how to make either device or host a required attribute, or if its even possible.

@elupus
Copy link
Contributor

elupus commented Jul 22, 2019

You can use voluptuous Inclusive/Exclusive
https://alecthomas.github.io/voluptuous/docs/_build/html/voluptuous.html

@barrystaes
Copy link
Author

Ok implemented a host and port config now. Thanks @elupus !

I am now going to look into how to properly test this locally, update documentation, etc.

@MartinHjelmare MartinHjelmare added this to Needs review in Dev Jul 23, 2019
Dev automation moved this from Needs review to Review in progress Jul 28, 2019
homeassistant/components/rfxtrx/__init__.py Show resolved Hide resolved
vol.Exclusive(CONF_DEVICE, CONNECTION_GROUP): cv.string,
vol.Exclusive(CONF_HOST, CONNECTION_GROUP): cv.string,
vol.Exclusive(CONF_DUMMY, CONNECTION_GROUP): cv.boolean,
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.positive_int,
Copy link
Contributor

Choose a reason for hiding this comment

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

port and host should be in a Inclusive group. That mean both are required. There is no point in a default port since this is all custom fetures anyway.

Copy link
Author

@barrystaes barrystaes Aug 1, 2019

Choose a reason for hiding this comment

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

I tried but failed. Do you have an example of such a inclusive group within a exclusive group?

Or is the yaml below fine? This is easy to implement using two Required wrapped into one Exclusive.

rfxtrx:
  network:
    host: 1.2.3.4
    port: 5678

Copy link
Author

Choose a reason for hiding this comment

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

@elupus do you have an example of such a inclusive group within a exclusive group? Could not find a component/platform that does this.

Copy link
Member

@Danielhiversen Danielhiversen left a comment

Choose a reason for hiding this comment

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

Remember to upgrade to pyRFXtrx 0.24.0

@barrystaes
Copy link
Author

Update: PR is not abandoned, i'll find time within a few weeks at most.

@MartinHjelmare
Copy link
Member

@barrystaes if you're not going to work on this PR for a while, please close it. Then you can reopen the PR yourself and we'll keep thread history. We're trying to decrease our open PR buffer. Thanks!

@frenck
Copy link
Member

frenck commented Nov 19, 2019

This PR is now running stale for 3 months, therefore I'm closing it for now.

Feel free to re-open when ready.

@frenck frenck closed this Nov 19, 2019
Dev automation moved this from Review in progress to Cancelled Nov 19, 2019
@lock lock bot locked and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

6 participants