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.

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: Fix Join Request retransmission timing (Interop test) #15227

Merged

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.

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
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

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:

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lorawan: Join Request retransmission timing - Interop Engine compliance test
5 participants