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

LoRa: Stack cleanup #6566

Merged
merged 6 commits into from Apr 10, 2018

Conversation

Projects
None yet
6 participants
@AnttiKauppila
Contributor

AnttiKauppila commented Apr 9, 2018

Description

Refactored and cleaned up some internal code.
Tested manually by running tests locally

Pull request type

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

AnttiKauppila added some commits Apr 6, 2018

LoRa: Improved handling of region selection
- This implements IOTCELL-697
- This touches API, but does not break it, old ones still work in a same manner!

@0xc0170 0xc0170 requested a review from kivaisan Apr 9, 2018

{
if (params) {
if (is_otaa) {
// if ((params->connection_u.otaa.dev_eui == NULL) ||

This comment has been minimized.

@0xc0170

0xc0170 Apr 9, 2018

Member

dead code shall be removed

// network server that the device was disconnected or restarted.
// At the moment, this implementation does not support a non-volatile
// memory storage.
//_lw_session.downlink_counter; //Get from NVM

This comment has been minimized.

@0xc0170

0xc0170 Apr 9, 2018

Member

why are these 2 lines here?

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Apr 9, 2018

Contributor

This is a reminder for NVM support. It will be added later for LoRa

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 9, 2018

@hasnainvirk, @kivaisan, @kjbracey-arm Can you review this?

@0xc0170 0xc0170 requested review from hasnainvirk and kjbracey-arm Apr 9, 2018

*/
void join_by_abp(const lorawan_connect_abp_t& abp_join);
lorawan_status_t join(bool is_otaa = true);

This comment has been minimized.

@kivaisan

kivaisan Apr 9, 2018

Contributor

I think is_otaa should not have a default value. Defaulting to OTAA can enable easy mistake in calling this method.

This comment has been minimized.

@kivaisan

kivaisan Apr 9, 2018

Contributor

The same comment applies to prepare_join method as well.

#include "lorawan/lorastack/phy/LoRaPHYUS915Hybrid.h"
#define LoRaPHY_region LoRaPHYUS915Hybrid
#else
#error "Unsupported region, check your configuration."

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 9, 2018

Contributor

Unfortunately this won't work, because any unknown identifier will be interpreted as 0, so it will pass the #if MBED CONF_LORA_PHY == EU868 test.

You need the identifiers you recognise to be non-zero. Which is a bit unfortunate if you're trying to be backwards-compatible with having a meaning for 0.

Removed default values from internal functions
- Also removed useless else from loraphy_target.h
@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 9, 2018

Changed according to Kimmo's and Kevin's comments

#define KR920 7
#define US915 8
#define US915_HYBRID 9

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 9, 2018

Contributor

These end up permanently defined for any application code including LoRaWANStack.h. Again, maybe some concatenation could be done to allow short IDs in the JSON but not clutter C namespace. Could be done in future.

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 9, 2018

Region handling is now improved and also made sure it is still backwards compatible

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 9, 2018

@0xc0170 Can you trigger the build for this?

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 10, 2018

@0xc0170 Can you merge this in now?

@0xc0170 0xc0170 merged commit 5e62d17 into ARMmbed:master Apr 10, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9555 cycles (+281 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@AnttiKauppila AnttiKauppila deleted the AnttiKauppila:stack_cleanup branch Apr 10, 2018

@ARMmbed ARMmbed deleted a comment from kjbracey-arm Apr 12, 2018

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