Lora unittests #8233

AnttiKauppila · 2018-09-24T16:39:49Z

Description

LoRaWAN classes unittested and small fixes done while testing
Also failing unittests were fixed

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

AnttiKauppila · 2018-09-24T16:40:09Z

@hasnainvirk @kivaisan Please review

UNITTESTS/features/cellular/framework/AT/at_cellularinformation/unittest.cmake

kivaisan · 2018-09-25T07:35:20Z

features/lorawan/LoRaWANStack.cpp

-            tr_debug("state_controller: Unknown state!");
-            status = LORAWAN_STATUS_SERVICE_UNKNOWN;
+            //Because this is internal function only coding error causes this
+            MBED_ASSERT(false);


But in release profile we shouldn't return LORAWAN_STATUS_OK either. So I would keep the original code but also have the assert.

Please keep the original code here. Or just add assert at the end after the original code. As @kivaisan mentioned, release build would strip off asserts.

kivaisan · 2018-09-25T07:38:25Z

features/lorawan/lorastack/mac/LoRaMacCommand.cpp

@@ -93,8 +93,7 @@ void LoRaMacCommand::parse_mac_commands_to_repeat()
            case MOTE_MAC_LINK_CHECK_REQ: { // 0 byte payload
                break;
            }
-            default:
-                break;
+            default: {}//Cannot happen


I think assert should be added here as well. Default can only happen if MAC command is missing from case list.

Yes. Assert should happen here. I am seeing a case with 1.1 where a corrupted message is mistaken as a mac command and it falls here.

kivaisan · 2018-09-25T07:51:06Z

features/lorawan/lorastack/phy/LoRaPHY.cpp

@@ -1343,7 +1300,7 @@ lorawan_status_t LoRaPHY::add_channel(const channel_params_t *new_channel,
    // Default channels don't accept all values
    if (id < phy_params.default_channel_cnt) {
        // Validate the datarate range for min: must be DR_0
-        if (new_channel->dr_range.fields.min > phy_params.min_tx_datarate) {
+        if (new_channel->dr_range.fields.min != DR_0) {


Should this be something like this:
// Validate the datarate range for min: must be greater than min_tx_datarate
if (new_channel->dr_range.fields.min > phy_params.min_tx_datarate) {
dr_invalid = true;
}

@kivaisan Actually no. This code is correct because for default channels minimum data-rate is 0 always. So if it is not DR_0, it's invalid.

hasnainvirk · 2018-09-25T08:00:37Z

UNITTESTS/features/lorawan/lorawaninterface/unittest.cmake

+)
+
+# defines
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMBED_CONF_LORA_PHY=\"EU868\"")


Do we need CMAKE_C_FLAGS ? I think CMAKE_CXX_FLAGS would do. Everything is C++ code here.

It does not harm to have those

hasnainvirk · 2018-09-25T08:29:03Z

@AnttiKauppila A general comment: Perhaps we should change the title as the PR touches plenty of other modules inside Mbed OS. Plus we may want to do a little squashing here as we have progression of related commits which may go in one or two functional commits.

AnttiKauppila · 2018-09-25T11:27:21Z

Review comments visited and fixed where needed.

hasnainvirk · 2018-09-25T11:29:13Z

Looks good to me +1.

AnttiKauppila · 2018-09-25T11:30:04Z

@hasnainvirk We need all those stubs to make the code compile. So anything which is under UNITTESTS/ folder are not touching modules in mbed-os

AnttiKauppila · 2018-09-28T11:44:05Z

@0xc0170 This is reviewed

AnttiKauppila · 2018-10-01T12:24:22Z

@0xc0170 @cmonr Can you trigger a build for this

0xc0170 · 2018-10-01T12:28:49Z

/morph build

mbed-ci · 2018-10-01T13:16:14Z

Build : SUCCESS

Build number : 3205
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8233/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-01T21:08:46Z

Test : SUCCESS

Build number : 3005
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8233/3005

mbed-ci · 2018-10-02T07:41:20Z

Exporter Build : ABORTED

Build number : 2797
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8233/

0xc0170 · 2018-10-02T07:41:43Z

One failure there caught earlier, will restart

AnttiKauppila · 2018-10-03T05:45:53Z

@0xc0170 Has this been restarted?

0xc0170 · 2018-10-04T09:26:05Z

Blocked on #8297, exporters are failing

0xc0170 · 2018-10-05T15:02:55Z

/morph export-build

mbed-ci · 2018-10-05T19:32:37Z

Exporter Build : SUCCESS

Build number : 2842
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8233/

hasnainvirk reviewed Sep 25, 2018

View reviewed changes

UNITTESTS/features/cellular/framework/AT/at_cellularinformation/unittest.cmake Show resolved Hide resolved

kivaisan reviewed Sep 25, 2018

View reviewed changes

hasnainvirk reviewed Sep 25, 2018

View reviewed changes

0xc0170 added the needs: review label Sep 25, 2018

AnttiKauppila force-pushed the lora_unittests branch from c5e98b3 to 5b7302a Compare September 25, 2018 11:26

AnttiKauppila force-pushed the lora_unittests branch from 5b7302a to d55749c Compare September 25, 2018 11:34

kivaisan approved these changes Sep 25, 2018

View reviewed changes

AnttiKauppila force-pushed the lora_unittests branch from d55749c to 380c886 Compare September 25, 2018 16:25

Antti Kauppila added 12 commits September 27, 2018 10:49

Lorawan unittests

b646534

More lorawan unit tests added

0f83f08

LorawanInterface unit test added

595caf6

Unit test for LorawanInterface

b6848c4

LoraPhy and regions unit test skeletons added

4453848

LoRa regions unittested, stubs licences revisited

0f8cfd8

LoRaPHY unittested, small fixed done during tests

f3d402f

LoRaMacCrypto unittested

e07940d

LoRaWANStack unit tested

bd90664

LoRaMac unittested

f6ecb31

cmake Paths corrected

a54c271

equeue_stub updated and code fixed accordingly

48e2ccd

AnttiKauppila force-pushed the lora_unittests branch from 380c886 to 48e2ccd Compare September 27, 2018 09:27

0xc0170 approved these changes Oct 1, 2018

View reviewed changes

0xc0170 added the needs: CI label Oct 1, 2018

0xc0170 removed the needs: review label Oct 1, 2018

0xc0170 added ready for merge release-version: 5.10.1 and removed needs: CI labels Oct 5, 2018

0xc0170 merged commit 943130c into ARMmbed:master Oct 5, 2018

0xc0170 removed the ready for merge label Oct 5, 2018

AnttiKauppila deleted the lora_unittests branch October 8, 2018 07:58

Lora unittests #8233

Lora unittests #8233

Uh oh!

Conversation

AnttiKauppila commented Sep 24, 2018

Description

Pull request type

Uh oh!

AnttiKauppila commented Sep 24, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Sep 25, 2018

Uh oh!

AnttiKauppila commented Sep 25, 2018

Uh oh!

hasnainvirk commented Sep 25, 2018

Uh oh!

AnttiKauppila commented Sep 25, 2018

Uh oh!

AnttiKauppila commented Sep 28, 2018

Uh oh!

AnttiKauppila commented Oct 1, 2018

Uh oh!

0xc0170 commented Oct 1, 2018

Uh oh!

mbed-ci commented Oct 1, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 1, 2018

Uh oh!

mbed-ci commented Oct 2, 2018

Uh oh!

0xc0170 commented Oct 2, 2018

Uh oh!

AnttiKauppila commented Oct 3, 2018

Uh oh!

0xc0170 commented Oct 4, 2018

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Uh oh!

Uh oh!