drivers/at: fix URC handling and add better testing #20423

derMihai · 2024-02-23T10:04:15Z

Contribution description

Changes in this PR:

remove asynchronous URC handling, as it is broken (see drivers/at: remove at_urc_isr module #20059). I might get back to this feature in the future and re-implement it, but it would need a complete re-work.
add robust URC handling where it's possible and document which actions the user has to take where it's not
refactor the response parsing to make it individually testable and add extensive unit testing.

At the first glance, the diffs in this PR are huge. However, much of it comes from refactoring. Therefore, I have split this PR in three commits:

drivers/at: removed async URC handling. - removes asynchronous URC handling. I am not sure I properly removed this feature from everywhere, please point me to additional locations that I might have overlooked.
drivers/at: refactor for better unit testing of parsing methods - extracts response parsing from the command sending procedures in the scope of unit testing
drivers/at: URC handling done, added unit tests, documentation. - this is where the actual functional changes happen. Adds roubst URC handling, unit testing and proper documentation.

Testing procedure

See tests/drivers/at/tests-with-config/emulate_dce.py
On native, run tests/drivers/tests-with-config/test_all_configs.sh. This will compile the at_tests program and run the unit tests with all possible configuration options. No actual modem is needed.

Issues/PRs references

addresses #20059
removes #14327 (comment)

riot-ci · 2024-02-23T10:31:20Z

Murdock results

✔️ PASSED

63e057c drivers/at: Fix sync URC handling. at_send_cmd_get_lines() keeps EOL.

Success	Failures	Total	Runtime
10002	0	10006	10m:31s

Artifacts

derMihai · 2024-02-23T12:21:27Z

drivers/include/at.h

@@ -465,7 +503,7 @@ ssize_t at_readline(at_dev_t *dev, char *resp_buf, size_t len, bool keep_eol, ui
 * @param[in]   dev         device to operate on
 * @param[in]   resp_buf    buffer to store line
 * @param[in]   len         size of @p resp_buf
- * @param[in]   keep_eol    true to keep the CR character in the response
+ * @param[in]   keep_eol    true to keep the trailing EOL sequence in the response


same as in at_readline()

derMihai · 2024-02-23T13:04:31Z

drivers/include/at.h

 * @param[in]   dev         device to operate on
 * @param[in]   resp_buf    buffer to store line
 * @param[in]   len         size of @p resp_buf
- * @param[in]   keep_eol    true to keep the CR character in the response
+ * @param[in]   keep_eol    true to keep the trailing EOL sequence in the response


This used to keep only the first character in the EOL sequence whenever keep_eol was true, which I find kind of arbitrary and creates a lot of edge cases to think about. Now you can either have the whole trailing EOL sequence or nothing. If e.g. the EOL is composite (\r\n), individual EOL characters will always be kept.

derMihai · 2024-02-23T13:18:54Z

drivers/at/at.c

+    } while (*p);
+}
+#endif
+
 static ssize_t get_lines(at_dev_t *dev, char *resp_buf, size_t len, bool keep_eol,


I didn't change the behavior, but I'm voting to change it s.t.:

EOL between lines should always be included in their original form - this means keep_eol will be unnecessary and always true

discard empty lines - which means leading EOLs or multiple in a row will get discarded
I think this will be much leaner and predictable for the user.

Any arguments against these changes?

how does this pr seperate multiple lines one gets through this function with keep_eol=false? (previously the at_readline would have had left the '/n' in )

-> this function would have returned multiple lines seperated by newline prior to this

I didn't change this, so I don't get what you are referring to. If you mean what I propose to change, then keep_eol will be removed (i.e. will always be true), and the original EOL sequences will stay in, except for multiple in a row. In other words, by my proposal, this function will return a list of non-empty lines separated by the original EOL sequence. Basically just call at_readline_skip_empty(keep_eol=true) untile timeout, OK or ERROR happens. I think this behavior is simpler and in line with what a user would expect.

Moreover, I really can't make up any use-case where throwing out the \r is necessary, and in the current implementation this creates some weird edge-cases, e.g. if a single \r is encountered, but no \n follows it, it is silently dropped.

you changed the behavior of 'at_readline_skip_empty' (at least that is what the comment change to drivers/include/at.h:494
is indicating to no longer return the /n of the /r/n sequence

since this function uses 'at_readline_skip_empty' there might be no line separation ( neither /n nor /r ) while it gets multiple lines.

so without changing this function you changed its behavior ( didn't you ?)

But this wouldn't have happened before either. at_readline_skip_empty(keep_eol=false) (which is only called until the first non-empty line is encountered) would have replaced the trailing \n with \0 anyway. And in case an empty line is returned, a \n or whatever is the last character in EOL is inserted.

Seems that this discussion reinforces my point that the EOL handling of this function needs to be simpified. ;)

I changed this behavior to the one proposed. This also includes some small changes in line parsing -- mainly that response buffer overflow is returned with -ENOBUFS and not silently ignored. I packed these changes in a separate commit.

kfessel · 2024-03-08T15:30:55Z

tests/drivers/at/tests-with-config/test_all_configs.sh

+	echo "  rcv EOL 1    = $rcv_eol_1"
+	echo "  rcv EOL 2    = $rcv_eol_2"
+
+	# make -j BOARD=native "HANDLE_URC=$handle_urc" "ECHO_ON=$echo" "SEND_EOL=\"$send_eol\"" "RECV_EOL_1=\"$rcv_eol_1\"" "RECV_EOL_2=\"$rcv_eol_2\"" compile-commands


seems like a leftover

No, I left this in on purpose to have the compile-commands.json of the first build that failes.

why use the test to build the compile-commands.json ?

The test runs through various build configurations, and I want the compile-commands.json of the particular build that failed, without having to re-build that per hand.
Alternatively, I can always build compile-commands whenever a test fails, with that particular build settings.

Ok I changed this so that compile-commands.json is built whenever the test fails.

tests/unittests/tests-at/tests-at.c

drivers/at/at.c

kfessel · 2024-03-08T16:25:53Z

drivers/at/at.c

 {
+    ssize_t res;


seem like this could just be int;
at_send_cmd() returns int;
get_lines probaly returns an int to that it turned into an ssize;

this might be the other way around since some of the other functions also seem to return int even though they return size data

atm this seems a little mixed

should be ssize_t everywhere a size is returned but isrpipe_read_timeout() returns int too...

right, so I changed everything like this:

fn that return 0 or negative error return int

fn that return a size or negative error return ssize_t

fn that return a size return size_t

kfessel · 2024-03-08T16:28:35Z

drivers/at/at.c

+    return get_lines(dev, resp_buf, len, keep_eol, timeout);
+}
+
+static int wait_prompt(at_dev_t *dev, uint32_t timeout)


may return size_t ?

return 0 or negative error -> stays int

drivers/at/at.c

tests/drivers/at/tests-with-config/test_all_configs.sh

kfessel

mostly good ( derMihai wants to do some minor changes) unless other reviewers chime in i would approve this

drivers/at/at.c

kfessel · 2024-03-13T16:12:40Z

minor api change ( the keep_eol is gone)

tests/unittests/tests-at/tests-at.c

kfessel

commit messages need to be rewritten / commits squashed

maybe have one of them contain remove keep_eol

kfessel · 2024-03-20T12:11:25Z

Error: Commit message is longer than 72 characters: "drivers/at: Added robust, synchronous URC handling. Added unit tests. - API change: at_send_cmd_get_lines() now always keeps the whole EOL sequence between lines"
Warning: Commit message is longer than 50 (but < 72) characters: "drivers/at: refactor for better unit testing of parsing methods"
Error: Process completed with exit code 1.

added -> add removed -> remove
( we do not need tens information with commit messages)

benpicco

Sorry, forgot about this one.
Looks good from my side!

derMihai requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners February 23, 2024 10:04

github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels Feb 23, 2024

benpicco requested a review from keestux February 23, 2024 10:10

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 23, 2024

derMihai force-pushed the mir/at_urc branch from abcb50f to 71c28ac Compare February 23, 2024 10:21

derMihai force-pushed the mir/at_urc branch from 71c28ac to 45afc63 Compare February 23, 2024 12:15

derMihai commented Feb 23, 2024

View reviewed changes

derMihai force-pushed the mir/at_urc branch from 45afc63 to edc88aa Compare February 23, 2024 13:00

derMihai commented Feb 23, 2024

View reviewed changes

derMihai force-pushed the mir/at_urc branch 4 times, most recently from 1d2efdf to a531ae1 Compare February 26, 2024 08:31

derMihai requested a review from miri64 as a code owner March 4, 2024 14:36

derMihai force-pushed the mir/at_urc branch 2 times, most recently from 779b0b7 to 9ed3725 Compare March 5, 2024 07:52

kfessel reviewed Mar 8, 2024

View reviewed changes

tests/unittests/tests-at/tests-at.c Outdated Show resolved Hide resolved

kfessel reviewed Mar 8, 2024

View reviewed changes

tests/unittests/tests-at/tests-at.c Outdated Show resolved Hide resolved

kfessel reviewed Mar 8, 2024

View reviewed changes

derMihai force-pushed the mir/at_urc branch 2 times, most recently from 9304d3f to 20a0480 Compare March 11, 2024 10:14

kfessel reviewed Mar 11, 2024

View reviewed changes

tests/drivers/at/tests-with-config/test_all_configs.sh Outdated Show resolved Hide resolved

derMihai force-pushed the mir/at_urc branch from 20a0480 to 440ccbd Compare March 11, 2024 14:16

kfessel reviewed Mar 12, 2024

View reviewed changes

drivers/at/at.c Outdated Show resolved Hide resolved

derMihai force-pushed the mir/at_urc branch 3 times, most recently from 074e868 to 4ab5bc1 Compare March 13, 2024 14:57

kfessel added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Mar 13, 2024

kfessel reviewed Mar 18, 2024

View reviewed changes

tests/unittests/tests-at/tests-at.c Outdated Show resolved Hide resolved

kfessel reviewed Mar 18, 2024

View reviewed changes

derMihai added 2 commits March 20, 2024 12:49

drivers/at: removed async URC handling.

b40c9a0

drivers/at: refactor for better unit testing of parsing methods

218d79f

derMihai force-pushed the mir/at_urc branch from 4ab5bc1 to e9f2f3b Compare March 20, 2024 11:57

drivers/at: Fix sync URC handling. at_send_cmd_get_lines() keeps EOL.

63e057c

derMihai force-pushed the mir/at_urc branch from e9f2f3b to 63e057c Compare March 20, 2024 13:34

benpicco approved these changes Mar 25, 2024

View reviewed changes

kfessel added this pull request to the merge queue Mar 25, 2024

Merged via the queue into RIOT-OS:master with commit e6f03db Mar 25, 2024
26 checks passed

derMihai mentioned this pull request Mar 26, 2024

drivers/at: remove at_urc_isr module #20059

Closed

Teufelchen1 mentioned this pull request Apr 4, 2024

unittests: Move at test to driver tests #20540

Merged

MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/at: fix URC handling and add better testing #20423

drivers/at: fix URC handling and add better testing #20423

derMihai commented Feb 23, 2024

riot-ci commented Feb 23, 2024 •

edited

Loading

derMihai Feb 23, 2024

derMihai Feb 23, 2024

derMihai Feb 23, 2024 •

edited

Loading

kfessel Mar 11, 2024 •

edited

Loading

derMihai Mar 11, 2024

kfessel Mar 11, 2024

derMihai Mar 12, 2024

derMihai Mar 13, 2024

kfessel Mar 8, 2024

derMihai Mar 11, 2024

kfessel Mar 11, 2024

derMihai Mar 11, 2024

derMihai Mar 11, 2024

kfessel Mar 8, 2024

kfessel Mar 8, 2024

derMihai Mar 11, 2024

derMihai Mar 11, 2024

kfessel Mar 8, 2024

derMihai Mar 11, 2024

kfessel left a comment •

edited

Loading

kfessel commented Mar 13, 2024

kfessel left a comment •

edited

Loading

kfessel commented Mar 20, 2024 •

edited

Loading

benpicco left a comment

drivers/at: fix URC handling and add better testing #20423

drivers/at: fix URC handling and add better testing #20423

Conversation

derMihai commented Feb 23, 2024

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Feb 23, 2024 • edited Loading

Murdock results

Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derMihai Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

kfessel Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfessel left a comment • edited Loading

Choose a reason for hiding this comment

kfessel commented Mar 13, 2024

kfessel left a comment • edited Loading

Choose a reason for hiding this comment

kfessel commented Mar 20, 2024 • edited Loading

benpicco left a comment

Choose a reason for hiding this comment

riot-ci commented Feb 23, 2024 •

edited

Loading

derMihai Feb 23, 2024 •

edited

Loading

kfessel Mar 11, 2024 •

edited

Loading

kfessel left a comment •

edited

Loading

kfessel left a comment •

edited

Loading

kfessel commented Mar 20, 2024 •

edited

Loading