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

Cellular fixes #6691

Merged
merged 7 commits into from Apr 27, 2018

Conversation

Projects
None yet
5 participants
@mirelachirica
Contributor

mirelachirica commented Apr 20, 2018

Description

Added hex string format reading support to AT handler to prevent hex buffer allocations in upper layers. Other minor fixes self explained by commit comments.

Pull request type

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

This comment has been minimized.

Contributor

mirelachirica commented Apr 20, 2018

@jarvte and @AriParkkila please review this

delete temp;
temp = _head;
}
_tail=NULL;

This comment has been minimized.

@jarvte

jarvte Apr 20, 2018

Contributor

unnecessary set to NULL in destructor

@@ -88,6 +99,7 @@ template <class T> class CellularList
delete temp;
temp = _head;
}
_tail=NULL;

This comment has been minimized.

@jarvte

jarvte Apr 20, 2018

Contributor

Should you add
_head = NULL

This comment has been minimized.

@mirelachirica

mirelachirica Apr 23, 2018

Contributor

NULL _head breaks the while loop, so it is NULL already

@@ -511,6 +513,67 @@ ssize_t ATHandler::read_string(char *buf, size_t size, bool read_even_stop_tag)
return len;
}

ssize_t ATHandler::read_hex_string(char *buf, size_t size)

This comment has been minimized.

@jarvte

jarvte Apr 20, 2018

Contributor

A lot similarities with read_string, can we avoid copy-paste?


using namespace mbed;
using namespace events;
using namespace mbed_cellular_util;

This comment has been minimized.

@0xc0170

0xc0170 Apr 20, 2018

Member

where is this coming from? Why is it not mbed::cellular_util rather?

This comment has been minimized.

@mirelachirica

mirelachirica Apr 20, 2018

Contributor

It has been there like that when CellularUtil was introduced. Just using it as it was defined. Do I need to change it?

This comment has been minimized.

@0xc0170

0xc0170 Apr 20, 2018

Member

OK, not in this PR, but I would definitely put it under mbed namespace if needed.

@0xc0170 0xc0170 added the needs: work label Apr 20, 2018

@jarvte

jarvte approved these changes Apr 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 25, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Apr 25, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 25, 2018

Build : SUCCESS

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

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 25, 2018

@kegilbert @cmonr Can you review the latest test failure? From the logs, seems like some directories were not created and caused exceptions. however build was executed

Update:

/morph test - removing this job from the queue

@mirelachirica mirelachirica force-pushed the mirelachirica:cellular_fixes branch from 5916ed3 to c0629c8 Apr 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 25, 2018

ah wait, another rebase - that could cause it.

@mirelachirica Please let us know if you are planning to rebase or once you do !

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Apr 25, 2018

I just rebased after seen the failure, to make sure branch is up to date.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 25, 2018

It's fine as long we are aware of rebase to be done/or just done.
I was just reviewing failures and that rebase might have caused it . Then I almost re-triggered new one as github did not show me immediately the rebase here. Restarting now

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 25, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 25, 2018

Test : FAILURE

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

0xc0170: This is the old one before the rebase (waiting for the real one, the last one)

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 27, 2018

Making a note here. Marking this as for 5.9.0-rc1 even though this is marked as a fix, and the commits are characteristic of fixes.

An automatic analysis of the files this PR is modifying indicates that all of their recent modifications are targeted for 5.9.0-rc1 release.

@cmonr cmonr merged commit 380973a into ARMmbed:master Apr 27, 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 9564 cycles (-620 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 27, 2018

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