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

Update NTPClient.ino #3919

Closed
wants to merge 4 commits into from
Closed

Update NTPClient.ino #3919

wants to merge 4 commits into from

Conversation

Danara5
Copy link

@Danara5 Danara5 commented Dec 5, 2017

When testing NTPClient on many devices on a home gateway, the router would only forward NTP responses to the first device to call NTP using that port. By giving each device a random port, the NTP responses are forwarded to the correct internal IP. The UDP port conflict may not be evident on all NAT routers, and could depend on the timing between NTP calls and how long the router keeps the entry in the port forward table. Randomizing the port used minimizes the chance of a conflict with other devices.

When testing NTPClient on a many devices on a home gateway, the router would only forward NTP responses to the first device to call NTP using that port.   By giving each device a random port, the NTP responses are forwarded to the correct internal IP.  The UDP port conflict may not be evident on all NAT routers, and could depend on the timing between NTP calls and how long the router keeps the  entry in the port forward table.  Randomizing the port used minimizes the chance of a conflict with other devices.
@themindfactory
Copy link
Contributor

themindfactory commented Dec 5, 2017 via email

@devyte
Copy link
Collaborator

devyte commented Dec 8, 2017

I think this code change is not viable, because the result of the random call could conflict with a whole range of local services, e.g.: mdns on port 5353.
In addition, if it's true that having multiple ESPs with the same localport can cause the router NAT to mess up, this won't fix the issue, it would only mitigate it, as multiple ESPs could still end up with the same port. It would be unlikely, but still possible, and if it should happen, the failure would be spurious.

@earlephilhower
Copy link
Collaborator

Closing as @devyte makes a valid point about this stomping on local services (and being 2 yrs+ old). If another maintainer feels otherwise, feel free to reopen.

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

5 participants