LoRa: State machine work #6808

hasnainvirk · 2018-05-03T16:24:29Z

Description

Major contribution of this PR is to rework the state machine for our LoRaWAN stack.
State machine diagrams can be found here [for internal people].
https://armh.sharepoint.com/:f:/r/sites/IoT-Connectivity/Shared%20Documents/LoRa/Mbed%20LoRaWAN%20Stack%20State%20Diagrams/Proposal-NewStateMachine?csf=1&e=PnvvkJ

Other commits either fix a discrepancy or improve integrity of the stack.

Target -> Mbed-OS 5.9

Pull request type

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

hasnainvirk · 2018-05-03T16:26:09Z

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

kivaisan · 2018-05-04T10:16:37Z

features/lorawan/LoRaWANStack.cpp

-     LoRaMacPrimitives.mcps_confirm     = callback(this, &LoRaWANStack::mcps_confirm_handler);
-     LoRaMacPrimitives.mcps_indication  = callback(this, &LoRaWANStack::mcps_indication_handler);
-     LoRaMacPrimitives.mlme_confirm     = callback(this, &LoRaWANStack::mlme_confirm_handler);
-     LoRaMacPrimitives.mlme_indication  = callback(this, &LoRaWANStack::mlme_indication_handler);
 }


Add initialization of _tx_msg and _rx_payload.

There's really no need to be initialising buffers (or there shouldn't be). But anyway, don't forget you can initialise arrays with _rx_msg() in the initialiser list. Don't have to use memset.

Added in constructor.

kivaisan · 2018-05-04T10:19:11Z

features/lorawan/lorastack/mac/LoRaMac.cpp

@@ -125,851 +119,698 @@ LoRaMac::LoRaMac()

    _params.sys_params.adr_on = false;
    _params.sys_params.max_duty_cycle = 0;
-
-    mac_primitives = NULL;
-    ev_queue = NULL;
 }


Add initialization of _mcps_indication, _mcps_confirmation, _mlme_indication and _mlme_confirmation

kjbracey · 2018-05-04T13:59:24Z

features/lorawan/LoRaWANStack.cpp

-     LoRaMacPrimitives.mcps_confirm     = callback(this, &LoRaWANStack::mcps_confirm_handler);
-     LoRaMacPrimitives.mcps_indication  = callback(this, &LoRaWANStack::mcps_indication_handler);
-     LoRaMacPrimitives.mlme_confirm     = callback(this, &LoRaWANStack::mlme_confirm_handler);
-     LoRaMacPrimitives.mlme_indication  = callback(this, &LoRaWANStack::mlme_indication_handler);
 }


There's really no need to be initialising buffers (or there shouldn't be). But anyway, don't forget you can initialise arrays with _rx_msg() in the initialiser list. Don't have to use memset.

kjbracey · 2018-05-04T14:06:00Z

features/lorawan/system/lorawan_data_structures.h

+    LORAMAC_BUSY                    = 0x00000004,
+    LORAMAC_CONNECTED               = 0x00000008,
+    LORAMAC_USE_OTAA                = 0x00000010,
+    LORAMAC_TX_DONE                 = 0x00000020,


Mixing flags and enums is always a bit questionable, and here you don't seem to be using them as flags at all - no combinations? But I see you clearing bits and setting bits.

If this is supposed to actually be an enum of states, then probably a good idea to not make it look like a bitfield - get rid of the values, and stop doing bit manipulations on it.

Or if it is supposed to be bits, get rid of the enum, and actually use a bitfield, as you are always manipulating bits individually, and don't need any tricky masking operations.

Maybe it's a mixture of a state and some flags.

It is a bit field, members of the bitfield are identified as an enum. Actual storage is as a 32 bit unsigned bitfield.

kjbracey · 2018-05-04T14:09:02Z

features/lorawan/LoRaWANStack.h

    device_states_t _device_current_state;
    lorawan_app_callbacks_t _callbacks;
    lorawan_session_t _lw_session;
    loramac_tx_message_t _tx_msg;
    loramac_rx_message_t _rx_msg;
    uint8_t _num_retry;
+    uint32_t _ctrl_flags;


Okay, so this isn't the enum? Because the compiler's correctly stopping you doing the bit manipulations, I guess?

Flags are now made Macros for clarity.

kjbracey · 2018-05-04T14:11:20Z

features/lorawan/LoRaWANStack.cpp

+        mlme_indication_handler();
+    }
+
+    memset(_rx_payload, 0, sizeof _rx_payload);


Why the memset? memsets are one of the most expensive things you can do - you don't want this sort of thing in data paths. Although I guess this isn't very high throughput...

kjbracey · 2018-05-04T14:30:04Z

features/lorawan/LoRaWANStack.cpp

+    uint8_t pos = 0;
+    mac_hdr.value = payload[pos++];
+
+    /**


I'm still not really grasping why all this MAC functionality is being moved out of the MAC. This totally stops the MAC actually being, well, a MAC. You've now got two "stack" and "MAC" classes which actually form the MAC between them, and I'm not immediately seeing where the dividing line is conceptually.

Maybe there is no clear line, but in that case the MAC class could be eliminated and just merge with this.

This is also less robust and clear than the original MAC code it's replacing - this lost the switch and is letting through reserved packets as a result.

hasnainvirk · 2018-05-07T10:13:42Z

@kjbracey-arm @kivaisan Please review again.

hasnainvirk · 2018-05-08T07:55:45Z

@0xc0170 This needs CI, previous cliapp build failed for some reason.

0xc0170 · 2018-05-08T08:39:25Z

I reviewed the log, restarted it

kjbracey · 2018-05-08T07:59:55Z

features/lorawan/lorastack/mac/LoRaMac.cpp


-        _lora_time.start(_params.timers.mac_state_check_timer, 1);
+        default:
+            tr_error("Unsupported data frame type");


Given lack of address filtering, might this be too verbose? You'd print this for every packet you overhear from another node talking to the access point. Maybe it's okay for current testing, and non-class-C.

kivaisan · 2018-05-08T08:52:57Z

features/lorawan/LoRaWANStack.cpp

+#define IDLE_FLAG                   0x00000000
+#define TX_ONGOING_FLAG             0x00000001
+#define MSG_RECVD_FLAG              0x00000002
+#define BUSY_FLAG                   0x00000004


BUSY_FLAG bit is only set and cleared but never checked so I thinki this should be removed.

Received data buffer from radio driver should be immutable.

We went through an exercise of adding band information to any new channel being added. Default channels were looked over. This commits duly adds missing band information to default channels.

Channel plan datastructure already contains channel parameters. Extraction is not needed.

Making our LoRaWAN stack thread safe. If RTOS is not present, locks don't do anything. ScopedLock is used to automate the lock release on context expiry.

hasnainvirk · 2018-05-08T13:40:30Z

@kjbracey-arm Can you please review again ?

There had been essentially two state machines running in our stack which was too cumbersome and was not alligned in any symmetry. In this work we make sure that: * There are no callbacks from the MAC layer to Stack controller layer. * Primitives are made local to the mac layer and are presented as read-only to the stack controller layer. * Interrupt handling and processing is moved to the stack controller layer. * Reception is divided into smaller units, seperating handling of Join Accept and normal data frames. MIC gets its own unit. * Extraction of data and MAC commands from the payload is also being done now in its own method. * To ensure integrity of the stack, and sanctity of the radio payload, we copy the radio payload buffer immediately in the rx interrupt and hoist a flag that prevents another interrupt from happening for a short while when we are processing the previous packet. * If an automatic uplink is on going, we do not send a TX_DONE event to application anymore as that is logically incorrect. * state_controller() is the central engine for the state machine. To save code space and memory, we are not handling each and every state in the state_controller(). Some of the states which have no processing to be done, are explicitely set. * For all the states who need special processing, seperate methods are added. * Class A always run to completion to IDLE and CLass C always runs to completion as RECEIVING.

The asia pacific region supports custom channel planning and downlink channel request. By virtue of a mistake, this information was missing and hence a custom channel support was not working. Fixes issue ARMmbed#6783.

Structure naming in the docs was wrong.

Message flags are used in the application so the logical place for them is in lorawan_types.h and not in lorawan_data_structures.h

While configuring RX parameters for the radio, we need to feed in rx windows 1 and 2 parameters which are computed when we do the transmission. We are actually setting the physical value of the data rate rather than data rate table index and the expectation was to set the data rate index.

If the frame length is not what we are expecting, it is found to be a good practise to actually continue with what we have received rather than aborting. As we have already demodulated the packet and RX slots are used up, ther is not so much benefit in dropping that packet.

cmonr · 2018-05-08T15:18:34Z

/morph build

mbed-ci · 2018-05-08T16:08:53Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-05-08T17:26:17Z

Test : SUCCESS

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

mbed-ci · 2018-05-08T22:50:56Z

Exporter Build : SUCCESS

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

AnttiKauppila · 2018-05-09T07:15:35Z

@0xc0170 This is ready to be merged

hasnainvirk · 2018-05-09T13:13:18Z

@0xc0170 @cmonr Can it be merged now ?

hasnainvirk force-pushed the state_machine_work branch 2 times, most recently from e66ce12 to 745bc7a Compare May 4, 2018 07:05

hasnainvirk changed the title ~~State machine work~~ LoRa: State machine work May 4, 2018

hasnainvirk mentioned this pull request May 4, 2018

LoRaWAN stack - get_channel_plan( ) not working #6783

Closed

0xc0170 added the needs: review label May 4, 2018

0xc0170 requested review from AnttiKauppila and kivaisan May 4, 2018 08:50

0xc0170 added the release-version: 5.9.0-rc1 label May 4, 2018

0xc0170 requested a review from kjbracey May 4, 2018 08:50

kivaisan reviewed May 4, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels May 4, 2018

kjbracey reviewed May 4, 2018

View reviewed changes

hasnainvirk force-pushed the state_machine_work branch 2 times, most recently from 5105862 to f36492a Compare May 7, 2018 10:12

kjbracey previously approved these changes May 8, 2018

View reviewed changes

kivaisan approved these changes May 8, 2018

View reviewed changes

Hasnain Virk added 6 commits May 8, 2018 16:24

Immutable Payload from radio

e5e8e79

Received data buffer from radio driver should be immutable.

Initializing band for default channels

bec61ce

We went through an exercise of adding band information to any new channel being added. Default channels were looked over. This commits duly adds missing band information to default channels.

Remove useless extraction

0be41f0

Channel plan datastructure already contains channel parameters. Extraction is not needed.

Immutable payload pointer to LoRaMacCommand class

73e433e

Immutable payload pointer in LoRaPHY

a331c4b

Adding thread safety

a75af97

Making our LoRaWAN stack thread safe. If RTOS is not present, locks don't do anything. ScopedLock is used to automate the lock release on context expiry.

hasnainvirk dismissed kjbracey’s stale review via b6ec2c5 May 8, 2018 13:24

hasnainvirk force-pushed the state_machine_work branch 2 times, most recently from b6ec2c5 to c48782d Compare May 8, 2018 13:27

Hasnain Virk added 6 commits May 8, 2018 16:45

Adding custom channel plan support in AS923

f63ed86

The asia pacific region supports custom channel planning and downlink channel request. By virtue of a mistake, this information was missing and hence a custom channel support was not working. Fixes issue ARMmbed#6783.

Doc fix

1a0d905

Structure naming in the docs was wrong.

Moving msg flags to lorawan_types.h

cc09e44

Message flags are used in the application so the logical place for them is in lorawan_types.h and not in lorawan_data_structures.h

hasnainvirk force-pushed the state_machine_work branch from 849e667 to be04a57 Compare May 8, 2018 13:45

kjbracey approved these changes May 8, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels May 8, 2018

cmonr added ready for merge and removed needs: CI labels May 8, 2018

AnttiKauppila approved these changes May 9, 2018

View reviewed changes

kivaisan mentioned this pull request May 9, 2018

Lora: Fix rx datarate #6846

Closed

0xc0170 merged commit b5a8ace into ARMmbed:master May 9, 2018

0xc0170 removed the ready for merge label May 9, 2018

mattbrown015 mentioned this pull request Jul 5, 2018

LoRaWAN Stack No Longer Builds with -DLORAWAN_COMPLIANCE_TEST #7428

Closed

LoRa: State machine work #6808

LoRa: State machine work #6808

Uh oh!

Conversation

hasnainvirk commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

hasnainvirk commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented May 7, 2018

Uh oh!

hasnainvirk commented May 8, 2018

Uh oh!

0xc0170 commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented May 8, 2018

Uh oh!

cmonr commented May 8, 2018

Uh oh!

mbed-ci commented May 8, 2018

Triggering tests

Uh oh!

mbed-ci commented May 8, 2018

Uh oh!

mbed-ci commented May 8, 2018

Uh oh!

AnttiKauppila commented May 9, 2018

Uh oh!

hasnainvirk commented May 9, 2018

Uh oh!

Uh oh!

hasnainvirk commented May 3, 2018 •

edited

Loading