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

Improve LAN discovery #586

Merged
merged 2 commits into from Oct 7, 2017

Conversation

Projects
None yet
5 participants
@Diadlo

Diadlo commented Aug 27, 2017

Issue: If another tox instance started on the not default port, LAN
discovery will be failed.

Now tox will iterate though all possible ports to find another tox
instances.


This change is Reviewable

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Aug 28, 2017

This change, reques update the spec
Also, I thought about trying two ports: default and incremented (any suggestions?)

Diadlo commented Aug 28, 2017

This change, reques update the spec
Also, I thought about trying two ports: default and incremented (any suggestions?)

@im-grey

This comment has been minimized.

Show comment
Hide comment
@im-grey

im-grey Aug 30, 2017

This is great news.

im-grey commented Aug 30, 2017

This is great news.

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Aug 30, 2017

@Diadlo can you please also add logging to that, so that if LAN discovery finds something this fact is logged

zoff99 commented Aug 30, 2017

@Diadlo can you please also add logging to that, so that if LAN discovery finds something this fact is logged

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Aug 31, 2017

@zoff99 Because LAN discovery uses broadcast messages, I only can handle when someone found you, but not when you found someone

Diadlo commented Aug 31, 2017

@zoff99 Because LAN discovery uses broadcast messages, I only can handle when someone found you, but not when you found someone

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 1, 2017

@Diadlo that sounds good. with logging i can actually try it on my LAN with 2 clients and see if it works.

because now there is no logging like that, and i think its not working on my LAN.
also could it be true that a LAN node has always less "priority" than a normal node, and then the internet route would be always preferred? (thats how it feels like in my LAN)

zoff99 commented Sep 1, 2017

@Diadlo that sounds good. with logging i can actually try it on my LAN with 2 clients and see if it works.

because now there is no logging like that, and i think its not working on my LAN.
also could it be true that a LAN node has always less "priority" than a normal node, and then the internet route would be always preferred? (thats how it feels like in my LAN)

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 1, 2017

https://github.com/TokTok/spec/blob/master/spec.md#lan-discovery
"When enabled, toxcore sends these packets every 10 seconds to keep delays low"

does that mean with landiscovery "ON" packets will be sent every 10 seconds forever? or will this stop at some point?
(thinking about mobile devices again)

zoff99 commented Sep 1, 2017

https://github.com/TokTok/spec/blob/master/spec.md#lan-discovery
"When enabled, toxcore sends these packets every 10 seconds to keep delays low"

does that mean with landiscovery "ON" packets will be sent every 10 seconds forever? or will this stop at some point?
(thinking about mobile devices again)

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 1, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

zoff99 commented Sep 1, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 1, 2017

does this only scan from "TOX_PORTRANGE_FROM 33445" to "TOX_PORTRANGE_TO 33545" ?

zoff99 commented Sep 1, 2017

does this only scan from "TOX_PORTRANGE_FROM 33445" to "TOX_PORTRANGE_TO 33545" ?

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 1, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

zoff99 commented Sep 1, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Sep 1, 2017

@zoff99 Added logging

does this only scan from "TOX_PORTRANGE_FROM 33445" to "TOX_PORTRANGE_TO 33545" ?

Yes

I thought about trying two ports: default and incremented

What do you think about it?

Diadlo commented Sep 1, 2017

@zoff99 Added logging

does this only scan from "TOX_PORTRANGE_FROM 33445" to "TOX_PORTRANGE_TO 33545" ?

Yes

I thought about trying two ports: default and incremented

What do you think about it?

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Sep 2, 2017

@Diadlo what is "incremented" port ?

zoff99 commented Sep 2, 2017

@Diadlo what is "incremented" port ?

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Sep 2, 2017

@zoff99

send_LANdiscovery(net_htons(TOX_PORT_DEFAULT), fr_c->dht);
for (uint16_t port = first; port < last; port++) {
    send_LANdiscovery(net_htons(port), fr_c->dht);
}

Diadlo commented Sep 2, 2017

@zoff99

send_LANdiscovery(net_htons(TOX_PORT_DEFAULT), fr_c->dht);
for (uint16_t port = first; port < last; port++) {
    send_LANdiscovery(net_htons(port), fr_c->dht);
}
@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Sep 17, 2017

Member

:lgtm_strong:


Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Member

robinlinden commented Sep 17, 2017

:lgtm_strong:


Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Oct 3, 2017

Member

Rebase (or checkbox).

Member

iphydf commented Oct 3, 2017

Rebase (or checkbox).

Improve LAN discovery
Issue: If another tox instance started on the not default port, LAN
discovery will be failed.

Now tox will iterate though all possible ports to find another tox
instances.
@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Oct 3, 2017

@iphydf Done (both)

Diadlo commented Oct 3, 2017

@iphydf Done (both)

@robinlinden robinlinden added this to the v0.1.11 milestone Oct 6, 2017

@robinlinden robinlinden merged commit 1577c75 into TokTok:master Oct 7, 2017

4 of 7 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
code-review/reviewable 2/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment