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

[IOTCELL-741] Separating public data structures #6588

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
7 participants
@hasnainvirk
Contributor

hasnainvirk commented Apr 10, 2018

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

This comment has been minimized.

Contributor

hasnainvirk commented Apr 10, 2018

@AnttiKauppila @kivaisan Please review.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:base_structs branch from 11e5235 to 47ac15c Apr 10, 2018

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

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

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Apr 10, 2018

Contributor

remove lorawan/ from include

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:base_structs branch from 47ac15c to bad7b89 Apr 10, 2018

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

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

This comment has been minimized.

@kivaisan

kivaisan Apr 10, 2018

Contributor

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 hasnainvirk force-pushed the hasnainvirk:base_structs branch from bad7b89 to 4c63cdc Apr 10, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 11, 2018

@AnttiKauppila @kivaisan This needs review

RX_TIMEOUT,
RX_ERROR,
JOIN_FAILURE,
} lorawan_event_t;

This comment has been minimized.

@kivaisan

kivaisan Apr 11, 2018

Contributor

Please move description of events from add_app_callbacks().

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:base_structs branch from 4c63cdc to 998f984 Apr 11, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 11, 2018

@AnttiKauppila @kivaisan please review again.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

/morph build

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

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 12, 2018

@0xc0170 please review.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 12, 2018

@kjbracey-arm Please review.

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 12, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 12, 2018

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 hasnainvirk force-pushed the hasnainvirk:base_structs branch from 998f984 to f074682 Apr 12, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 12, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

/morph build

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 12, 2018

@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

This comment has been minimized.

mbed-ci commented Apr 12, 2018

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

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

@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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

[IOTCELL-741] Separating public data structures
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 hasnainvirk dismissed stale reviews from 0xc0170 and kjbracey-arm via c34b5e6 Apr 13, 2018

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:base_structs branch from f074682 to c34b5e6 Apr 13, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 13, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 13, 2018

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

This comment has been minimized.

Contributor

hasnainvirk commented Apr 13, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 13, 2018

@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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 16, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 16, 2018

@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

This comment has been minimized.

Contributor

hasnainvirk commented Apr 17, 2018

@0xc0170 @cmonr Can this get CI please ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

@0xc0170 @cmonr Can this get CI please ?

I scheduled one few minutes ago, waiting for the result

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Apr 17, 2018

@0xc0170 This can proceed with merge.

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

@cmonr cmonr merged commit 4522405 into ARMmbed:master Apr 17, 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 8549 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
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 17, 2018

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