LoRa refactoring #6279

AnttiKauppila · 2018-03-06T10:27:13Z

Description

Internal refactoring for LoRa done.

Main target of refactoring was to remove LoRaMac dependency from classes which should not have it.
Basically code has been moved to more logical places and functionality has not been modified
Doxygen documentation updated for all new functions.
Greentea tests have been run manually, results:
report_mbed-lora-tests-private-tests-lorawan-abp.html: >> Test cases: 2 passed, 0 failed
report_mbed-lora-tests-private-tests-lorawan-basic.html: >> Test cases: 18 passed, 0 failed
report_mbed-lora-tests-private-tests-lorawan-extended.html: >> Test cases: 13 passed, 0 failed
report_mbed-lora-tests-private-tests-lorawan-otaa_with_json_incorrect.html: >> Test cases: 1 passed, 0 failed
report_mbed-lora-tests-private-tests-lorawan-private_otaa.html: >> Test cases: 1 passed, 0 failed

Pull request type

AnttiKauppila · 2018-03-06T10:33:05Z

@kjbracey-arm, @kivaisan please review this.

0xc0170 · 2018-03-07T11:29:20Z

Looking at the changes, isn't this breaking change (should it be labeled as such in the PR type) ?

AnttiKauppila · 2018-03-07T12:18:30Z

Doesn't "breaking change" mean that API is somehow changed?
What is the difference between "refactor" and "breaking change"?
As I mentioned I have only changed the place of the code and verified using our test set that nothing has changed from application perspective.

adbridge · 2018-03-07T12:29:22Z

@AnttiKauppila yes breaking change means either a change in API, OR a change in behaviour from the users perspective. If this has neither then it is just refactor and can go into a patch release.

kivaisan · 2018-03-08T10:14:03Z

features/lorawan/LoRaWANStack.h

+    uint8_t _num_retry;
+    events::EventQueue *_queue;
+    bool _duty_cycle_on;
+    uint8_t _app_port;


We can save some padding bytes by moving _app_port next to _num_retry

kivaisan · 2018-03-08T10:16:20Z

features/lorawan/lorastack/mac/LoRaMacCommand.cpp

@@ -416,8 +426,8 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(uint8_t *payload,

 bool LoRaMacCommand::is_sticky_mac_command_pending()
 {
+    //DEAD CODE: mac_cmd_buf_idx_to_repeat is never set


This is actually a bug in void LoRaMacCommand::parse_mac_commands_to_repeat(). Add
mac_cmd_buf_idx_to_repeat = cmd_cnt;
to the end of that function.

kivaisan · 2018-03-08T10:39:02Z

features/lorawan/lorastack/mac/LoRaMacCommand.h

+    /**
+      * Indicates if there are any pending sticky MAC commands
+      */
+    bool sticky_mac_cmd;


We now have two similar variables (sticky_mac_cmd vs. mac_cmd_buf_idx_to_repeat). We can later check if these could be combined.

This new variable is used in add_mac_command() and merging it with mac_cmd_buf_idx_to_repeat would mean that if the value is non-zero it should be increased but how much, we need to investigate. Plus it might be potential to cause a break in functionality, so we will leave it later as you suggested.

0xc0170 · 2018-03-08T17:01:44Z

/morph build

mbed-ci · 2018-03-08T17:41:55Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-03-08T20:30:18Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-09T04:06:20Z

Test : SUCCESS

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

AnttiKauppila · 2018-03-12T11:22:15Z

@adbridge and @0xc0170 Do you have some comments still for this?

AnttiKauppila · 2018-03-15T08:58:31Z

When will this merge happen?

0xc0170 · 2018-03-15T09:31:21Z

When will this merge happen?

We will have gatekeeping most likely today (we were preparing two release candidates and faced some CI related issues).

cmonr · 2018-03-15T15:52:12Z

@AnttiKauppila It looks like this PR will need to be rebased, due to an earlier LORA-related PR being brought in.

AnttiKauppila · 2018-03-16T10:56:11Z

Conflict resolved

cmonr · 2018-03-16T15:57:17Z

@AnttiKauppila Please undo your last commit, and rebase your changes with master.

Our release process does not work well with merge commits from master within PRs.

I can start CI as soon as that's corrected.

- LoRaMacCommand does not have any external dependencies anymore - Also LoRaMacMlme is not using LoRaMacCommand anymore

- get_phy_params function was very heavy weight and needed to be refactored. - switch-case clauses have been refactored to be functions now and the complexity of the usage has been improved a lot. - There are no functional changes, this is internal only change

- Internal refactoring only, no functional changes

mbed-ci · 2018-03-18T08:00:30Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-03-18T09:15:13Z

Test : FAILURE

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

mbed-ci · 2018-03-18T10:06:00Z

Exporter Build : SUCCESS

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

AnttiKauppila · 2018-03-19T07:50:32Z

ci-morph test has now failed 3 times in a row (different reasons) and none of the failures is related to LoRa???
Where can I report issues about unstable tests?

AnttiKauppila · 2018-03-19T09:56:58Z

@0xc0170 Can this be rebuild?

0xc0170 · 2018-03-19T10:15:18Z

Where can I report issues about unstable tests?

@studavekar Please review

I'll check the rest of the queue if it has similar issues

/morph test

mbed-ci · 2018-03-19T12:19:09Z

Test : SUCCESS

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

0xc0170 · 2018-03-19T12:28:12Z

@kjbracey-arm Can you review this refactor?

AnttiKauppila · 2018-03-19T15:02:58Z

It looks like this is going to last forever, so I am planning to push 20+ commits extra on this tomorrow. Then the initial message will be changed as well, because of new feature will be introduced.

adbridge · 2018-03-19T15:11:35Z

@AnttiKauppila If you push a new feature then this moves out to 5.9

0xc0170 · 2018-03-19T15:14:46Z

It looks like this is going to last forever, so I am planning to push 20+ commits extra on this tomorrow. Then the initial message will be changed as well, because of new feature will be introduced.

It's ready, just would like @kjbracey-arm to have a last approval and goes in. We should not mix features-bugs-other things together. And adding here 20 commits won't help getting this faster.

studavekar · 2018-03-19T16:19:21Z

Where can I report issues about unstable tests?
@studavekar Please review

Flash - clock and cache test - failure - #6224 - workaround - #6340
http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6279/1240

ST Link device issues. - Fix cold boot device so communication link is restored
http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6279/1249
http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6279/1250

kjbracey

Fine for master, but I would query the 5.8.1 label regardless. Do we really want general refactoring in patch releases, if it's not fixing a specific bug and has a chance of accidentally introducing a new one?

AnttiKauppila · 2018-03-20T08:17:58Z

I am fine with 5.9 label

AnttiKauppila · 2018-03-20T13:50:30Z

@0xc0170 Who will do the merge for this?

cmonr · 2018-03-20T22:28:34Z

🖐️

AnttiKauppila force-pushed the master branch from 1a60824 to 6dde1cf Compare March 6, 2018 10:49

cmonr requested review from kjbracey and kivaisan March 6, 2018 14:22

cmonr added the needs: review label Mar 6, 2018

AnttiKauppila force-pushed the master branch from 6dde1cf to 506925f Compare March 7, 2018 10:05

kivaisan suggested changes Mar 8, 2018

View reviewed changes

kivaisan approved these changes Mar 8, 2018

View reviewed changes

0xc0170 previously approved these changes Mar 12, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Mar 12, 2018

adbridge added the release-version: 5.8.1 label Mar 12, 2018

cmonr added needs: work and removed ready for merge labels Mar 15, 2018

Antti Kauppila added 3 commits March 16, 2018 17:58

LoRa: LoRaMac need removed from LoRaMacCommand class

3c7bd1b

- LoRaMacCommand does not have any external dependencies anymore - Also LoRaMacMlme is not using LoRaMacCommand anymore

LoRa: Removed LoRaMac dependency from MIB and MLME classes

d232d3b

- Internal refactoring only, no functional changes

0xc0170 approved these changes Mar 19, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Mar 19, 2018

cmonr added needs: review and removed ready for merge labels Mar 19, 2018

kjbracey approved these changes Mar 20, 2018

View reviewed changes

0xc0170 added ready for merge release-version: 5.9.0-rc1 and removed needs: review release-version: 5.8.1 labels Mar 20, 2018

cmonr merged commit 53cb6e0 into ARMmbed:master Mar 20, 2018

cmonr removed the ready for merge label Mar 20, 2018

studavekar mentioned this pull request Mar 20, 2018

Zero delay (attach) test fails for REALTEK_RTL8195AM #6410

Closed

This was referenced Mar 21, 2018

Fix resetting of max_eirp and antenna_gain values #6401

Closed

Lora: Fix resetting of max_eirp and antenna_gain values (mbed-os 5.8 branch) #6414

Closed

LoRa refactoring #6279

LoRa refactoring #6279

Uh oh!

Conversation

AnttiKauppila commented Mar 6, 2018 • edited by adbridge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

AnttiKauppila commented Mar 6, 2018

Uh oh!

0xc0170 commented Mar 7, 2018

Uh oh!

AnttiKauppila commented Mar 7, 2018

Uh oh!

adbridge commented Mar 7, 2018

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!

0xc0170 commented Mar 8, 2018

Uh oh!

mbed-ci commented Mar 8, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 8, 2018

Uh oh!

mbed-ci commented Mar 9, 2018

Uh oh!

AnttiKauppila commented Mar 12, 2018

Uh oh!

AnttiKauppila commented Mar 15, 2018

Uh oh!

0xc0170 commented Mar 15, 2018

Uh oh!

cmonr commented Mar 15, 2018

Uh oh!

AnttiKauppila commented Mar 16, 2018

Uh oh!

cmonr commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Mar 18, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 18, 2018

Uh oh!

mbed-ci commented Mar 18, 2018

Uh oh!

AnttiKauppila commented Mar 19, 2018

Uh oh!

AnttiKauppila commented Mar 19, 2018

Uh oh!

0xc0170 commented Mar 19, 2018

Uh oh!

mbed-ci commented Mar 19, 2018

Uh oh!

0xc0170 commented Mar 19, 2018

Uh oh!

AnttiKauppila commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Mar 19, 2018

Uh oh!

0xc0170 commented Mar 19, 2018

Uh oh!

studavekar commented Mar 19, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

AnttiKauppila commented Mar 6, 2018 •

edited by adbridge

Loading

cmonr commented Mar 16, 2018 •

edited

Loading

AnttiKauppila commented Mar 19, 2018 •

edited

Loading