ATCmdParser: merge scanf and recv functions #11720

mtomczykmobica · 2019-10-21T13:06:53Z

Description

vrecv and vscanf should be merged together. The goal is to give a configuration parameter for this function which would indicate are we trying to find a match from one line(scanf) or do we might end up processing multiple lines(recv) to find a match.

Pull request type

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

Reviewers

@michalpasztamobica
@AnttiKauppila

Release Notes

ciarmcom · 2019-10-21T15:00:34Z

@mtomczykmobica, thank you for your changes.
@AnttiKauppila @michalpasztamobica @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica

Functionally this makes perfect sense to me and reduces code duplication. I only left some minor remarks.
@VeijoPesonen , if you, as the author of this idea, had a few minutes to take a look and let us know if you see anything alarming, that would be great.

michalpasztamobica · 2019-10-21T18:54:34Z

platform/source/ATCmdParser.cpp

-}
-
-bool ATCmdParser::vrecv(const char *response, std::va_list args)
+int ATCmdParser::vrecvscanf(const char *response, std::va_list args, bool issacnf)


typo? Should be isscanf but now it is issacnf?
But why don't we use something that would describe the purpose of this flag, such as multiline or multiple_lines or something equally descriptive?

michalpasztamobica · 2019-10-21T19:14:54Z

platform/source/ATCmdParser.cpp

            // If just peeking for OOBs, and at start of line, check
            // readability
-            if (!response && j == 0 && !_fh->readable()) {
-                return false;
+            if (!issacnf && !response && j == 0 && !_fh->readable()) {


I see that old vrecv didn't have this check, but actually I don't see why we would not return an error if the file handle (that is - serial port) is not readable, no matter if we're reading single or multiple lines?
Unless there is a good reason, I suggest to return error regardless of the flag.

michalpasztamobica · 2019-10-21T19:20:22Z

platform/source/ATCmdParser.cpp

+}
+
+// Command parsing with line handling
+bool ATCmdParser::vsend(const char *command, std::va_list args)


Why was this moved? The header file has this one declared before vrecvscanf. Perhaps we could keep the header's order?

michalpasztamobica · 2019-10-21T19:25:29Z

platform/ATCmdParser.h

@@ -222,7 +222,7 @@ class ATCmdParser : private NonCopyable<ATCmdParser> {
     */
    bool recv(const char *response, ...) MBED_SCANF_METHOD(1, 2);

-    bool vrecv(const char *response, std::va_list args);
+    int vrecvscanf(const char *response, std::va_list args, bool isscnf);


I think this now deserves its own doxygen documentation. vrecv was kind of wrapped by recv and vscanf was wrapped by scanf, so they never got their own doxygen. But now the new "Frankenstein's" vrecvscanf has an extra parameter (currently with a typo isscnf, but hopefully it would become something more descriptive, such as 'multiline' or sth similar) that is not documented anywhere and also its int return value fits more under the scanf's doxygen?

VeijoPesonen · 2019-10-22T06:43:34Z

Functionally this makes perfect sense to me and reduces code duplication. I only left some minor remarks.
@VeijoPesonen , if you, as the author of this idea, had a few minutes to take a look and let us know if you see anything alarming, that would be great.

Ok, I'll check and also inform the original author of the idea :)
@kjbracey-arm , for your information if you would like to have a look.

VeijoPesonen · 2019-10-22T06:45:56Z

platform/ATCmdParser.h

@@ -222,7 +222,23 @@ class ATCmdParser : private NonCopyable<ATCmdParser> {
     */
    bool recv(const char *response, ...) MBED_SCANF_METHOD(1, 2);

-    bool vrecv(const char *response, std::va_list args);
+    /**


There is no need to change the API, the original functions could act as wrappers for the newly introduced vrecvscanf.

vrecv and vscanf:

were already wrapped by recv and scanf,

were not used anywhere else, but in recv and scanf

were not documented properly with Doxygen

I understand they were mentioned in the header file, but I don't think they were ever really meant to be an API?
I would go with Marcin's solution - they can be safely removed.
But actually - should we perhaps move the function header and the doxygen of vrecvscanf into the *.cpp file, to make this function private and pretend to be an API any more?
@mtomczykmobica , @VeijoPesonen , would that make sense?

Agree with @michalpasztamobica. There is no reason to add another level of calls. recv and scanf are API and wrap old vrecv and vscanf/new vrecvscanf.

Those are kind of points @kjbracey-arm should answer. My personal opinion is that once it's exposed as part of public API, undocumented or not, you can't remove it like that. Least what should be done is to mark the PR as breaking change. And yes, if the API is to be changed then the new function should be made as private.

After consultation with @michalpasztamobica functions vrecv and vscanf come back to code. Function vrecvscanfk made private. PR type doesn't change.

michalpasztamobica

Looks like everyone should be happy with the current version.

VeijoPesonen · 2019-10-22T09:09:49Z

platform/ATCmdParser.h

@@ -282,7 +300,7 @@ class ATCmdParser : private NonCopyable<ATCmdParser> {
     */
    int scanf(const char *format, ...) MBED_SCANF_METHOD(1, 2);

-    int vscanf(const char *format, std::va_list args);
+    int vscanf(const char *response, std::va_list args);


This could be reverted back fully, to be in line with scanf

VeijoPesonen · 2019-10-22T09:15:17Z

platform/source/ATCmdParser.cpp

            // If just peeking for OOBs, and at start of line, check
            // readability
            if (!response && j == 0 && !_fh->readable()) {
-                return false;
+                return -1;


~~Should this actually return 0. That's at least the behavior of C++'s scanf function.~~

> On success, the function returns the number of items of the argument list successfully filled. This count can match the expected number of items or be less (even zero) due to a matching failure, a reading error, or the reach of the end-of-file.
~~- cstdio/scanf~~
Scratch that, that comment can be ignored.

VeijoPesonen

I assume you have tested this with ESP8266 and ran the socket tests.

mtomczykmobica · 2019-10-22T13:10:03Z

I assume you have tested this with ESP8266 and ran the socket tests.

Yes, i've tested netsocket tests.

evedon · 2019-10-23T09:51:20Z

Can you add unit tests for the new function please?

michalpasztamobica · 2019-10-23T10:02:40Z

@evedon , I currently do not see any unittests for platform/ATCmdParser.
Adding them from scratch would be quite a large task, because it would make most sense if we tested the whole API, not just the newly added vrecvscanf?
Would it be OK, if we created some work item (internal JIRA ticket, preferably) and planned it for the future, rather than adding it within this PR?

evedon · 2019-10-23T10:19:07Z

@evedon , I currently do not see any unittests for platform/ATCmdParser.
Adding them from scratch would be quite a large task, because it would make most sense if we tested the whole API, not just the newly added vrecvscanf?
Would it be OK, if we created some work item (internal JIRA ticket, preferably) and planned it for the future, rather than adding it within this PR?

I don't think we should create more tech debt. I think it is okay to create a JIRA task to add tests for the whole API but I would rather that the new function is tested in this PR.

0xc0170 · 2019-11-19T13:16:13Z

CI restarted

0xc0170 · 2019-11-19T13:59:54Z

There was an update, I restarted CI (aborted current one).

mbed-ci · 2019-11-19T14:09:00Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

mbed-ci · 2019-11-19T14:48:37Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

mtomczykmobica · 2019-11-19T15:02:11Z

@0xc0170 Problem is with valgrind. ATCmParser has some memory leaks, needs to be investigate and fix.

mtomczykmobica · 2019-11-20T08:12:18Z

@0xc0170 Valgrind problem fixed, please rerun CI job.

mtomczykmobica · 2019-11-20T08:14:57Z

@0xc0170 Valgrind proplem fixed, please rerun CI job.

0xc0170 · 2019-11-20T10:06:17Z

CI restarted

mbed-ci · 2019-11-20T11:04:33Z

Test run: SUCCESS

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

0xc0170 · 2019-11-20T14:38:25Z

@mtomczykmobica Can you do a quick amend to your first commit for the new function addition? It should at least have this in :

The goal is to give a configuration parameter for this function which would indicate are we trying to find a match from one line(scanf) or do we might end up processing multiple lines(recv) to find a match

Describe the new functionality (marked as refactor here?).

…figuration parameter for this function which would indicate are we trying to find a match from one line(scanf) or do we might end up processing multiple lines(recv) to find a match.

mtomczykmobica · 2019-11-21T06:42:59Z

@mtomczykmobica Can you do a quick amend to your first commit for the new function addition? It should at least have this in :

The goal is to give a configuration parameter for this function which would indicate are we trying to find a match from one line(scanf) or do we might end up processing multiple lines(recv) to find a match

Describe the new functionality (marked as refactor here?).

Commit description changed.
This is not new functionality because it is exactly the same but implemented in different way. (using one function with parameter against to two functions).

0xc0170 · 2019-11-21T11:34:22Z

👍

For future reference, keep the first line of the commit msg within 50 characters, the next paragraphs could be longer.

I'll start CI

mbed-ci · 2019-11-21T12:46:44Z

Test run: SUCCESS

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

mtomczykmobica force-pushed the ONME-4405 branch 2 times, most recently from 611b6be to 3d884a4 Compare October 21, 2019 13:59

ciarmcom requested review from AnttiKauppila, michalpasztamobica and a team October 21, 2019 15:00

ciarmcom added the needs: review label Oct 21, 2019

michalpasztamobica suggested changes Oct 21, 2019

View reviewed changes

mtomczykmobica force-pushed the ONME-4405 branch from 3d884a4 to 012625b Compare October 22, 2019 05:33

michalpasztamobica approved these changes Oct 22, 2019

View reviewed changes

mtomczykmobica force-pushed the ONME-4405 branch 2 times, most recently from cec6129 to 04a64bb Compare October 22, 2019 05:47

VeijoPesonen reviewed Oct 22, 2019

View reviewed changes

mtomczykmobica force-pushed the ONME-4405 branch from 04a64bb to 1b89986 Compare October 22, 2019 07:23

mtomczykmobica requested a review from VeijoPesonen October 22, 2019 07:24

mtomczykmobica force-pushed the ONME-4405 branch from 1b89986 to f18ba4d Compare October 22, 2019 08:48

michalpasztamobica approved these changes Oct 22, 2019

View reviewed changes

VeijoPesonen reviewed Oct 22, 2019

View reviewed changes

VeijoPesonen approved these changes Oct 22, 2019

View reviewed changes

mtomczykmobica force-pushed the ONME-4405 branch from f18ba4d to e8bb0a5 Compare October 22, 2019 09:37

VeijoPesonen approved these changes Oct 22, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Oct 23, 2019

0xc0170 added needs: CI release-version: 5.15.0-rc1 and removed needs: work release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

mtomczykmobica force-pushed the ONME-4405 branch from 76dc0d4 to ea5e77e Compare November 19, 2019 13:49

mtomczykmobica force-pushed the ONME-4405 branch 2 times, most recently from b3b23bc to 195a38a Compare November 20, 2019 07:10

0xc0170 added needs: work and removed needs: CI labels Nov 20, 2019

Marcin Tomczyk added 2 commits November 20, 2019 22:35

ATCmdParser: merge scanf and recv functions.The goal is to give a con…

15eb2a6

…figuration parameter for this function which would indicate are we trying to find a match from one line(scanf) or do we might end up processing multiple lines(recv) to find a match.

ATCmdParser: unittests implementation

1d3a1b7

mtomczykmobica force-pushed the ONME-4405 branch from 195a38a to 1d3a1b7 Compare November 21, 2019 06:37

0xc0170 added needs: CI and removed needs: work labels Nov 21, 2019

0xc0170 approved these changes Nov 21, 2019

View reviewed changes

0xc0170 removed the needs: CI label Nov 21, 2019

0xc0170 merged commit 2bde658 into ARMmbed:master Nov 21, 2019

ATCmdParser: merge scanf and recv functions #11720

ATCmdParser: merge scanf and recv functions #11720

Uh oh!

Conversation

mtomczykmobica commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Oct 21, 2019

Uh oh!

michalpasztamobica left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen commented Oct 22, 2019

Uh oh!

VeijoPesonen Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalpasztamobica Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalpasztamobica left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

mtomczykmobica commented Oct 22, 2019

Uh oh!

evedon commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalpasztamobica commented Oct 23, 2019

Uh oh!

evedon commented Oct 23, 2019

Uh oh!

0xc0170 commented Nov 19, 2019

Uh oh!

0xc0170 commented Nov 19, 2019

Uh oh!

mbed-ci commented Nov 19, 2019

Uh oh!

mbed-ci commented Nov 19, 2019

Uh oh!

mtomczykmobica commented Nov 19, 2019

mtomczykmobica commented Oct 21, 2019 •

edited

Loading

VeijoPesonen Oct 22, 2019 •

edited

Loading

michalpasztamobica Oct 22, 2019 •

edited

Loading

VeijoPesonen Oct 22, 2019 •

edited

Loading

evedon commented Oct 23, 2019 •

edited

Loading