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

[LoRaWAN]: Adding QOS in response to LinkADRReq and fixing class C bugs #8405

Merged
merged 9 commits into from Oct 16, 2018

Conversation

Projects
None yet
5 participants
@hasnainvirk
Contributor

hasnainvirk commented Oct 12, 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.
Unit tests for missing methods and fixes for broken logic in the unit tests is also added.

Pull request type

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

@hasnainvirk hasnainvirk changed the title from Qos impl with utests to Adding QOS in response to LinkADRReq and fixing class C bugs Oct 12, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Oct 12, 2018

@AnttiKauppila, @cmonr Please review.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Oct 12, 2018

@cmonr cmonr self-requested a review Oct 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 12, 2018

@hasnainvirk Please take a look at the unittest. Looks like one of them is producing a segfault.

@cmonr cmonr added needs: work and removed needs: review labels Oct 12, 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.
Unit Test Fixes for LoRaWAN
Missing methods are added.
Logic was broken at various places which needed to be fixed.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl_with_utests branch from 18a07e5 to 3bfef7f Oct 16, 2018

@hasnainvirk hasnainvirk changed the title from Adding QOS in response to LinkADRReq and fixing class C bugs to [LoRaWAN]: Adding QOS in response to LinkADRReq and fixing class C bugs Oct 16, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Oct 16, 2018

@cmonr This commit will fix master 8f0df13. Please don't revert the PR #8299 . Merge this one instead.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Oct 16, 2018

@hasnainvirk I re-ran the unit-test check and that still failed, then tried re-build on Jenkins as @OPpuolitaival suggested , but it still failed:
rerun: https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_unittests/366/
re-build: https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_unittests/367/
am I missing something?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 16, 2018

@cmonr This commit will fix master 8f0df13. Please don't revert the PR #8299 . Merge this one instead.

Still failing, can you review the failures?

@AnttiKauppila please review

Unit test fix for cancel_send()
PR 8299 went in but correct unit tests were not there.
This commit is adding proper unittests for 8299.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:QOS_impl_with_utests branch from 8f0df13 to 9c3f73b Oct 16, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 16, 2018

/morph build

@cmonr cmonr added the needs: CI label Oct 16, 2018

@cmonr

cmonr approved these changes Oct 16, 2018

Really hoping the rest of the PR does't fail the rest of CI.

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 16, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit adf9165 into ARMmbed:master Oct 16, 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(+0 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-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 667 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8967 cycles (-757 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 needs: CI label Oct 16, 2018

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