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

NTP-based internet test #2118

Merged
merged 10 commits into from
Jun 25, 2020
Merged

NTP-based internet test #2118

merged 10 commits into from
Jun 25, 2020

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented May 29, 2020

Problem

  1. Gen 3 platforms do not have internet test at all
  2. Using 8.8.8.8:53 over TCP may not be ideal as it's TCP and most devices nowadays use UDP for cloud comms and it skips DNS step
  3. The logic around resetting network interfaces due to a number of failed cloud connection attempts is broken and only works somewhat correctly with TCP-based devices (Photon and P1 only)

Solution

This PR introduces an SNTP (Simple Network Time Protocol) implementation to be used for internet tests. By default we are targeting pool.ntp.org, which allows us to also test DNS and allows us to use the distributed network of NTP servers around the globe in the pool over UDP.

We are also changing the logic around when to trigger network interface reset due to cloud connection errors. Previously only "connection" attempts were registered as failures, whereas handshake errors were not taken into account at all. This does work somewhat ok for TCP-based devices (Photon/P1), but these days does not make much sense for connection-less UDP-based devices.

Now, we are taking into account any kind of a cloud connection error and will wait up to 5 minutes of consecutive cloud connection failures across all platforms to trigger a reset of network interfaces.

Steps to Test

  • Unit tests
  • Corrupt device/server keys, watch the logs. The device should reset network interfaces after about 5 minutes
  • Probably easiest to test on WiFi devices: disconnect WAN on router/set firewall rules to forbid the devices to communicate over UDP. The device should reset network interfaces after about 5 minutes

Example App

N/A

References

  • [CH36520]
  • [CH48170]
  • [CH51505]
  • [CH34989]

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@keeramis
Copy link
Contributor

keeramis commented Jun 2, 2020

I will start by testing out some tests mentioned on one of my devices.

@avtolstoy avtolstoy force-pushed the feature/ntp-internet-test branch 2 times, most recently from e1931de to c281414 Compare June 22, 2020 14:01
Copy link
Member

@sergeuz sergeuz left a comment

Choose a reason for hiding this comment

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

I'm approving this PR since it does the job. The timestamp calculations don't seem to be correct though.

hal/network/util/simple_ntp_client_detail.h Outdated Show resolved Hide resolved
hal/network/util/simple_ntp_client.cpp Show resolved Hide resolved
hal/network/util/simple_ntp_client.cpp Outdated Show resolved Hide resolved
hal/network/util/simple_ntp_client.h Outdated Show resolved Hide resolved
hal/network/util/simple_ntp_client.cpp Outdated Show resolved Hide resolved
hal/network/util/simple_ntp_client_detail.h Outdated Show resolved Hide resolved
hal/network/util/simple_ntp_client_detail.h Show resolved Hide resolved
hal/src/electron/socket_hal.cpp Show resolved Hide resolved
system/src/system_task.cpp Outdated Show resolved Hide resolved
@avtolstoy avtolstoy added ready to merge PR has been reviewed and tested and removed needs review labels Jun 25, 2020
@avtolstoy avtolstoy merged commit f7c29a3 into develop Jun 25, 2020
@avtolstoy avtolstoy deleted the feature/ntp-internet-test branch June 25, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready to merge PR has been reviewed and tested
Projects
None yet
3 participants