Skip to content

LoRaWAN: Adding acquisition of metadata, backoff and a cancel_send() API #6910

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

Merged
merged 10 commits into from
May 25, 2018

Conversation

hasnainvirk
Copy link
Contributor

@hasnainvirk hasnainvirk commented May 15, 2018

Description

This PR adds new APIs to our Mbed LoRaWAN stack which facilitate the application
in acquiring metadata relating to transmission or reception as well as an API to cancel
an outstanding outgoing message.

Dependency: #6901

Target: Mbed OS 5.9

Internal ref: IOTCELL-575

Pull request type

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

@hasnainvirk
Copy link
Contributor Author

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

@0xc0170
Copy link
Contributor

0xc0170 commented May 15, 2018

@hasnainvirk can you look at astyle issues - see travis astyle? You can ignore some files that are already on master (will be fixed separately). but for new code, would be nice to align

{
if (_loramac.clear_tx_pipe() == LORAWAN_STATUS_OK) {
if (_device_current_state == DEVICE_STATE_SENDING &&
_loramac.get_device_class() == CLASS_A) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only in class A?

/**
* The transmission time on air of the frame.
*/
uint32_t tx_toa;
Copy link
Contributor

@kivaisan kivaisan May 16, 2018

Choose a reason for hiding this comment

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

Please reorder variables to avoid padding

* The SNR for the received packet.
*/
uint8_t snr;
} lorawan_rx_metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reorder variables to avoid padding

/**
* Frame pending status.
*/
uint8_t fpending_status;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably fpending bit is not needed. If auto uplink is disabled, stack sends UPLINK_REQUIRED event.

@hasnainvirk
Copy link
Contributor Author

@0xc0170 Style corrections for remaining item could be the topic of another PR. I rectified only those style conflicts pertaining to this PR.

@hasnainvirk
Copy link
Contributor Author

@kjbracey-arm Can you please review again ? I added a few commits.

@kjbracey
Copy link
Contributor

If you're basing on #6901, going to have to wait for its review before any merge of this is possible.

@hasnainvirk
Copy link
Contributor Author

hasnainvirk commented May 16, 2018

@kjbracey-arm reviewing doesn't hurt :) I know we have to wait for #6901

kjbracey
kjbracey previously approved these changes May 16, 2018
kivaisan
kivaisan previously approved these changes May 16, 2018
@hasnainvirk
Copy link
Contributor Author

@0xc0170 This needs CI. Failed for some unrelated issue in the previous run.

@cmonr
Copy link
Contributor

cmonr commented May 21, 2018

@hasnainvirk We can start CI once this is rebased. Some PRs recently came into master, causing conflicts.

A rebase will also relaunch pr-head, whose issues have been resolved earlier today.

Hasnain Virk added 4 commits May 24, 2018 15:54
In Class C, rx timeout does not take place for RX2 windows, so if we have
not received anything, we would be retrying but if the no. of retries are
maxed out, and we have not recieved anything yet, we need a mechanism to
tell the upper layer that this has happened.
The scope of style corrections is local to this PR only.
Metadata APIs should return an error if the stack is not initialized yet.
General stability improvements are performed.
A flag is added if a Class C RX2 window is open.
We shouldn't open it again if its already opened.

TX_CRYPTO_ERROR is renamed to CRYPTO_ERROR.
Keeping TX_CRYPTO_ERROR for backwards compatibility.
@hasnainvirk
Copy link
Contributor Author

@kivaisan @kjbracey-arm Please review the latest commit. @cmonr Now the CI can be run on this.

@cmonr
Copy link
Contributor

cmonr commented May 25, 2018

Going to run since CI is just about empty at the moment. Normally we'd wait until at least one reviewer gives the ok, but both mentioned reviewers were already out but the time this PR was ready for review.

/morph build

@mbed-ci
Copy link

mbed-ci commented May 25, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 25, 2018

Copy link
Contributor

@kivaisan kivaisan left a comment

Choose a reason for hiding this comment

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

I don't think we need a flag for RX2 window in class C.
As discussed, there are three cases where RX2 is opened:

  • When switching from class A to C
  • After TX_DONE
  • After RX1

So for me it looks like there is a bug if we try to reopen it while it is already open. Please investigate this more.

@mbed-ci
Copy link

mbed-ci commented May 25, 2018

@cmonr
Copy link
Contributor

cmonr commented May 25, 2018

@hasnainvirk Please take a look at @kivaisan's comment.

@adbridge
Copy link
Contributor

After a discussion with @ChiefBureaucraticOfficer and the fact that Kevin has approved changes and it is unlikely there will be anyone in Finland able to comment on this now before RC1 codefreeze. We will take it in as is.
@hasnainvirk Please consider the comments made by @kivaisan and submit a PR for 5.9-rc2 to fix if required.

@cmonr cmonr merged commit c0895cb into ARMmbed:master May 25, 2018
@hasnainvirk
Copy link
Contributor Author

@adbridge Understood.

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.

7 participants