Skip to content
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.

Sign up for GitHub

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

Jump to bottom

lorawan: Fix Join Request retransmission timing (Interop test) #15227

Merged
merged 1 commit into from
Feb 16, 2022
Merged

lorawan: Fix Join Request retransmission timing (Interop test) #15227

merged 1 commit into from
Feb 16, 2022

Conversation

zul00
Copy link
Contributor

@zul00 zul00 commented Feb 14, 2022

Summary of changes

Fixes #15226

On interop test 1.2.2.4, Join Request retransmission is expected to be 6 s + worst case air transmission. This delay is to accommodate for JoinAccept through RX2.

The call_in in process_reception_timeout of RX2-window adds 500 ms delay between RX2 symbol-interrupt-timeout and the next join request retransmission. This is an isolated change and only affect the retransmission of Join Request.

Adding this delay improves the chance of succeeding test 1.2.2.4 (subset of 1.2.2)

Impact of changes

The retransmission of Join Request will be delayed by 500 ms. This modification improve the chance of succeeding Interop Test 1.2.2.

Migration actions required

Documentation

None

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@hasnainvirk, as you were the original maintainer of it, can you give your opinion on it?

@zul00
Copy link
Contributor Author

zul00 commented Feb 14, 2022

Below is the screenshot of three test results where we passed it 3 times in a row:

image

Below are some detail from one of the test.
image

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Feb 14, 2022
@ciarmcom ciarmcom requested a review from a team February 14, 2022 11:00
@ciarmcom
Copy link
Member

@zul00, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@zul00
Fixed Join Request retransmission timing (Interop test)
61f8374 
On interop test 1.2.2.4, Join Request retransmission is expected to be 6
s + worst case air transmission. This delay is to accommodate for
JoinAccept through RX2.

The `call_in` in process_reception_timeout of RX2-window adds 500 ms
delay between RX2 symbol-interrupt-timeout and the next join request
retransmission. This is an isolated change and only affect the
retransmission of Join Request.

Adding this delay improves the chance of succeeding test
1.2.2.4 (subset of 1.2.2)
@zul00 zul00 force-pushed the fix/lora_timing/join_req_retransmission/call_in branch from c0542a7 to 61f8374 Compare February 14, 2022 11:07
0xc0170
0xc0170 reviewed Feb 14, 2022
View reviewed changes
connectivity/lorawan/source/LoRaWANStack.cpp
state_controller(DEVICE_STATE_JOINING);
const int ret = _queue->call_in(
500, this, &LoRaWANStack::state_controller, DEVICE_STATE_JOINING);
MBED_ASSERT(ret != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lot of errors in this stack are reported just via tr_error. Should this be the same?
or just use error() instead of assert in this case

Copy link
Contributor Author

@zul00 zul00 Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other locations, failure from EventQueue::call which indicates failure in allocating the event, would be handled by assertion. for example:

mbed-os/connectivity/lorawan/source/LoRaWANStack.cpp

Lines 514 to 516 in 61f8374

const int ret = _queue->call(this, &LoRaWANStack::process_transmission);
MBED_ASSERT(ret != 0);
(void)ret;

So I am following the same pattern.

@ciarmcom ciarmcom requested a review from a team February 14, 2022 15:30
@ciarmcom
Copy link
Member

@zul00, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 16, 2022

CI started

@mergify mergify bot added needs: CI and removed needs: review labels Feb 16, 2022
@zul00
Copy link
Contributor Author

zul00 commented Feb 16, 2022

@0xc0170, I see you already approved it and removed needs:review label from both of my PR. However, only one of the PR that needs to be merged, either this one or the other one #15228

In my opinion, this PR has an isolated impact compared to the other one. But, I am open to opinions from others regarding this patch.

@mbed-ci
Copy link

mbed-ci commented Feb 16, 2022

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit 2d59c75 into ARMmbed:master Feb 16, 2022
@mergify mergify bot removed the ready for merge label Feb 16, 2022
@mbedmain mbedmain added release-version: 6.16.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
component: connectivity release-version: 6.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lorawan: Join Request retransmission timing - Interop Engine compliance test
5 participants
@zul00 @ciarmcom @0xc0170 @mbed-ci @mbedmain