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

Adding QOS in response to LinkADRReq and fixing class C bugs #8183

Merged
merged 7 commits into from Oct 11, 2018

Conversation

Projects
None yet
7 participants
@hasnainvirk
Contributor

hasnainvirk commented Sep 19, 2018

Description

Adds QOS mechanism for unconfirmed messages as per 1.0.2 spec in response to LinkADRReq mac command. In addition to that it fixes various bugs potentially hindering class C operation.

Pull request type

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

This comment has been minimized.

Contributor

hasnainvirk commented Sep 19, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 19, 2018

Tests added here
ARMmbed/mbed-lora-tests-simulator-private#6 [Simulator test]
ARMmbed/mbed-lora-tests-private#59 [Functional HW test]

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl branch from ab24d37 to 2b2bef3 Sep 19, 2018

@kivaisan

LGTM. Just few cosmetics.

if (_device_current_state == DEVICE_STATE_JOINING) {
_device_current_state = DEVICE_STATE_AWAITING_JOIN_ACCEPT;
}
if (_device_current_state == DEVICE_STATE_SENDING) {
if (_loramac.get_mcps_confirmation()->req_type == MCPS_CONFIRMED) {
_ctrl_flags |= TX_ONGOING_FLAG;
_ctrl_flags &= ~TX_DONE_FLAG;
//_ctrl_flags &= ~TX_DONE_FLAG;

This comment has been minimized.

@kivaisan

kivaisan Sep 20, 2018

Contributor

Remove dead code

// application when RX2 windows is elapsed, i.e., in process_reception_timeout()
_ctrl_flags &= ~TX_ONGOING_FLAG;
} else {
//_ctrl_flags |= TX_DONE_FLAG;

This comment has been minimized.

@kivaisan

kivaisan Sep 20, 2018

Contributor

Dead code here too

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl branch from 2b2bef3 to 6e2bf48 Sep 20, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 20, 2018

Dead code removed, rebased.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 20, 2018

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl branch from 6e2bf48 to 64768fd Sep 20, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 20, 2018

@kivaisan @AnttiKauppila @kjbracey-arm Please review again. Added a few commits for stability improvement and rebased after few PRs went in.

@@ -889,6 +889,8 @@ void LoRaMac::on_backoff_timer_expiry(void)
void LoRaMac::open_rx1_window(void)
{
tr_debug("Opening RX1 Window");

This comment has been minimized.

@kivaisan

kivaisan Sep 20, 2018

Contributor

No. Opening RX windows is quite time critical so let's not print until the window has been opened.

}
void LoRaMac::open_rx2_window()
{
tr_debug("Opening RX2 Window, Frequency = %lu", _params.rx_window2_config.frequency);

This comment has been minimized.

@kivaisan

kivaisan Sep 20, 2018

Contributor

The same issue here.

@cmonr cmonr added needs: work and removed needs: CI labels Sep 20, 2018

hasnainvirk added some commits Sep 19, 2018

Style correction
LinkADRReq parameters and certain parameters used camel case
which is not the recommended style.
Adding check for ongoing automatic-uplink
Before going after an automatic uplink, we should check if there was an
automatic uplink already ongoing, i.e., the ack for the previous
automatic uplink cycle has not been received.
If there is we shouldn't queue the new automatic uplink and wait for
the previous Ack cycle to complete.
Adding QOS handling and fixing bugs for Class C
LinkADRReq mac command can be used by the network server to set a
certain level of QOS using NbTrans field which is applicable to
Unconfirmed traffic only for 1.0.2 spec.
This commit introduces mechanisms to facilitate this QOS. It means to
repeat an outgoing unconfirmed message NbTrans times without changing
its frame counter.

For class C, we have retired the ack_expiry_timer_for_class_c and have
replaced it with another timer which mimics the RX2 closure as in Class
A but doesn't actually close RX2 window. It's just a mechanism by which
the state machine is informed that the you can proceed forward, we have
not received anything in RX2 window either. This is needed as RX2
doesn't timeout in class C (i.e., the radio remains in continuous mode).
In addition to that we need to close any pending timers for Receive
windows after the MIC has passed and the Duplicate counter check has
also been passed.
Making sure that RX slots open after state change
After transmission we should change the state before invoking opening of
slots as we may start receiving in the rx slots and the state would
suddenly change from SENDING to RECEIVING without going through the
ACK_WAIT state (in case of CONFIRMED messages). Tests show that after
this slight adjustment, our number of ack retries have significantly
reduced.
TX post-process for CONFIRMED UL in no-reception case
The idea behind the method post_process_no_reception() is to post
process any outgoing TX but we shouldn't do that if a CONFIRMED message
is outgoing and there are still some retries left.
Proper handling of RX1 frequency in rx_config
Previously, we weren't filling in RX1 frequecny in rx_window1_config
structure. However, everything worked as in LoRaPHY::rx_config() API
there was a check which filled in correct RX1 frequency.
Now we are filling in RX1 freq. properly while we are computing
parameters for RX1 window.
Fixing coverity findings
A couple of the coverity analysis findings are being treated here. For
the rest there will be a separate PR.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl branch from 64768fd to bc976c6 Sep 21, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 21, 2018

@kivaisan @kjbracey-arm @AnttiKauppila @0xc0170 Please review again. Tests updated as well in their respective PRs. Some corner cases are covered and improvements in stability.

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Oct 4, 2018

@0xc0170 Canthis be built?

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Oct 7, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2018

/morph mbed2-build

@cmonr cmonr merged commit 5c675d3 into ARMmbed:master Oct 11, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+32 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
jenkins-ci/unittests Test job: successful
Details
travis-ci/astyle Passed, 612 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10604 cycles (+1362 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 11, 2018

cmonr added a commit to cmonr/mbed-os that referenced this pull request Oct 11, 2018

Revert "Merge pull request ARMmbed#8183 from hasnainvirk/QOS_impl"
This reverts commit 5c675d3, reversing
changes made to 2b04a02.

adbridge added a commit that referenced this pull request Oct 12, 2018

Merge pull request #8393 from cmonr/unittest-fix
Revert "Merge pull request #8183 from hasnainvirk/QOS_impl"

JarkkoPaso added a commit to JarkkoPaso/mbed-os that referenced this pull request Oct 17, 2018

adbridge added a commit that referenced this pull request Oct 19, 2018

Revert "Merge pull request #8183 from hasnainvirk/QOS_impl"
This reverts commit 5c675d3, reversing
changes made to 2b04a02.

theamirocohen added a commit to theamirocohen/mbed-os that referenced this pull request Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment