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: Remedy for issue #7230 #7445

Merged
merged 3 commits into from Jul 13, 2018

Conversation

Projects
None yet
7 participants
@hasnainvirk
Contributor

hasnainvirk commented Jul 9, 2018

Description

Link for the original issue #7230
Unfortunately, a refactor in the past slightly changed the behaviour of re-connection and we stopped using LORAWAN_CONNECT_IN_PROGRESS return code. This was added in the first place to help the application know that the stack is waiting for a JoinAccept from the network server. There was a confusion created because of this mistake regarding re-connection. We have now devised clear rules about what can be done and how its done. Here are the rules which are now added to the API documentation:

For ABP: If everything goes well, LORAWAN_STATUS_OK is returned for first call followed by a
'CONNECTED' event. Otherwise a negative error code. Any subsequent call will return
LORAWAN_STATUS_ALREADY_CONNECTED and no event follows.

For OTAA: When a JoinRequest is sent, LORAWAN_STATUS_CONNECT_IN_PROGRESS is returned followed
by a 'CONNECTED' event when the JoinAccept is received. Otherwise a negative error code is
returned. Any subsequent call will return LORAWAN_STATUS_ALREADY_CONNECTED and no
event follows.

TARGET release

Mbed OS 5.10

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

hasnainvirk added some commits Jul 6, 2018

LoRaWAN: Handling re-joining when already Joined
This is a remedy for the issue #7230.
While the device is joining, LORAWAN_STATUS_CONNECT_IN_PROGRESS is returned.
However, if the device is already joined, we will return LORAWAN_STATUS_ALREADY_CONNECTED.
Updating docs
API documentation is updated to clear how the connection related return codes will
work from now on.
@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 9, 2018

@kivaisan @kjbracey-arm Please review.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 9, 2018

For OTAA: When a JoinRequest is sent, LORAWAN_STATUS_CONNECT_IN_PROGRESS is returned followed
by a 'CONNECTED' event when the JoinAccept is received. Otherwise a negative error code is
returned.

Otherwise? Does that mean if it doesn't send a JoinRequest? I guess so, but the phrasing is a bit convoluted here - "When this..., followed by X when... Otherwise..."

I may have raised this before - POSIX has an extra error code, which distinguish the two cases of "this connect request is now in progress" (EINPROGRESS) and "there was a connect already in progress, so I'm ignoring this call" (EALREADY). (And both are distinct from "this is connected, so I'm ignoring you" (EISCONN)".

Is it worth distinguishing that - what code do you currently get if this is called between JoinRequest and JoinAccept event?

Streamlining connect() API with posix like retcodes
For ABP: First call to connect() or connect(params) will return LORAWAN_STATUS_OK
         and a CONNECTED event will be sent. Any subsequent call will return
         LORAWAN_STATUS_ALREADY_CONNECTED (posix EISCONN) and no event is generated.

FOR OTAA: First call to connect() or connect(params) will return LORAWAN_STATUS_CONNECT_IN_PROGRESS
          and a CONNECTED event will be sent whenever the JoinAccept is received. If the application
          calls connect again before receiving the CONNECTED event, LORAWAN_STATUS_BUSY will be returned.
          After the CONNECTED event is dispatched, any subsequent call to connect() or connect(params) API
          will be returned with LORWAN_STATUS_ALREADY_CONNECTED.

No new parameters are accepted after the first call. The application must disconnect before making
a connect() call with new parameters.
@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 10, 2018

@kjbracey-arm Please review again.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 11, 2018

@kjbracey-arm @cmonr @0xc0170 Please review.

@cmonr cmonr requested review from cmonr and 0xc0170 Jul 11, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 12, 2018

@0xc0170 Needs CI here.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:issue_7230 branch from 1ceeb41 to f0844b4 Jul 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 12, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jul 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 12, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-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 c5ba97f into ARMmbed:master Jul 13, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9014 cycles (-1147 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 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Jul 13, 2018

@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Jul 16, 2018

@hasnainvirk When there is something wrong with the LoRaWAN session parameters the 'JOIN_FAILURE' event occurs after the first join request transmission. This surprised me, I expected the 'JOIN_FAILURE' after 12 join attempts.

Immediately after the 'JOIN_FAILURE' I see another transmission but then that's it. In other words, I can't see the 12 join attempts that I would expect.

I thought I'd seen a discussion about the expected behaviour of 'JOIN_FAILURE' somewhere but I can't find it now.

I'm using 38744b9.

@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Jul 16, 2018

I think the problem is caused by LoRaWAN: Handling re-joining when already Joined where the return value of LoRaMac::send_join_request() changes from LORAWAN_STATUS_OK to LORAWAN_STATUS_CONNECT_IN_PROGRESS.

The following change meant the join got retried again:

diff --git a/features/lorawan/lorastack/mac/LoRaMac.cpp b/features/lorawan/lorastack/mac/LoRaMac.cpp
index 2a925744d..6d7ed946b 100644
--- a/features/lorawan/lorastack/mac/LoRaMac.cpp
+++ b/features/lorawan/lorastack/mac/LoRaMac.cpp
@@ -772,7 +772,7 @@ bool LoRaMac::continue_joining_process()
     }

     // Schedule a retry
-    if (handle_retransmission() != LORAWAN_STATUS_OK) {
+    if (handle_retransmission() != LORAWAN_STATUS_CONNECT_IN_PROGRESS) {
         return false;
     }
@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 16, 2018

@mattbrown015 You are right. My local branch had the code correctly as you have shown. For some reason my PR still had a check for OK status.

@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Jul 16, 2018

@hasnainvirk Great! Relatively easy to sort out then. :-)

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jul 16, 2018

@mattbrown015 I have opened a PR for the fix. handle_retransmission() is shared between both Join and Data paths that's why the confusion happened. #7520

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2018

@mattbrown015 thanks for bringing this up

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

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