Fix: GGD Test failures on ST #1616

muneebahmed10 · 2019-12-13T20:14:00Z

Description

GGD_JSONRequestGetFile() would not receive the entire file
in one call on some boards. This adds a loop to receive the
file in chunks if the request was not completed.

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed.

gkwicker

A few questions, see comments

gkwicker · 2019-12-13T22:16:26Z

libraries/freertos_plus/aws/greengrass/test/aws_test_greengrass_discovery.c

+                                                  &ulByteRead,
+                                                  &xJSONFileRetrieveCompleted,
+                                                  ulJSONFileSize );
+            } while( ( xStatus == pdPASS ) && ( xJSONFileRetrieveCompleted != pdTRUE ) && ( ulBufferSize - ulByteRead ) > 0 && ( ++ulRequestLoopCounter < ggdTestMAX_REQUEST_LOOP_COUNT ) );


Is there any way in which ulBufferSize < ulByteRead? Imagine something isn't working properly – should we check to make sure this isn't the case before continuing in this loop? See this PR for an example of where not checking for this caused a problem.

Good catch, I think checking ulBufferSize > ulByteRead is more appropriate since unsigned will always be > 0

gkwicker · 2019-12-13T22:18:32Z

libraries/freertos_plus/aws/greengrass/test/aws_test_greengrass_discovery.c

+
+            do
+            {
+                xStatus = GGD_JSONRequestGetFile( &xSocket,


Two questions:

This logic looks a little different than the other two blocks, but they seem to be doing the same thing. Is there any reason for the difference?

Since we have 3 blocks of code that look almost the same, could this operation be moved to a static utility function?

Good catch, it should be similar to the other three.

Created a static helper function per your suggestion.

cobusve · 2019-12-13T22:59:29Z

libraries/freertos_plus/aws/greengrass/test/aws_test_greengrass_discovery.c

@@ -47,6 +47,7 @@
 #define ggdTestJSON_PORT_ADDRESS_1         1234
 #define ggdTestJSON_PORT_ADDRESS_3         4321
 #define ggdTestLOOP_NUMBER                 10
+#define ggdTestMAX_REQUEST_LOOP_COUNT      2


Do we really only want 1 attempt with 1 retry? Feels like this should be more than 2?

For the specific board this PR is meant to address, two iterations are required to retrieve the document from the socket. For every other board for which these tests passed, it was done in one go. I increased it to 3, and it can be increased in the future if a new board requires more iterations.

abhidixi11 · 2019-12-20T23:32:55Z

libraries/freertos_plus/aws/greengrass/test/aws_test_greengrass_discovery.c

+                                          pulByteRead,
+                                          pxJSONFileRetrieveCompleted,
+                                          ulJSONFileSize );
+    } while( ( xStatus == pdPASS ) && ( *pxJSONFileRetrieveCompleted != pdTRUE ) && ( ulBufferSize > *pulByteRead ) && ( ++ulRequestLoopCounter < ggdTestMAX_REQUEST_LOOP_COUNT ) );


Do we have to use ggdTestMAX_REQUEST_LOOP_COUNT ? We are checking pxJSONFileRetrieveCompleted and ulBufferSize > pulByteRead, should these checks be sufficient to read the entire file and terminate if there is an error ?

With our code, no, and that's why there's no loop counter for the similar code in the library. However, since this is part of a test, I thought that we should guard against the possibility that a change to GGD_JSONRequestGetFile might break it in a way such that those variables don't get updated properly.

* Fix: GGD Test failures on ST and other boards GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed. * Add logging statement to GGD File Retrieval

* Fix: GGD Test failures (#1616) * Fix: GGD Test failures on ST and other boards GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed. * Add logging statement to GGD File Retrieval * Unapologetically deleting WiFi test cases. (#1685) * Unapologetically deleting WiFi test cases that don't do much. Test case references are left in commented out. Consider this shame mark. * uncrustify. * Deleting all references to unused references. * Update Major number for Release from 201912 to 202002 * Update checksum file for 202002.00 release Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]>

* Bringing changes for AFR Release 202002.00 (#1723) * Fix a dependency parsing bug in CMake from #1161 (#1655) In previous PR when we add a feature to disable building demos and test, we add a check to not search dependencies of aws_demos and aws_tests target when demos or tests are disabled. But we forgot to also move adding the dependencies to demos_base and tests_base inside the check. As a result, the "dependencies" from previous for loop was added to demos_base and tests_base. * Add board dir for cypress in CMake (#1684) * Merge pull request #1700 from tgsong/ota_e2e_s3_cleanup Only delete files uploaded in OTA E2E * OTA e2e scripts: Replace dots with underscores (#1717) * Update FreeRTOS-Kernel submodule branch to release-candidate Update FreeRTOS-Kernel submodule branch from V10.2.1-convergence-FreeRTOS-Source to release-candidate_10.3.0_rc3 commit id: d7550ef1a0560ceddac7c0b8bc0148272ac641a6 Co-authored-by: Tiangang Song <[email protected]> Co-authored-by: Pavan Madehalli Ranganath <[email protected]> * Variable name change from uxPendedTicks to xPendedTicks. (#1724) Due to the kernel version bump, Tracealyzer also needs to be updated. This is to fix the build failure on Windows and Microchip in this PR -- #1723 * Update FreeRTOS-Kernel submodule to V10.3.0-kernel-only (#1743) Update FreeRTOS-Kernel submodule branch from release-candidate_10.3.0_rc3 to V10.3.0-kernel-only commit id: 210b1ff * Change http echo server from 'httpbin.org' to 'postman-echo.com' (#1742) HTTPS_Client_System test Group is fails for TI when connecting to 'httpbin.org'. Connect to 'postman-echo.com' to solve this issue. * Update checksums.json file (#1744) * Update CHANGELOG.md (#1748) * Change name from "Amazon FreeRTOS" to "FreeRTOS" (#1736) * Cypress kernel dependency workaround (#1750) * From FreeRTOS kernel 10.2.0, copying over Wiced_CY port to vendor directory. * Make Cortex R4 target use the kernel files at new location. * The project file update probably won't matter, but in IDE at least you'll now get the correct source files. * Change Nordic kernel portable files path (#1751) * Change paths for Nordic test and demo projects to build with kernel kernel_branch: release-candidate_10.3.0_rc3 commit id: d7550ef1a0560ceddac7c0b8bc0148272ac641a6 * Change Cmake and IDE to use kernel portable files from Nordic SDK * Temporary fix for Renesas test/demo failure * Temporary fix for Renesas test/demo failure * Changes for 202002.00 Release (#1756) * Fix: GGD Test failures (#1616) * Fix: GGD Test failures on ST and other boards GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed. * Add logging statement to GGD File Retrieval * Unapologetically deleting WiFi test cases. (#1685) * Unapologetically deleting WiFi test cases that don't do much. Test case references are left in commented out. Consider this shame mark. * uncrustify. * Deleting all references to unused references. * Update Major number for Release from 201912 to 202002 * Update checksum file for 202002.00 release Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]> * Change uxPendedTicks to xPendedTicks in all proofs (#1758) This ensures that CBMC proofs are able to build after the name-change in commit 23839bb. * ECC key import fix. (#1760) * Add FreeRTOS_Porting_Guide.pdf and FreeRTOS_Qualification_Guide.pdf (#1764) * Change release date from 2/17/2020 to 2/18/2020 (#1769) Co-authored-by: Tiangang Song <[email protected]> Co-authored-by: Pavan Madehalli Ranganath <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]> Co-authored-by: yngki <[email protected]> Co-authored-by: Ravishankar Bhagavandas <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Kareem Khazem <[email protected]> Co-authored-by: lundinc2 <[email protected]>

* Fix: GGD Test failures (aws#1616) * Fix: GGD Test failures on ST and other boards GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed. * Add logging statement to GGD File Retrieval * Unapologetically deleting WiFi test cases. (aws#1685) * Unapologetically deleting WiFi test cases that don't do much. Test case references are left in commented out. Consider this shame mark. * uncrustify. * Deleting all references to unused references. * Update Major number for Release from 201912 to 202002 * Update checksum file for 202002.00 release Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]>

* Bringing changes for AFR Release 202002.00 (aws#1723) * Fix a dependency parsing bug in CMake from aws#1161 (aws#1655) In previous PR when we add a feature to disable building demos and test, we add a check to not search dependencies of aws_demos and aws_tests target when demos or tests are disabled. But we forgot to also move adding the dependencies to demos_base and tests_base inside the check. As a result, the "dependencies" from previous for loop was added to demos_base and tests_base. * Add board dir for cypress in CMake (aws#1684) * Merge pull request aws#1700 from tgsong/ota_e2e_s3_cleanup Only delete files uploaded in OTA E2E * OTA e2e scripts: Replace dots with underscores (aws#1717) * Update FreeRTOS-Kernel submodule branch to release-candidate Update FreeRTOS-Kernel submodule branch from V10.2.1-convergence-FreeRTOS-Source to release-candidate_10.3.0_rc3 commit id: d7550ef1a0560ceddac7c0b8bc0148272ac641a6 Co-authored-by: Tiangang Song <[email protected]> Co-authored-by: Pavan Madehalli Ranganath <[email protected]> * Variable name change from uxPendedTicks to xPendedTicks. (aws#1724) Due to the kernel version bump, Tracealyzer also needs to be updated. This is to fix the build failure on Windows and Microchip in this PR -- aws#1723 * Update FreeRTOS-Kernel submodule to V10.3.0-kernel-only (aws#1743) Update FreeRTOS-Kernel submodule branch from release-candidate_10.3.0_rc3 to V10.3.0-kernel-only commit id: 210b1ff * Change http echo server from 'httpbin.org' to 'postman-echo.com' (aws#1742) HTTPS_Client_System test Group is fails for TI when connecting to 'httpbin.org'. Connect to 'postman-echo.com' to solve this issue. * Update checksums.json file (aws#1744) * Update CHANGELOG.md (aws#1748) * Change name from "Amazon FreeRTOS" to "FreeRTOS" (aws#1736) * Cypress kernel dependency workaround (aws#1750) * From FreeRTOS kernel 10.2.0, copying over Wiced_CY port to vendor directory. * Make Cortex R4 target use the kernel files at new location. * The project file update probably won't matter, but in IDE at least you'll now get the correct source files. * Change Nordic kernel portable files path (aws#1751) * Change paths for Nordic test and demo projects to build with kernel kernel_branch: release-candidate_10.3.0_rc3 commit id: d7550ef1a0560ceddac7c0b8bc0148272ac641a6 * Change Cmake and IDE to use kernel portable files from Nordic SDK * Temporary fix for Renesas test/demo failure * Temporary fix for Renesas test/demo failure * Changes for 202002.00 Release (aws#1756) * Fix: GGD Test failures (aws#1616) * Fix: GGD Test failures on ST and other boards GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed. * Add logging statement to GGD File Retrieval * Unapologetically deleting WiFi test cases. (aws#1685) * Unapologetically deleting WiFi test cases that don't do much. Test case references are left in commented out. Consider this shame mark. * uncrustify. * Deleting all references to unused references. * Update Major number for Release from 201912 to 202002 * Update checksum file for 202002.00 release Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]> * Change uxPendedTicks to xPendedTicks in all proofs (aws#1758) This ensures that CBMC proofs are able to build after the name-change in commit 23839bb. * ECC key import fix. (aws#1760) * Add FreeRTOS_Porting_Guide.pdf and FreeRTOS_Qualification_Guide.pdf (aws#1764) * Change release date from 2/17/2020 to 2/18/2020 (aws#1769) Co-authored-by: Tiangang Song <[email protected]> Co-authored-by: Pavan Madehalli Ranganath <[email protected]> Co-authored-by: Yuhui.Zheng <[email protected]> Co-authored-by: yngki <[email protected]> Co-authored-by: Ravishankar Bhagavandas <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> Co-authored-by: Muneeb Ahmed <[email protected]> Co-authored-by: Kareem Khazem <[email protected]> Co-authored-by: lundinc2 <[email protected]>

Fix: GGD Test failures on ST

2206d44

GGD_JSONRequestGetFile() would not receive the entire file in one call on some boards. This adds a loop to receive the file in chunks if the request was not completed.

gkwicker suggested changes Dec 13, 2019

View reviewed changes

Fix unsigned comparison

633f5d6

cobusve reviewed Dec 13, 2019

View reviewed changes

Move repeated loops to helper function

9fbd834

muneebahmed10 requested review from abhidixi11 and gkwicker December 16, 2019 20:40

muneebahmed10 added 2 commits December 17, 2019 14:57

Minor comment fix

c61ca94

Add logging statement to GGD File Retrieval

f4b67bb

abhidixi11 reviewed Dec 20, 2019

View reviewed changes

gkwicker approved these changes Jan 9, 2020

View reviewed changes

abhidixi11 approved these changes Jan 9, 2020

View reviewed changes

muneebahmed10 merged commit 3ae5d3c into aws:master Jan 9, 2020

muneebahmed10 deleted the fix/ggdtests branch January 9, 2020 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: GGD Test failures on ST #1616

Fix: GGD Test failures on ST #1616

muneebahmed10 commented Dec 13, 2019 •

edited

Loading

gkwicker left a comment

gkwicker Dec 13, 2019

muneebahmed10 Dec 13, 2019

gkwicker Dec 13, 2019

muneebahmed10 Dec 14, 2019

cobusve Dec 13, 2019

muneebahmed10 Dec 14, 2019

abhidixi11 Dec 20, 2019

muneebahmed10 Dec 20, 2019

Fix: GGD Test failures on ST #1616

Fix: GGD Test failures on ST #1616

Conversation

muneebahmed10 commented Dec 13, 2019 • edited Loading

Description

Checklist:

gkwicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muneebahmed10 commented Dec 13, 2019 •

edited

Loading