[IOTCELL-741] Separating public data structures #6588

hasnainvirk · 2018-04-10T11:09:02Z

Description

Any data structure used in LoRaWANBase class should be available
in a separate header in order to make the code easy to port and
easy to read as the developer doesn't need to know about all the
internal data structures being used in Mbed LoRaWAN stack.

Pull request type

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

hasnainvirk · 2018-04-10T11:09:32Z

@AnttiKauppila @kivaisan Please review.

AnttiKauppila · 2018-04-10T11:14:09Z

features/lorawan/LoRaWANBase.h

@@ -19,7 +19,7 @@
 #ifndef LORAWAN_BASE_H_
 #define LORAWAN_BASE_H_

-#include "lorawan/system/lorawan_data_structures.h"
+#include "lorawan/base_structs.h"


remove lorawan/ from include

kivaisan · 2018-04-10T11:20:18Z

features/lorawan/LoRaWANBase.h

@@ -19,7 +19,7 @@
 #ifndef LORAWAN_BASE_H_
 #define LORAWAN_BASE_H_

-#include "lorawan/system/lorawan_data_structures.h"
+#include "lorawan/base_structs.h"


base_structs.h is not a good name as file is not just about structs but all kind of data types. Also filename should include term lora.

hasnainvirk · 2018-04-11T12:39:41Z

@AnttiKauppila @kivaisan This needs review

kivaisan · 2018-04-11T12:51:24Z

features/lorawan/lorawan_public_ds.h

+    RX_TIMEOUT,
+    RX_ERROR,
+    JOIN_FAILURE,
+} lorawan_event_t;


Please move description of events from add_app_callbacks().

hasnainvirk · 2018-04-11T13:10:20Z

@AnttiKauppila @kivaisan please review again.

0xc0170 · 2018-04-12T06:59:52Z

/morph build

hasnainvirk · 2018-04-12T07:36:20Z

@0xc0170 please review.

hasnainvirk · 2018-04-12T07:36:41Z

@kjbracey-arm Please review.

mbed-ci · 2018-04-12T07:42:49Z

Build : FAILURE

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

kjbracey · 2018-04-12T07:55:01Z

I think the naming is a little off - general assumption is normally that headers are public, unless they're in a /system subdirectory or something, so including "public" in the name is a bit weird.

Also, to make it a bit more generic than structures, often "types" is used, eg "nsapi_types.h". I'd suggest "lorawan_types.h"

hasnainvirk · 2018-04-12T08:20:30Z

@kjbracey-arm Please review again. @0xc0170 Morph build needed.

0xc0170 · 2018-04-12T08:48:46Z

/morph build

hasnainvirk · 2018-04-12T09:09:28Z

@0xc0170 Morph build failed for some BLE test. Logs are just too much to find anything useful. Can you please help me here to figure out what is happening here ?

mbed-ci · 2018-04-12T09:16:55Z

Build : SUCCESS

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

Triggering tests

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

0xc0170 · 2018-04-12T10:13:03Z

@0xc0170 Morph build failed for some BLE test. Logs are just too much to find anything useful. Can you please help me here to figure out what is happening here ?

The last build is fine after your latest changes. I restarted jenkins CI as well as there was failure not related tot this changeset I assume, please verify

mbed-ci · 2018-04-12T11:36:12Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-04-12T20:52:09Z

Test : SUCCESS

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

Any data structure used in LoRaWANBase class should be available in a separate header in order to make the code easy to port and easy to read as the developer doesn't need to know about all the internal data structures being used in Mbed LoRaWAN stack.

hasnainvirk · 2018-04-13T07:43:43Z

@kjbracey-arm @0xc0170 Please review again. Rebased after resolving conflict.

0xc0170 · 2018-04-13T08:00:48Z

/morph build

mbed-ci · 2018-04-13T08:57:51Z

Build : SUCCESS

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

Triggering tests

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

hasnainvirk · 2018-04-13T10:11:39Z

@0xc0170 Seems like CI ran out of memory. pr-head for CLI app failed. Could we start the job again ?

0xc0170 · 2018-04-13T10:35:43Z

@0xc0170 Seems like CI ran out of memory. pr-head for CLI app failed. Could we start the job again ?

This is known problem, waiting for CI infra team to fix or github. Affected few other PR. Olli can provide details

mbed-ci · 2018-04-13T11:32:31Z

Test : SUCCESS

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

mbed-ci · 2018-04-13T13:33:59Z

Exporter Build : SUCCESS

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

hasnainvirk · 2018-04-16T08:01:31Z

@0xc0170 @cmonr Needs CI again. Hope this time CI Infra team have eventually fixed the issue.

0xc0170 · 2018-04-16T08:23:58Z

@0xc0170 @cmonr Needs CI again. Hope this time CI Infra team have eventually fixed the issue.

In progress, yet not fixed. That is the only CI stage missing in this PR at the moment.

hasnainvirk · 2018-04-17T11:39:38Z

@0xc0170 @cmonr Can this get CI please ?

0xc0170 · 2018-04-17T11:46:04Z

@0xc0170 @cmonr Can this get CI please ?

I scheduled one few minutes ago, waiting for the result

hasnainvirk · 2018-04-17T12:28:09Z

@0xc0170 This can proceed with merge.

hasnainvirk force-pushed the base_structs branch from 11e5235 to 47ac15c Compare April 10, 2018 11:13

AnttiKauppila reviewed Apr 10, 2018

View reviewed changes

hasnainvirk force-pushed the base_structs branch from 47ac15c to bad7b89 Compare April 10, 2018 11:15

kivaisan reviewed Apr 10, 2018

View reviewed changes

hasnainvirk force-pushed the base_structs branch from bad7b89 to 4c63cdc Compare April 10, 2018 11:33

0xc0170 added the needs: review label Apr 10, 2018

kivaisan reviewed Apr 11, 2018

View reviewed changes

hasnainvirk force-pushed the base_structs branch from 4c63cdc to 998f984 Compare April 11, 2018 13:07

AnttiKauppila approved these changes Apr 11, 2018

View reviewed changes

0xc0170 added the release-version: 5.9.0-rc1 label Apr 11, 2018

kivaisan approved these changes Apr 12, 2018

View reviewed changes

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

hasnainvirk force-pushed the base_structs branch from 998f984 to f074682 Compare April 12, 2018 08:19

kjbracey previously approved these changes Apr 12, 2018

View reviewed changes

0xc0170 previously approved these changes Apr 12, 2018

View reviewed changes

hasnainvirk dismissed stale reviews from 0xc0170 and kjbracey via c34b5e6 April 13, 2018 07:41

hasnainvirk force-pushed the base_structs branch from f074682 to c34b5e6 Compare April 13, 2018 07:41

kjbracey approved these changes Apr 13, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Apr 17, 2018

cmonr merged commit 4522405 into ARMmbed:master Apr 17, 2018

cmonr removed the ready for merge label Apr 17, 2018

[IOTCELL-741] Separating public data structures #6588

[IOTCELL-741] Separating public data structures #6588

Uh oh!

Conversation

hasnainvirk commented Apr 10, 2018

Description

Pull request type

Uh oh!

hasnainvirk commented Apr 10, 2018

Uh oh!

AnttiKauppila Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Apr 11, 2018

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

hasnainvirk commented Apr 12, 2018

Uh oh!

hasnainvirk commented Apr 12, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Uh oh!

kjbracey commented Apr 12, 2018

Uh oh!

hasnainvirk commented Apr 12, 2018

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

hasnainvirk commented Apr 12, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Triggering tests

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Uh oh!

hasnainvirk commented Apr 13, 2018

Uh oh!

0xc0170 commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 13, 2018

Triggering tests

Uh oh!

hasnainvirk commented Apr 13, 2018

Uh oh!

0xc0170 commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 13, 2018

Uh oh!

hasnainvirk commented Apr 16, 2018

Uh oh!

0xc0170 commented Apr 16, 2018

Uh oh!

hasnainvirk commented Apr 17, 2018

Uh oh!

0xc0170 commented Apr 17, 2018

Uh oh!

hasnainvirk commented Apr 17, 2018

Uh oh!

Uh oh!

AnttiKauppila Apr 10, 2018 •

edited

Loading