LoRaWAN: Proper size checks for link ADR cmds & correct include path in Unittests framework #9601
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hasnainvirk
changed the title
|
@hasnainvirk, thank you for your changes. |
hasnainvirk
force-pushed
the
mem_leak_link_adr
branch
from
3600df1 to
9d2cb35
Compare
hasnainvirk
changed the title
hasnainvirk
force-pushed
the
mem_leak_link_adr
branch
from
9d2cb35 to
07fcdcd
Compare
|
@cmonr We may need to trigger build again. The failure seems to be unrelated to PR. |
|
Hi @hasnainvirk , |
kjbracey
reviewed
Feb 5, 2019
Test run: SUCCESSSummary: 12 of 12 test jobs passed |
hasnainvirk
force-pushed
the
mem_leak_link_adr
branch
from
07fcdcd to
7354268
Compare
|
@kjbracey-arm Please review again. |
kjbracey
approved these changes
Feb 6, 2019
|
@0xc0170 @NirSonnenschein This can go in too. |
0xc0170
changed the title
|
No reviews from @mbed-os-wan ? At least one here! |
0xc0170
approved these changes
Feb 7, 2019
|
@0xc0170 Antti is OOO. The other LoRaWAN option in mbed-os-wan is me and I opened this PR :) |
cmonr
suggested changes
Feb 11, 2019
AnttiKauppila
approved these changes
Feb 12, 2019
added 3 commits
February 14, 2019 17:15
enable_test() seems to include CTest framework in the CMakeLists.txt but that would result in inconsistencies and CTest framework will not be able to find DartConfiguration file or MemCheck tool. Including CTest framework explicitely in the CMakeLists.txt seems to solve the issue.
In a specific branch path 'adr_settings' in link_adr_request() API, the structure adr_settings of type link_adr_params_t will be rendered uninitialized. To prevent this we initialize the construct as zero. In addition to that, to handle the case properly we should check for the command identifier and the command payload length anticipating contiguous blocks of adr commands. If we find a discrepency in size, we should abort.
Updating unit test in response to the change in the link ADR related APIs.
hasnainvirk
force-pushed
the
mem_leak_link_adr
branch
from
7354268 to
a14acfa
Compare
|
@cmonr Updated as suggested. Thanks for a having splendid pair of eyes :) |
cmonr
approved these changes
Feb 15, 2019
|
CI started |
Test run: SUCCESSSummary: 12 of 12 test jobs passed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses two issues:
i) adr_settings construct of type link_adr_params_t in link_adr_request() API in the LoRaPHYUS915 class would be render uninitialized if the program takes a specific branch path. We have fixed that by zero initializing the construct. In addition to that we are now checking for the proper payload size anticipating contiguous link ADR command blocks.
ii) Currently, mbed Unittests will fail to run memcheck tool or other tools as the DartConfiguration.Tcl file would not be found. The problem is the inclusion of Ctest framework at the wrong place. We need explicit inclusion in the CmakeLists.txt which fixes the issue,
Pull request type
Reviewers
@AnttiKauppila
@kjbracey-arm
@cmonr