Skip to content

ATCmdParser: Fix OOB handling performance-wise #8598

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 31, 2018
Merged

ATCmdParser: Fix OOB handling performance-wise #8598

merged 1 commit into from
Oct 31, 2018

Conversation

VeijoPesonen
Copy link
Contributor

Description

Improve ATCmdParser performance when it comes to OOB handling.

Work done by @kjbracey-arm.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@VeijoPesonen
Copy link
Contributor Author

@kjbracey-arm please review - this code is taken from you.
@marcuschangarm for your information.

@0xc0170 0xc0170 requested a review from kjbracey October 31, 2018 07:59
@VeijoPesonen VeijoPesonen mentioned this pull request Oct 31, 2018
Merged
@kjbracey
Copy link
Contributor

Needs some description in the commit message.

If I recall/understand correctly the issue is that the process_oob check would only return immediately if no data at all on entry, or when receiving a known OOB. Any other line noise or unknown OOBs could lead to a timeout delay - read the noise or unknown OOB then timeout waiting for another line of input.

This revised version modifies the parser to recheck readable after each line end when only looking for OOBs, so it can immediate exit.

@VeijoPesonen
Copy link
Contributor Author

Previous comment copied to the commit message

@kjbracey
Fixes ATCmdParser OOB handling performance-wise
6440228 
The issue is that the process_oob check would only return immediately
if no data at all on entry, or when receiving a known OOB. Any other
line noise or unknown OOBs could lead to a timeout delay - read the
noise or unknown OOB then timeout waiting for another line of input.

This revised version modifies the parser to recheck readable after each
line end when only looking for OOBs, so it can immediate exit.
@kjbracey kjbracey mentioned this pull request Oct 31, 2018
Merged
@VeijoPesonen VeijoPesonen mentioned this pull request Oct 31, 2018
Merged
@0xc0170 0xc0170 changed the title Fixes ATCmdParser OOB handling performance-wise ATCmdParser: Fix OOB handling performance-wise Oct 31, 2018
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Build : FAILURE

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

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2018

[DEBUG] Errors: Error: L6200E: Symbol __asm___11_lwip_sha1_c____REV16 multiply defined (by BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_sha1.o and BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_md4.o).
[DEBUG] Errors: Error: L6200E: Symbol __asm___11_lwip_sha1_c____REVSH multiply defined (by BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_sha1.o and BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_md4.o).
[DEBUG] Errors: Error: L6200E: Symbol __asm___11_lwip_sha1_c____RRX multiply defined (by BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_sha1.o and BUILD/NUCLEO_F429ZI/ARM/mbed-os/features/lwipstack/lwip/src/netif/ppp/polarssl/lwip_md4.o).

I've seen this error earlier today. Does not look related to the changes here. is script failing to clean something or what else? @cmonr @studavekar Do you recognize this one?

Restarting to confirm

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Build : FAILURE

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

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2018

/morph build

@cmonr
Copy link
Contributor

cmonr commented Oct 31, 2018

#8598 (comment)

@0xc0170 This shows up every once in a while, but we're not sure why. Restarting generally solves the issue.

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Exporter Build : SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented Oct 31, 2018

Test : SUCCESS

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

@cmonr cmonr merged commit 3d09f3b into ARMmbed:master Oct 31, 2018
@VeijoPesonen VeijoPesonen deleted the fix_atcmdparser_oob_handling branch November 1, 2018 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.10.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@VeijoPesonen @kjbracey @0xc0170 @mbed-ci @cmonr @SeppoTakalo