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: Remove max_packet_size #7318

Merged
merged 2 commits into from Jul 13, 2018

Conversation

Projects
None yet
7 participants
@AriParkkila
Contributor

AriParkkila commented Jun 25, 2018

Description

Removed get_max_packet_size() from the cellular AT stack. Now the implementation always tries to write a packet to modem without checking its size. If the size was bigger than protocol MTU or that modem can handle, then modem will return an error.

There is no change in API (except a slight delay due to extra AT command/response), because sendto() API request still returns the same error code as before.

Fixes PR #7181 and Arm internal ref IOTCELL-1089.

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@AriParkkila

This comment has been minimized.

Contributor

AriParkkila commented Jun 25, 2018

@mirelachirica @jarvte please review

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jun 25, 2018

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jun 26, 2018

The bellow CHECK should be modified to check against NSAPI_ERROR_OK. Previously the socket_send was checking size against max_packet_size which in this case was default 0, and as a result was returning NSAPI_ERROR_DEVICE_ERROR.

void Test_AT_CellularStack::test_AT_CellularStack_socket_send() ... CHECK(NSAPI_ERROR_DEVICE_ERROR == st.socket_send(sock, "addr", 4)); }

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 29, 2018

@mirelachirica Is this request for changes?

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jun 29, 2018

Yes, test_AT_CellularStack_socket_send needs to be updated.

@cmonr cmonr added needs: work and removed needs: review labels Jun 30, 2018

@0xc0170 0xc0170 changed the title from Cellular: Removed max_packet_size to Cellular: Remove max_packet_size Jul 2, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

@AriParkkila Any update?

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jul 9, 2018

@cmonr Ari is on holiday. I might look into this and do the fix.

@mirelachirica mirelachirica force-pushed the AriParkkila:cellular-max-packet-size branch from 1bfc9ec to 4d431cd Jul 11, 2018

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jul 11, 2018

Rebased and fixed the socket_send unit test.

@cmonr

cmonr approved these changes Jul 11, 2018

LGTM

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 11, 2018

/morph build

@cmonr cmonr added needs: review and removed needs: work labels Jul 11, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 11, 2018

@AriParkkila Would it be possible to get someone else from @ARMmbed/mbed-os-wan to approve the changes as well?

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 11, 2018

Build : SUCCESS

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

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 Jul 12, 2018

@ARMmbed/team-st-mcd Can you review the test failures above? We have noticed this in the recent CI runs, started few hours ago. 2 nucleo boards.

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 12, 2018

Hi Martin

same comment as #7471 (comment)

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2018

@jeromecoutant I see now. there is a dependency issue that we were not aware?

This however does not explain what I've just reproduced locally

// 2 commands run to execute the test
mbed test -m NUCLEO_F401RE -t ARM -n tests-mbed_hal-sleep -DMBED_MEM_TRACING_ENABLED=1 -DMBED_HEAP_STATS_ENABLED=1 --compile
mbedgt -n tests-mbed_hal-sleep -v -V

// results

[1531398737.41][CONN][RXD] ++ MbedOS Error Info ++
[1531398737.47][CONN][RXD] Error Status: 0x80FF0100 Code: 256 Module: 255
[1531398737.51][CONN][RXD] Error Message: Fatal Run-time error
[1531398737.53][CONN][RXD] Location: 0x8007A17
[1531398737.54][CONN][RXD] Error Value: 0x0
[1531398737.65][CONN][RXD] Current Thread: Id: 0x20002400 Entry: 0x800A0ED StackSize: 0x1000 StackMem: 0x20001400 SP: 0x20002328
[1531398737.67][CONN][RXD] -- MbedOS Error Info --
[1531398737.71][CONN][RXD] Cannot initialize RTC with LSE

Got F401RE, no changes to master (using e1df16e84 - (HEAD -> master, upstream/master) Merge pull request #7365 from jeromecoutant/PR_RTC_SHADOW (10 hours ago) - latest master

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2018

This indicates the problem for lp ticker in the rtc init . Please try to reproduce this locally and let us know. Not sure why the CI did not fail for any prior PR (I reviewed RTC shadow PR today again to make sure it was all green really)

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 12, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2018

@jeromecoutant Created tracking issue for this where we can continue #7497

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 12, 2018

@ARMmbed/mbed-os-wan Would you mind reviewing? This PR is almost ready to come in.

@mbed-ci

This comment has been minimized.

@mirelachirica

This comment has been minimized.

Contributor

mirelachirica commented Jul 13, 2018

@TeemuKultala can you review this?

@cmonr cmonr merged commit a4117f6 into ARMmbed:master Jul 13, 2018

14 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, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10014 cycles (-171 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

@AriParkkila AriParkkila deleted the AriParkkila:cellular-max-packet-size branch Sep 10, 2018

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