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

Stack refactoring #6411

Merged
merged 26 commits into from Apr 4, 2018

Conversation

Projects
None yet
6 participants
@AnttiKauppila
Contributor

AnttiKauppila commented Mar 21, 2018

Description

set_device_class() function added to LoRaWANInterface class to switch between classes at runtime

  • Mostly refactoring done.
  • Some small bug fixes done.
  • Green tea tests have been run manually in local machine

This is Mbed OS 5.9 content

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

kivaisan and others added some commits Feb 15, 2018

Changed RegionNextChannel function in order to return LoRaMacStatus_t…
… instead of a boolean

Removed the while loop checking the return value from set_next_channel
(GitHub Issue Lora-net/LoRaMac-node#357)

The new return values are:

LORAWAN_STATUS_OK                    : A channel has been found.
LORAWAN_STATUS_NO_FREE_CHANNEL_FOUND : No free channel has been found (AS923 and KR920 regions)
LORAWAN_STATUS_DUTYCYCLE_RESTRICTED  : No channel found due to the duty-cycle or JoinReq back-off restrictions. Trial must be delayed.
LORAWAN_STATUS_NO_CHANNEL_FOUND      : No channel has been found. Re-enabled the default channels.
Fix an issue with sequence calls.
This issue is only present for a device in class c mode, which
has perform unconfirmed uplinks.

Lora-net/LoRaMac-node#327
Fix rx slot handling
Store the rx slot temporarily. When in class C, this variable will be changed
in function OpenContinuousRx2Window.
Bug fix in RX timeout and RX error handling for class c nodes.
1. Do not stop the 2nd window timer, as it is not running.
2. Wait for the OnAckTimeout event, before setting MacDone
3. Process for class c also the 2nd window timeout part, as we do
   not have a 2nd window timer.
Update handling for functions OnRadioRxError and OnRadioRxTimout.
This is especially important for class c devices.
Update DevStatusAnd format
In the DevStatusAns format, the protocol requires RFU(7:6) value = 0
Add set_device_class API to change active device class
This API can be used to runtime change device class.

Please note that only class A and C are supported at the moment.
Trying to set class B will return LORAWAN_STATUS_UNSUPPORTED.

Fix set_device_class documentation

fix documentation
Fix reception of class C messages
- Do not put radio into sleep when message is received in class c mode
- Experimental feature for acknowledging confirmed downlink messages
Remove redundant event from timer callbacks
Since our timers are now already using events, we no longer need to
defer timer callback calls.
LoRa: LoRaWANInterface refactored.
- Only internal changes, no functionality changes
- Some minor improvements to LoRaWanStack
LoRa: LoRaPHY dependency removed from LoRaMacStack
- This is internal change, no functionality has been changed
- LoRaWanInterface cleaned up and code moved to LoRaMacStack
- Compliance code in LoRaMacStack moved to EOF
- Green tea tests have been run manually
- Doxygen updated accordingly

LoRA: reorder class members
LoRaWANStack is made independent of MAC sublayers
- Only internal changes, no API has been broke.
- Tested by manually running Green tea tests
LoRa: LoRaMAC class refactored
- Internal change only, no functional changes
- Tested by running Green tea tests manually
LoRa: Struct cleanups
- Unneeded structs removed and replaced by variables in functions
@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Mar 21, 2018

@kivaisan @kjbracey-arm Please review

Fix compilance test compilation
Fix compilation of compilance test and at the same time refactor compliance
test handler. Renamed mcps_request as test_request as it is only used for
compliance test. Also fixed a bug with null buffer in send_compliance_test_frame_to_mac.

@0xc0170 0xc0170 requested review from kivaisan and kjbracey-arm Mar 21, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 21, 2018

@AnttiKauppila Can this be done in smaller related PR ? (I set PR type in your first comment to only refactoring, but after looking at these changes, it's not just refactor. this is also introducing new API).

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Mar 22, 2018

I will start my Winter holiday today and because my previous commit took more than 2 weeks, this one got bigger and bigger meanwhile. I will be out next week and LoRa team needs this to proceed.
I already put Fix, refactor + feature in my commit details, because all of these are included in this PR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 23, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 23, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 23, 2018

Poor time for an ARM license issue...
/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 23, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 23, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 23, 2018

Build : SUCCESS

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

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 24, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

} else {
return val_in_range(datarate, phy_params.dwell_limit_datarate,
phy_params.max_tx_datarate);
if (is_datarate_supported(datarate)) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

Don't need to change it this time, but general style suggestion - use early exit for parameter validation

if (!is_datarate_supported(datarate)) {
    return false;
}

Saves indenting all the body of the function, reducing diffs and merge conflicts. This style doesn't scale well as the number of parameter checks increases.

typedef struct {
/*!
* Compiliance test request

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

Compliance

lorawan_connect_t connection_params;
//TODO: LoRaWANStack don't need to know these values, move to LoRaMac (or below)
#if (1 == MBED_CONF_LORA_OVER_THE_AIR_ACTIVATION)

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

Just #if MBED_CONF_LORA_OVER_THE_AIR_ACTIVATION, surely?

You're not doing #if (1 == defined(XXX))...

//TODO: LoRaWANStack don't need to know these values, move to LoRaMac (or below)
#if (1 == MBED_CONF_LORA_OVER_THE_AIR_ACTIVATION)
static uint8_t dev_eui[] = MBED_CONF_LORA_DEVICE_EUI;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

These should be const to save RAM.

* LORAWAN_STATUS_UNSUPPORTED is requested class is not supported,
* or other negative error code if request failed.
*/
virtual lorawan_status_t set_device_class(const device_class_t device_class) = 0;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

The const here is meaningless - remove it to avoid confusion.

stk_obj().set_lora_callbacks(callbacks);
return LORAWAN_STATUS_OK;
}
lorawan_status_t LoRaWANInterface::set_device_class(const device_class_t device_class)

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 3, 2018

Contributor

(You could keep the const here though, meaning "I don't change device_class after function entry" - just doesn't need to be in the prototype).

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 3, 2018

LoRa: Small fixes
- changed few static variables to have const
@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 3, 2018

@kjbracey-arm I have modified the code as you suggested, can you please re-review

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 3, 2018

@0xc0170 Could you trigger build for this?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 3, 2018

Build : SUCCESS

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

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.

@0xc0170 0xc0170 added ready for merge and removed needs: work labels Apr 4, 2018

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Apr 4, 2018

@cmonr Will you do the trick and merge this in?

@cmonr cmonr merged commit 75cb4d7 into ARMmbed:master Apr 4, 2018

11 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 8613 cycles (-510 cycles)
Details
travis-ci/littlefs Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 4, 2018

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

@adbridge adbridge referenced this pull request Apr 20, 2018

Merged

Lora: small fixes #6569

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