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

It is not possible to address a bulb outside of the current network #25

Closed
wants to merge 6 commits into from

Conversation

DaIgeb
Copy link
Contributor

@DaIgeb DaIgeb commented Jan 12, 2016

Hi
I tried to access a bulb outside of my current network.

As the router I'm using does not allow to send a broadcast package across different networks I was stuck. After some debugging I found out how I could improve this library to allow this.

Hopefully you like my changes.

Cheers

@codecov-io
Copy link

Current coverage is 55.12%

Merging #25 into master will increase coverage by +0.46% as of 9d1cee8

@@            master     #25   diff @@
======================================
  Files           42      42       
  Stmts         1211    1219     +8
  Branches       181     183     +2
  Methods          0       0       
======================================
+ Hit            662     672    +10
+ Partial         24      22     -2
  Missed         525     525       

Review entire Coverage Diff as of 9d1cee8

Powered by Codecov. Updated on successful CI builds.

@MariusRumpf
Copy link
Owner

I see two features one might want from what you have written:

  1. Use a different or additional broadcast address , which targets another subnet or light not only 255.255.255.255 (this net)
  2. Provide a set of known lights at the start. These may be set as checkable by the discovery or be set as undiscoverable but known and can be addressed.

Your pull request addresses feature one, you address the lights direct for discovery. I have the following suggestion:

  • Discovery in general does not need a sequence number in the packet, since we do not wait for an answer (this is what you changed to make the tests pass)
  • The broadcast addresses don't need to change at runtime, I think we should set them with the init() function and not pass it to startDiscovery as parameter.

I will create an issue for the second feature. Thanks for your work.

@DaIgeb
Copy link
Contributor Author

DaIgeb commented Jan 18, 2016

I have faced an additional problem with the broadcast package for which I intend to provide a second pull request. The issue is that windows does not resolve the broadcast address 255.255.255.255 to all connected networks but only the the one of the first adapter. Which in my case led to be not able to discover the bulb at all until switching the broadcast address. This seems to be a Windows only limitation.

@MariusRumpf MariusRumpf mentioned this pull request Mar 25, 2016
@MariusRumpf
Copy link
Owner

Merged into develop branch from which it will find it's way into master. Thanks a lot!

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

3 participants