Skip to content

LoRaWAN: Fixing Hard fault in CN470 PHY layer #7806

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
Merged
merged 2 commits into from
Sep 2, 2018
Merged

LoRaWAN: Fixing Hard fault in CN470 PHY layer #7806

merged 2 commits into from
Sep 2, 2018

Conversation

hasnainvirk
Copy link
Contributor

@hasnainvirk hasnainvirk commented Aug 16, 2018 •
edited
Loading

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
Copy link
Contributor Author

hasnainvirk commented Aug 16, 2018 •
edited
Loading

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

@cmonr cmonr requested a review from a team August 16, 2018 14:30
@imi415
Copy link
Contributor

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
Copy link
Contributor Author

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

0xc0170
0xc0170 reviewed Aug 20, 2018
View reviewed changes
features/lorawan/FSB_Usage.txt Outdated
@@ -0,0 +1,52 @@
Frequency sub-bands in US915/US915Hybrid/AU915:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 Fixing Hard fault in CN470 PHY layer LoRaWAN: Fixing Hard fault in CN470 PHY layer Aug 20, 2018
@hasnainvirk
Copy link
Contributor Author

@kjbracey-arm Please review.

kjbracey
kjbracey approved these changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

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 fix_for_cn470 branch 2 times, most recently from 269a1a7 to abb0206 Compare August 21, 2018 14:48
@0xc0170
Copy link
Contributor

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
Copy link
Contributor Author

hasnainvirk commented Aug 22, 2018 •
edited
Loading

@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
Copy link
Contributor Author

@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
Copy link
Contributor

0xc0170 commented Aug 27, 2018

Please review travis event failure (cant compile the changes)

Adding override for TX ch. selection in CN470 PHY
5cca2f2 
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
Copy link
Contributor Author

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

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

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

@hasnainvirk
Copy link
Contributor Author

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

@0xc0170
Copy link
Contributor

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
Copy link
Contributor Author

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

@cmonr
Copy link
Contributor

cmonr commented Aug 29, 2018

/morph build

@mbed-ci
Copy link

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

mbed-ci commented Aug 29, 2018

Exporter Build : SUCCESS

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

@cmonr
Copy link
Contributor

cmonr commented Aug 29, 2018

Reshuffling the Integration Test queue. Will restart shortly.

@cmonr
Copy link
Contributor

cmonr commented Sep 2, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Sep 2, 2018

Test : SUCCESS

Build number : 2742
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7806/2742

@0xc0170 0xc0170 merged commit c7d6560 into ARMmbed:master Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 left review comments

@kjbracey kjbracey kjbracey approved these changes

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

@AnotherButler AnotherButler AnotherButler approved these changes

@kivaisan kivaisan kivaisan approved these changes

Assignees
No one assigned
Labels
release-version: 5.10.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@hasnainvirk @imi415 @0xc0170 @cmonr @mbed-ci @kjbracey @AnttiKauppila @AnotherButler @kivaisan @NirSonnenschein