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: Remove singleton pattern #6692

Merged
merged 2 commits into from Apr 24, 2018

Conversation

Projects
None yet
6 participants
@kivaisan
Contributor

kivaisan commented Apr 20, 2018

Description

LoRaMacCrypto was the last source that was not yet converted to a C++ class. Since that is now a C++ class, also singleton pattern can be removed from LoRa stack. This will simplify the code and enable easier tracking of memory consumption and further refactorings.

Pull request type

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

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 20, 2018

0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
LoRaMacCrypto::LoRaMacCrypto()
{
memset(mic_block_b0, 0, sizeof(mic_block_b0));

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 20, 2018

Contributor

Could avoid explicit memsets by zero-initting arrays like any other element:

LoRaMacCrypto::LoRaMacCrypto() : mic_block_b0(), a_block(), ...

You will still need to manually set the non-zero elements though (with current C++03).

This comment has been minimized.

@kivaisan

kivaisan Apr 20, 2018

Contributor

Ok. I'll change this.

@@ -70,7 +70,8 @@ class LoRaWANStack: private mbed::NonCopyable<LoRaWANStack> {
} device_states_t;
public:
static LoRaWANStack& get_lorawan_stack();
LoRaWANStack();
~LoRaWANStack();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 20, 2018

Contributor

I can't see any reason to be defining an empty destructor here. Yes, it's an existing pattern, but tidy up?

This comment has been minimized.

@kivaisan

kivaisan Apr 20, 2018

Contributor

Ok, I'll remove the empty destructor as this class is not supposed to be herited.

int encrypt_payload(const uint8_t *buffer, uint16_t size, const uint8_t *key,
uint32_t address, uint8_t dir, uint32_t seq_counter,
uint8_t *enc_buffer);
#define LORAMAC_MIC_BLOCK_B0_SIZE 16

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 20, 2018

Contributor

Not a fan of this define - it's uniquely specified in one place - sizeof mic_block_b0. You can just use that to reference, and the size of the mic_block is obviously where it's defined: mic_block_b0[16].

(I often try to avoid excessive defines, having been traumatised by a source file that had 100 lines like this:

HW_REG_A = REG_A_INIT_VALUE;
HW_REG_B = REG_B_INIT_VALUE;

Yay, pointless indirection. Don't need a define for a constant that occurs in exactly 1 place, unless you need to expose it as a config option.)

int decrypt_payload(const uint8_t *buffer, uint16_t size, const uint8_t *key,
uint32_t address, uint8_t dir, uint32_t seq_counter,
uint8_t *dec_buffer);
#define LORAMAC_AES_CMAC_KEY_LENGTH 16

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 20, 2018

Contributor

On a similar note, this wouldn't be needed if you passed the key length as a parameter to compute_mic, and that's sizeof nwk_skey. Would actually be a bit tighter. Or the key could actually be a proper type - a struct with a 16-byte array in it, so you can use sizeof inside compute_mic.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

Review jenkins CI failures, related

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 20, 2018

CI failure was caused by missing constructor when LoRa was built with missing mbedtls configurations. I'll fix this.

@kivaisan kivaisan force-pushed the kivaisan:remove_singleton branch 4 times, most recently from 59c1300 to ff9b116 Apr 20, 2018

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 20, 2018

@kjbracey-arm please review again.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

Please review docs failure in Travis CI

@kivaisan kivaisan force-pushed the kivaisan:remove_singleton branch from ff9b116 to 6d179f5 Apr 20, 2018

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 20, 2018

@0xc0170 For some reason Doxygen started to complain about the Semtech's header even though I didn't modify it. But this is now fixed.

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

@0xc0170 0xc0170 requested review from AnttiKauppila and hasnainvirk Apr 20, 2018

@hasnainvirk

Just a textual change required.

@@ -542,6 +544,11 @@ class LoRaMac {
*/
LoRaMacChannelPlan channel_plan;
/**
* Channel planning subsystem

This comment has been minimized.

@hasnainvirk

hasnainvirk Apr 20, 2018

Contributor

Its a crypto subsystem

kivaisan added some commits Apr 18, 2018

Lora: Change LoRaCrypto to a C++ class
In order to get rid of static variables, LoRaCrypto functionality is now
in a C++ class.
Lora: Remove singleton construction of LoRaWANStack
After changing LoRaMacCrypto as C++ class, we no longer have static variables
in LoRa implementation. Therefore singleton pattern can be removed.

@kivaisan kivaisan force-pushed the kivaisan:remove_singleton branch from 6d179f5 to 2b2ce30 Apr 20, 2018

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 20, 2018

@hasnainvirk Text fixed.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 20, 2018

Build : SUCCESS

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

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.

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented Apr 24, 2018

@0xc0170 Could you restart the test? This failure does not seem to be related to this PR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 24, 2018

/morph test

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

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 0e86fa2 into ARMmbed:master Apr 24, 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 8771 cycles (-150 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10112B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

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

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