Skip to content

driver/at: add option to keep EOL character in readline, and make EOL configurable#9185

Merged
aabadie merged 3 commits intoRIOT-OS:masterfrom
OTAkeys:pr/add_at_parser_eol
May 29, 2018
Merged

driver/at: add option to keep EOL character in readline, and make EOL configurable#9185
aabadie merged 3 commits intoRIOT-OS:masterfrom
OTAkeys:pr/add_at_parser_eol

Conversation

@vincent-d
Copy link
Copy Markdown
Member

@vincent-d vincent-d commented May 24, 2018 •
edited
Loading

Contribution description

With modem, I have some use cases where I want to keep all EOL characters in the response when calling at_send_cmd_get_lines. For instance when reading a file or an HTTP response.

This PR add a parameter to at_send_cmd_get_lines thus to at_readline to not remove EOL characters. So far '\r' was removed.

It also let the user configure the expected response EOL characters with 2 macros.

(this is not yet tested)

Issues/PRs references

Follow-up #7084

@vincent-d vincent-d added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels May 24, 2018
@vincent-d vincent-d requested review from aabadie and kaspar030 May 24, 2018 13:43
aabadie
aabadie reviewed May 24, 2018
View reviewed changes
drivers/at/at.c Outdated
}
}
if (*resp_pos == '\n') {
if (*resp_pos == eol[sizeof(eol) - 1]) {
Copy link
Copy Markdown
Contributor

@aabadie aabadie May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here it should be:

if ((*resp_pos == eol[sizeof(eol) - 1]) || 
    ((sizeof(eol) == 2) && (*resp_pos == eol[0]))) {

And it will work if AT_RECV_EOL_2 is empty and AT_RECV_EOL_1 is \r which is the case for me.

Copy link
Copy Markdown
Member Author

@vincent-d vincent-d May 25, 2018

Choose a reason for hiding this comment

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

Changed to if (*resp_pos == eol[sizeof(eol) - 2]). The sizeof(eol) - 1 was a mistake, as it should always be \0. Now the last character of eol[] is used, so either AT_RECV_EOL_2 if both are set or AT_RECV_EOL_1 if the former is empty.

@vincent-d vincent-d force-pushed the pr/add_at_parser_eol branch from 21c394c to a2b75b9 Compare May 25, 2018 08:03
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 29, 2018

@vincent-d, is this PR tested on your side ?
Otherwise, I tested it with the Arduino MKRWAN1300, using \r in EOL1 and an empty EOL2 and it works like a charm.
I'm inclined to ACK but maybe @kaspar030 is also interested by having a look/testing.

kaspar030
kaspar030 reviewed May 29, 2018
View reviewed changes
drivers/at/at.c Outdated
else if (res > 0) {
bytes_left -= res;
if ((res == 2) && (strncmp(pos, "OK", 2) == 0)) {
if ((res == (2 + (keep_eol ? 1 : 0))) && (strncmp(pos, "OK", 2) == 0)) {
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 May 29, 2018

Choose a reason for hiding this comment

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

here and below: if keep_eol, it has values 0 or 1, thus the ternary operator can be removed. e.g., just use 2 + keep_eol)

Copy link
Copy Markdown
Member Author

@vincent-d vincent-d May 29, 2018

Choose a reason for hiding this comment

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

changed

@vincent-d vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels May 29, 2018
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented May 29, 2018

tested with ublox-c030-u201

aabadie
aabadie approved these changes May 29, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK, please squash

@vincent-d vincent-d force-pushed the pr/add_at_parser_eol branch from b7c30c0 to faa51a2 Compare May 29, 2018 14:52
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented May 29, 2018

squashed

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 29, 2018

@vincent-d, there are failing tests on Murdock ('unknown type name 'bool'', an include is missing I think)

@vincent-d vincent-d force-pushed the pr/add_at_parser_eol branch from faa51a2 to 3f620d5 Compare May 29, 2018 15:31
@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented May 29, 2018

there are failing tests on Murdock

Sorry, should be fixed (amended directly)

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 29, 2018

all green, go!

@aabadie aabadie merged commit ad31664 into RIOT-OS:master May 29, 2018
@aabadie aabadie mentioned this pull request May 29, 2018
Merged
@cladmi cladmi added this to the Release 2018.07 milestone Jul 10, 2018
@toonst toonst deleted the pr/add_at_parser_eol branch August 21, 2018 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@aabadie aabadie aabadie approved these changes

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

4 participants

@vincent-d @aabadie @kaspar030 @cladmi