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

Specify broadcast address for discovery #176

Merged
merged 5 commits into from Jul 25, 2019

Conversation

kochinc
Copy link
Contributor

@kochinc kochinc commented Jul 20, 2019

Sometimes it's useful to be able to specify the broadcast address for discovery. On a host with multiple network interfaces, when a discovery message is sent to 255.255.255.255:9999, the routing table decides which interface it is routed to. By specifying broadcast address, e.g. 192.168.xxx.255, the discovery message can be directed to a particular subnet only.

The modification to discover.py is very limited and shouldn't affect existing programs using the library. The cli.py is changed to include a --target option for specifying broadcast address.

In situations such as multiple network interfaces, it may be necessary to specify the broadcast address so that the discovery message is sent to the desired local network.
Sometimes it's necessary to specify what broadcast address it should send the discovery message. For instance, there are multiple network interfaces on the host.
Since --target is a top-level option and we want to use it in subcommands, use ctx.parent.params['target'] to retrieve it.
@@ -63,7 +65,7 @@ def discover(
if protocol is None:
protocol = TPLinkSmartHomeProtocol()

target = "255.255.255.255"
#target = "255.255.255.255"

Choose a reason for hiding this comment

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

block comment should start with '# '

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line can be deleted.

pyHS100/cli.py Outdated
@@ -40,12 +40,17 @@
required=False,
help="The device name, or alias, of the device to connect to.",
)
@click.option(
"--target",

Choose a reason for hiding this comment

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

trailing whitespace

@coveralls
Copy link

coveralls commented Jul 20, 2019

Coverage Status

Coverage increased (+0.08%) to 80.357% when pulling 70227be on kochinc:master into 2d60467 on GadgetReactor:master.

Copy link
Collaborator

@kirichkov kirichkov left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rytilahti merge if you're OK.

Copy link
Collaborator

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Looks fine to me, I have only one question: do you think it's better to have an address instead of an interface name for this?

@@ -43,6 +43,7 @@ class Discover:
@staticmethod
def discover(
protocol: TPLinkSmartHomeProtocol = None,
target: str = "255.255.255.255",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if that's the proper position for this in the argument list, but considering the current master has evolved and is breaking the backwards compatibility, I'd propose that we simply adjust discover to allow keyword-only arguments.

Copy link
Collaborator

@rytilahti rytilahti Jul 25, 2019

Choose a reason for hiding this comment

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

After giving this a bit thought, I think this can be changed in a separate PR, so let's get this merged if someone is eagerly waiting for this functionality. #158 is at least partially related (there are mentions about multiple subnets in the comments).

@rytilahti rytilahti merged commit 2c79feb into GadgetReactor:master Jul 25, 2019
@kirichkov
Copy link
Collaborator

Looks fine to me, I have only one question: do you think it's better to have an address instead of an interface name for this?

Not sure whether this was directed at me, or @kochinc, my general preference is to deal with interfaces, rather than IP addresses - network ranges change, interfaces generally stay the same. If go for interfaces, however, how do we address the 'broadcast' since broadcast should send to all interfaces? Perhaps a interface='all' keyword argument?

@kochinc
Copy link
Contributor Author

kochinc commented Jul 25, 2019

Either way it's fine for me. At first I wanted to add an interface parameter. After inspected the code, I realized it's very simple to just include a broadcast target address parameter. That solved my issue with TP-Link device discovery on Home Assistant.

@jkriegshauser
Copy link

Would it be possible to get a release with this merged so that we can get Home Assistant working with Kasa on different subnets before python-kasa is integrated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants