Skip to content

Replaced in ESP8266::_oob_* recv() with scanf() #11459

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
Sep 13, 2019
Merged

Replaced in ESP8266::_oob_* recv() with scanf() #11459

merged 1 commit into from
Sep 13, 2019

Conversation

dmaziec
Copy link
Contributor

@dmaziec dmaziec commented Sep 11, 2019 •
edited
Loading

Description

ATCmdParser::recv() should be used when we skim through data trying to look for something and ATCmdParser::scanf() is to be used when we know what we are supposed to get- as in _oob_* functions, except from _oob_connection_status()

Pull request type

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

Reviewers

@VeijoPesonen
@SeppoTakalo
@AnttiKauppila
@michalpasztamobica

Release Notes

VeijoPesonen
VeijoPesonen reviewed Sep 11, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about _oob_connection_status()?

@dmaziec dmaziec force-pushed the recv_scanf_oob branch 2 times, most recently from 6d3e350 to c0e296a Compare September 11, 2019 14:37
@dmaziec
Copy link
Contributor Author

dmaziec commented Sep 11, 2019 •
edited
Loading

How about _oob_connection_status()?

@VeijoPesonen , it is tricky to have scanf do the job for us here, because we search for strings of different length and scanf will return as soon as it matches anything (like - one letter is good enough to match a string).
recv has a workaround for this so it is better suited for usage in _oob_connection_status().

Replaced in ESP8266::_oob_* recv() with scanf()
9ddef62 
`ATCmdParser::recv()` should be used when we skim through data trying to look for something and `ATCmdParser::scanf()` is to be used when we know what we are supposed to get- as in `_oob_*` functions, except from `_oob_connection_status()`
@ciarmcom
Copy link
Member

@dmaziec1, thank you for your changes.
@AnttiKauppila @VeijoPesonen @SeppoTakalo @michalpasztamobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@VeijoPesonen
Copy link
Contributor

How about _oob_connection_status()?

@VeijoPesonen , it is tricky to have scanf do the job for us here, because we search for strings of different length and scanf will return as soon as it matches anything (like - one letter is good enough to match a string).
recv has a workaround for this so it is better suited for usage in _oob_connection_status().

Good, I hadn't realized that myself.

VeijoPesonen
VeijoPesonen approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 12, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 12, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci
Copy link

mbed-ci commented Sep 12, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@mbed-ci
Copy link

mbed-ci commented Sep 12, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 12, 2019

Is this refactor, I am more inclined to mark this as fix to be in 5.14.x ?

@VeijoPesonen
Copy link
Contributor

This is refactoring.

@0xc0170 0xc0170 merged commit f5dd7db into ARMmbed:master Sep 13, 2019
@ccli8 ccli8 mentioned this pull request Sep 20, 2019
Closed
@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc1 labels Oct 3, 2019
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
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

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@AnttiKauppila AnttiKauppila Awaiting requested review from AnttiKauppila

@SeppoTakalo SeppoTakalo Awaiting requested review from SeppoTakalo

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

Successfully merging this pull request may close these issues.
7 participants
@dmaziec @ciarmcom @VeijoPesonen @0xc0170 @mbed-ci @michalpasztamobica @adbridge