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

[quectel] fixes PPP resume during warm boot #2772

Merged
merged 1 commit into from
May 28, 2024

Conversation

technobly
Copy link
Member

@technobly technobly commented May 22, 2024

Problem

  • slo/connect_time failing warm boot times of < 30s on EG91-NAX due to PPP data link not resuming, requiring a modem power cycle and cold boot, which adds ~30s to the connection time. So instead of 6-10s warm boot times, we were seeing 31-33s warm boot times about 50% of the time.

Solution

  • After a lengthy investigation, the root cause was found to be related to the 1s delay requirement after the +++ command (which switches PPP back to command mode). This required by spec delay needed to be increased by at least 4ms. We'll set this delay to 1100ms based on some feedback from Quectel.
  • Additionally, during testing it was rarely seen that the PPP_ECHO_REQUEST_ATTEMPTS could sometimes be higher than 3 but lower than 10 and still work. So we'll bump this up to 10 just to avoid a cold boot in that case, which would add another ~30s to the ~3s delay of trying all 3 attempts and failing. If it should ever exceed 10 attempts, it will take about ~40s to boot, but I personally have not seen this occur in over 1000 warm boots.

Steps to Test

  • Included below is a diff to speed up testing of the slo/connect_time test. It can be run locally in a loop, and should not fail, nor should any of the warm boot times be anywhere near 30s. Typically they are in the 6-10s range.
connect_time_test.diff
diff --git i/user/tests/integration/slo/connect_time/connect_time.cpp w/user/tests/integration/slo/connect_time/connect_time.cpp
index aa398dec2..9732f1e1f 100644
--- i/user/tests/integration/slo/connect_time/connect_time.cpp
+++ w/user/tests/integration/slo/connect_time/connect_time.cpp
@@ -160,7 +160,7 @@ size_t serializeStatsAsJson(char* buf, size_t size) {
 }
 
 bool testCloudConnectTimeFromColdBoot() {
-    if (stats.coldBootCount >= CONNECT_COUNT) {
+    if (stats.coldBootCount >= 1) {
         return false;
     }
     const auto t0 = millis();
@@ -315,13 +315,13 @@ void loop() {
     }
 
 DEFINE_COLD_BOOT_TEST(01)
-DEFINE_COLD_BOOT_TEST(02)
-DEFINE_COLD_BOOT_TEST(03)
-DEFINE_COLD_BOOT_TEST(04)
-DEFINE_COLD_BOOT_TEST(05)
-DEFINE_COLD_BOOT_TEST(06)
-DEFINE_COLD_BOOT_TEST(07)
-DEFINE_COLD_BOOT_TEST(08)
+// DEFINE_COLD_BOOT_TEST(02)
+// DEFINE_COLD_BOOT_TEST(03)
+// DEFINE_COLD_BOOT_TEST(04)
+// DEFINE_COLD_BOOT_TEST(05)
+// DEFINE_COLD_BOOT_TEST(06)
+// DEFINE_COLD_BOOT_TEST(07)
+// DEFINE_COLD_BOOT_TEST(08)
 
 DEFINE_WARM_BOOT_TEST(01)
 DEFINE_WARM_BOOT_TEST(02)
diff --git i/user/tests/integration/slo/connect_time/connect_time.spec.js w/user/tests/integration/slo/connect_time/connect_time.spec.js
index 276213f4a..ccb5b3b0d 100644
--- i/user/tests/integration/slo/connect_time/connect_time.spec.js
+++ w/user/tests/integration/slo/connect_time/connect_time.spec.js
@@ -54,7 +54,7 @@ before(function() {
 
 // TODO: The test runner doesn't support resetting the device in a loop from within a test
 // FIXME: it does now, the tests need to be fixed
-for (let i = 1; i <= CONNECT_COUNT; ++i) {
+for (let i = 1; i <= 1; ++i) {
 	test(`cloud_connect_time_from_cold_boot_${i.toString().padStart(2, '0')}`, async () => {
 		// dump any failure logs from this test
 		let testLog = await fetchLog();

@technobly technobly force-pushed the fix/ppp-resume-on-warm-boot branch from 7b5690e to b16eac2 Compare May 28, 2024 18:10
@scott-brust scott-brust merged commit 1c34048 into develop May 28, 2024
13 checks passed
@scott-brust scott-brust deleted the fix/ppp-resume-on-warm-boot branch May 28, 2024 18:47
@scott-brust scott-brust mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants