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: Fixing Hard fault in CN470 PHY layer #7806

Merged
merged 2 commits into from Sep 2, 2018

Conversation

Projects
None yet
10 participants
@hasnainvirk
Contributor

hasnainvirk commented Aug 16, 2018

Description

CN470 PHY layer has been missing an override for the selecting next TX channel and the default functionality was kicking in which was not suitable for CN470 PHY layer.
In addition to that fix, we have also introduced the concept of FSBs just like US915 and AU915.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

Dependency:

#7802

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 16, 2018

@AnttiKauppila @kivaisan @kjbracey-arm Please review.
Until "Removing US915Hybrid", is covered by #7802 . Only the last commit is to be reviewed here.

@cmonr cmonr added the needs: review label Aug 16, 2018

@cmonr cmonr requested a review from Aug 16, 2018

@imi415

This comment has been minimized.

imi415 commented Aug 17, 2018

Issue: ARMmbed/mbed-os-example-lorawan#93
It's sure that a fsb_mask is necessary in practice since a standard gateway only accept up to 8 uplink channels 👍

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 17, 2018

@imi415 the fsb-mask is there exactly for this reason. If the base station have 8 channel support, and all 96 channels are enabled in the spec, you will have 1/12 chance of hitting the right channel. Network acquisition can become very tedious. You can can use fsb-mask to mask out any unused channels.

@@ -0,0 +1,52 @@
Frequency sub-bands in US915/US915Hybrid/AU915:

This comment has been minimized.

@0xc0170

0xc0170 Aug 20, 2018

Member

@AnotherButler Can you review this file addition (is .txt the right file prefix) and also location?

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 20, 2018

Contributor

This is a helper txt file. There are many such files in other locations like LWIP.

This comment has been minimized.

@AnotherButler

AnotherButler Aug 20, 2018

Contributor

@hasnainvirk Do you think any of this information should be added to the Handbook, or do you think it's too component-specific and should stay only in the code?

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 21, 2018

Contributor

@AnotherButler Yeah. This applies to only three PHYs. Maybe we could have a little section in Handbook too. It's not general enough so I thought a .txt would do.

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 21, 2018

Contributor

@AnotherButler Oh, I lost your commit !. Sorry, I was reworking some stuff and had a force push in. Could you please be kind enough to send the Copy-edit patch again ?

@hasnainvirk hasnainvirk changed the title from Fixing Hard fault in CN470 PHY layer to LoRaWAN: Fixing Hard fault in CN470 PHY layer Aug 20, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 20, 2018

@kjbracey-arm Please review.

@kjbracey-arm

Wondering if you should be a bit more specific than -china, and the need for this immediately suggests the other fsb-mask should have some sort of corresponding suffix. But no firm objections.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:fix_for_cn470 branch 2 times, most recently from 269a1a7 to abb0206 Aug 21, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

@hasnainvirk Did you just rebase or what has changed (when you update PR, please leave a comment with the status) - helps us to be aware of changes (if this needs rereview or not).

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 22, 2018

@0xc0170 This PR is secondary. It depends upon #7802. Once that is in, this PR will get a final kick.
It will have only one commit, all other commits are actually covered in #7802. I will update this PR once again after #7802 is merged.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:fix_for_cn470 branch from abb0206 to 6f5610b Aug 27, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 27, 2018

@0xc0170 Preceding PR merged. This PR updated and ready to go. It's already reviewed.
@AnotherButler Can you please provide us with any editorial changes in FSB_Usage.txt ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 27, 2018

Please review travis event failure (cant compile the changes)

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 27, 2018

Adding override for TX ch. selection in CN470 PHY
set_next_channel() is the base function provided by LoRaPHY class and should be overridden
by the PHYs who behave differently as compared to EU868 like PHY layers.
CN470 PHY had been missing such an override.
In addition to that we have provided a parameter "fsb-mask-china" that can be used to
enforce a custom frequency sub-band of operation as most of the base stations in the market
may not support all 96 channels. Such a strategy will help in rapid network acquisition.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:fix_for_cn470 branch from 6f5610b to 5cca2f2 Aug 27, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 27, 2018

@0xc0170 Updated the PR. An internal API was simplified in some prior PR which didn't reflect in this PR.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Aug 27, 2018

Copy edit FSB_Usage.txt
Copy edit for active voice, consistent tense and precise language.
@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 28, 2018

@AnotherButler Thank you Amanda. @0xc0170 This PR is ready to go after editorial commit.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 29, 2018

@0xc0170 This is ready to go. Please entertain.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 29, 2018

@0xc0170 This is ready to go. Please entertain.

Is this for 5.10? We are now close to freeze so only 5.10 as there are still few left

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 29, 2018

@0xc0170 This is 5.10. This is a necessary patch as Chinese region is broken at the moment.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 29, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

Reshuffling the Integration Test queue. Will restart shortly.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 2, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit c7d6560 into ARMmbed:master Sep 2, 2018

14 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.0%) RAM(+0.0%)
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 was successful
Details
travis-ci/astyle Passed, 553 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9919 cycles (+769 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

@0xc0170 0xc0170 removed the ready for merge label Sep 2, 2018

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