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

Merging changes from ATParser towards parser unification #5212

Merged
merged 2 commits into from Oct 5, 2017

Conversation

Projects
None yet
8 participants
@SenRamakri
Contributor

SenRamakri commented Sep 27, 2017

ARM Internal Ref: IOTMORF-1232
Adding @kjbracey-arm @hasnainvirk

@geky

geky approved these changes Sep 27, 2017

platform/ATCmdParser.cpp Outdated
// Check for oob data
struct oob *oob = _oobs;
while ( oob ) {

This comment has been minimized.

@geky

geky Sep 27, 2017

Member

formatting, should be while (oob) {

* returns immediately if there is no data to process.
*
* @return true if oob data processed, false otherwise
*/

This comment has been minimized.

@geky

geky Sep 27, 2017

Member

formatting, should match comment style in file

This comment has been minimized.

@0xc0170

0xc0170 Oct 2, 2017

Member

@SenRamakri Any update or respond to this ?

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ATCmdParserChanges2 branch 2 times, most recently Sep 27, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 28, 2017

Why are you writing a separate routine ? This logic is already a part of vrecv(), check here https://github.com/ARMmbed/mbed-os/blob/master/platform/ATCmdParser.cpp#L274

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

I do feel, as I did when I saw this go into ATParser, that it was duplicating quite a lot of code. Having the API does avoid having to do recv("impossible string") to just check for oobs, which is nice though.

Could we not make it defer to recv? Maybe pass NULL as the "expected" to invoke the behaivour?

Also I think it doesn't change behaviour /enough/.

I'm wary that this leaves the timeout quite high, and - if any unrecognised OOB arrives, we end up waiting for the same timeout we would have used for a command response. Seems like we should have the timeout set lower while checking for OOB - effectively we want a "hurry up and finish the line" timeout, not a "respond to my command" timeout. Also, after a newline, we should do redo the readable check and immediately return, to avoid:

* We're readable
* We parse an OOB we don't recognise
* We sit there for 5 seconds waiting for more data

So I guess my counter proposal is something like:

  • parse_oob() sets timeout lower temporarily, calls recv(NULL)
  • recv(NULL) checks for readable at start of each line, so recv only waits for input if looking for a response or trying to complete a line
@geky

This comment has been minimized.

Member

geky commented Sep 29, 2017

So the problem this fixed is here:
https://github.com/ARMmbed/esp8266-driver/blob/44fb447c72a37fb397a413467e02208ffeddb88e/ESP8266/ESP8266.cpp#L232

The read is inside an oob callback that's called from process_oob (here). It reads in the packet and is very likely to fail if the timeout is small. Not only will you lose the packet, but the esp8266 will dump a bunch of raw data on the serial line.

So you can't set the timeout low while processing oobs, unless you're able to set it high again in the callback. But the callback will also need to return the atparser to its previous timeout.

I think only doing the readable check once was just a simple coding mistake, but it still caught the majority of issues.

You could use a flag or something to share the same code path, but at this point recv(NULL) and process_oob() are the same function with just different names. We went with process_oob() to make its intention a bit more clear.

Feel free to refactor the function, but not sure it should block the unification effort.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 2, 2017

Yeah, I guess further tweaks can wait - as this is effectively a merge, let it just be a merge. (Plus as this is an addition, can't break any existing AtCmdParser users).

That timeout of 0 that ESP8266 was setting was totally inappropriate - you do need some reasonable finite time to let the line be transferred - 0 would fall apart quite readily, as you'd never wait for an ongoing line to be finished.

Conceptually the API should probably have two timeouts - the "wait for expected string" timeout and the "when do I give up reading the current line" timeout. What you want for the OOBs is effectively the first one being zero, while the second remains small and finite as ever.

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Oct 2, 2017

Thanks for the review, Kevin. Since we all agree that the unification doesn't adversely affect anything I'll go ahead merge the unified ATCmdParser and take care of the changes being discussed later.

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ATCmdParserChanges2 branch to 3e1459b Oct 2, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 2, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 3, 2017

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 3, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 4, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1516

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 4, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 4, 2017

@hasnainvirk To be certain with the proper testing for this changeset, is morph enough?

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Oct 4, 2017

@0xc0170 It should be alright.

@theotherjimmy theotherjimmy merged commit 0dc264f into ARMmbed:master Oct 5, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 31, 2018

My proposed change finally implemented in #8598

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