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: State machine work #6808

Merged
merged 12 commits into from May 9, 2018

Conversation

Projects
None yet
7 participants
@hasnainvirk
Contributor

hasnainvirk commented May 3, 2018

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

This comment has been minimized.

Contributor

hasnainvirk commented May 3, 2018

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:state_machine_work branch 2 times, most recently from e66ce12 to 745bc7a May 3, 2018

@hasnainvirk hasnainvirk changed the title from State machine work to LoRa: State machine work May 4, 2018

@0xc0170 0xc0170 requested review from AnttiKauppila and kivaisan May 4, 2018

@0xc0170 0xc0170 requested a review from kjbracey-arm May 4, 2018

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);
}

This comment has been minimized.

@kivaisan

kivaisan May 4, 2018

Contributor

Add initialization of _tx_msg and _rx_payload.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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.

This comment has been minimized.

@hasnainvirk

hasnainvirk May 7, 2018

Contributor

Added in constructor.

@@ -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;
}

This comment has been minimized.

@kivaisan

kivaisan May 4, 2018

Contributor

Add initialization of _mcps_indication, _mcps_confirmation, _mlme_indication and _mlme_confirmation

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

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);
}

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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.

This comment has been minimized.

@hasnainvirk

hasnainvirk May 7, 2018

Contributor

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

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;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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

This comment has been minimized.

@hasnainvirk

hasnainvirk May 7, 2018

Contributor

Flags are now made Macros for clarity.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 4, 2018

Contributor

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 hasnainvirk force-pushed the hasnainvirk:state_machine_work branch 2 times, most recently from 5105862 to f36492a May 7, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 7, 2018

@kjbracey-arm @kivaisan Please review again.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 8, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 8, 2018

I reviewed the log, restarted it

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm May 8, 2018

Contributor

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.

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

This comment has been minimized.

@kivaisan

kivaisan May 8, 2018

Contributor

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

hasnainvirk added some commits May 3, 2018

Immutable Payload from radio
Received data buffer from radio driver should be immutable.
Initializing band for default channels
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
Channel plan datastructure already contains channel parameters.
Extraction is not needed.
Adding thread safety
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 hasnainvirk force-pushed the hasnainvirk:state_machine_work branch 2 times, most recently from b6ec2c5 to c48782d May 8, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 8, 2018

@kjbracey-arm Can you please review again ?

hasnainvirk added some commits May 3, 2018

State Machine rework
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.
Adding custom channel plan support in AS923
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 #6783.
Doc fix
Structure naming in the docs was wrong.
Moving msg flags to lorawan_types.h
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
Datarate bug fix in rx windows configs
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.
Removing abort from rx in case of FL discrepency
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.

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 8, 2018

/morph build

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

@mbed-ci

This comment has been minimized.

mbed-ci commented May 8, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented May 9, 2018

@0xc0170 This is ready to be merged

@kivaisan kivaisan referenced this pull request May 9, 2018

Closed

Lora: Fix rx datarate #6846

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 9, 2018

@0xc0170 @cmonr Can it be merged now ?

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

13 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/astyle Passed, 845 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9089 cycles (-470 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

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

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