drivers/at: parse +CME/+CMS responses and save error value #20289

derMihai · 2024-01-23T13:02:01Z

Parse +CME/+CMS responses and save error value

Contribution description

Motivation

Modems, especially mobile ones, can be very shaky. As such, in the scope of debugging, it is very helpful to know failure reasons.
Luckily, many modems can offer detailed error codes. Once this feature is enabled, they will answer with +CME ERROR: <error_code> (or +CMS ERROR: <error_code> for SMS-related commands), where <error_code> is either an integer error code or a string. This is much better than the standard ERROR response, which provides no further information.

Unfortunately, in the current state, the AT driver does not parse any errors. Generally, if the response is not OK then -1 is returned and that's it.

Contribution

This patch enables extended error parsing. If a +CME/+CMS answer is detected, most functions will return -AT_ERR_EXTENDED, and the <error_code> will be available in a buffer for further inspection.

Design considerations

I tried to keep the changes at a minimum, maintain API compatibility and offer a clean solution at the same time. At the end, I had to sacrifice on change minimalism. As follows, this is a big PR. Individual changes are annotated inline.

Anyway, some generalities:

<error_response> is retrievable from a buffer instead of parsing and returning it as an error code because, depending modem configuration, it might be a string. Let the user handle that.
the error buffer is passed in at driver initialization out of following reasons
- to keep API as unchanged as possible (e.g. functions not expecting a response do not provide a response buffer)
- in the future, I might improve the URC parsing (currently is very unreliable - see this issue). This buffer could be then used for URC inspection. Otherwise, either heap or large stack allocations will be required.

Testing procedure

See tests/drivers/at/tests-with-config/emulate_dce.py.

derMihai · 2024-01-23T13:02:51Z

~~I will add inline comments regarding design decisions in a few moments.~~
Done.

derMihai · 2024-01-23T13:05:06Z

drivers/at/at.c

@@ -88,18 +96,54 @@ static void _isrpipe_write_one_wrapper(void *_dev, uint8_t data)
 #endif
 }

-int at_dev_init(at_dev_t *dev, uart_t uart, uint32_t baudrate, char *buf, size_t bufsize)
+int at_dev_init(at_dev_t *dev, at_dev_init_t const *init)


Argument list was getting too long.

Is that really a problem?

I think a long argument list is less visible. Especially when it comes to similar-typed arguments - we have two buffers now.

drivers/at/at.c

derMihai · 2024-01-23T13:12:21Z

drivers/at/at.c

@@ -224,8 +268,7 @@ int at_send_cmd(at_dev_t *dev, const char *command, uint32_t timeout)
            return -1;
        }

-        if (at_expect_bytes(dev, CONFIG_AT_SEND_EOL AT_RECV_EOL_1 AT_RECV_EOL_2,
-                            timeout)) {
+        if (at_expect_bytes(dev, CONFIG_AT_SEND_EOL, timeout)) {


this was buggy. AT_RECV_EOL_1 AT_RECV_EOL_2 is returned no matter if the command echoing feature is enabled or not.

some commands do not reply with a newline at all, most notably the prompt commands (on Ublox LTE modules, they return > directly instead of e.g. \r\n>). As most commands parse the response with at_readline_skip_empty(), this newline pruning is no longer needed here. The only exception is at_send_cmd_get_lines(), which adds empty lines to the response too. It is now patched that it skips the first empty line.

BTW. I created #20281 because it allows me to see exactly what the modem is returning for an AT command. For example:

2024-01-24 22:26:30,373 # send 1 AT+COPS? 2024-01-24 22:26:30,374 # UART_DEV(1) TX: AT+COPS? > 2024-01-24 22:26:30,401 # Success: UART_DEV(1) RX: [AT+COPS?\r\r\n] 2024-01-24 22:26:30,403 # Success: UART_DEV(1) RX: [+COPS: 0\r\n] 2024-01-24 22:26:30,404 # Success: UART_DEV(1) RX: [\r\n] 2024-01-24 22:26:30,405 # Success: UART_DEV(1) RX: [OK\r\n]

drivers/at/at.c

drivers/include/at.h

derMihai · 2024-01-23T13:45:41Z

tests/drivers/at/Makefile

@@ -2,8 +2,11 @@ include ../Makefile.drivers_common

 USEMODULE += shell
 USEMODULE += at
-USEMODULE += at_urc_isr_medium


Async URC handling is broken. See this issue.

drivers/at/at.c

keestux · 2024-01-24T22:03:15Z

The title of this PR does not really cover the actual redesign that is going on.

keestux · 2024-01-24T22:04:13Z

Funny note. This evening I started looking at the at driver (again, after more than two years). I was thinking to rewrite this driver, ha ha. Note, I wrote several Arduino libraries for Ublox modems, for example https://github.com/SodaqMoja/Sodaq_R4X. So, I do know bits and pieces about this stuff.

derMihai · 2024-01-25T12:22:09Z

Funny note. This evening I started looking at the at driver (again, after more than two years). I was thinking to rewrite this driver, ha ha. Note, I wrote several Arduino libraries for Ublox modems, for example https://github.com/SodaqMoja/Sodaq_R4X. So, I do know bits and pieces about this stuff.

Looks good. Nice way of detecting echoes!
Right now we're using a layer on top of this driver that provides ergonomic command formatting and response argument parsing. Might make sense to include that here too.

derMihai · 2024-02-01T09:34:52Z

The title of this PR does not really cover the actual redesign that is going on.

Right, so next step is to fix the URC handling (as a first step, synchronously only). I wonder if I should open a separate PR for that, or to add it here and change the name to something like drivers/at: redesign error reporting and URC handling.

drivers/at/at.c

benpicco · 2024-02-09T15:49:40Z

drivers/at/at.c

+            res = at_parse_resp(dev, pos);
+
+            switch (res) {
+            case 0:


Please add a short comment

drivers/at/at.c

drivers/include/at.h

benpicco · 2024-02-09T15:54:11Z

drivers/include/at.h

@@ -177,6 +198,8 @@ typedef struct {
 typedef struct {
    isrpipe_t isrpipe;      /**< isrpipe used for getting data from uart */
    uart_t uart;            /**< UART device where the AT device is attached */
+    char *err_buf;          /**< error buffer */


What is an error buffer?

See line 61.

benpicco · 2024-02-09T15:54:32Z

drivers/at/at.c

@@ -88,18 +96,54 @@ static void _isrpipe_write_one_wrapper(void *_dev, uint8_t data)
 #endif
 }

-int at_dev_init(at_dev_t *dev, uart_t uart, uint32_t baudrate, char *buf, size_t bufsize)
+int at_dev_init(at_dev_t *dev, at_dev_init_t const *init)


Is that really a problem?

drivers/include/at.h

benpicco · 2024-02-12T13:45:30Z

drivers/include/at.h

+    uint32_t baudrate;      /**< UART device baudrate */
+    char *rx_buf;           /**< UART rx buffer */
+    size_t rx_buf_size;     /**< UART rx buffer size */
+    char *err_buf;          /**< error buffer */


Can this be NULL?

Would it make sense to allow this?
There was no error reporting before, so if a user doesn't need this, they might not want to pay for the additional buffer.

I'll have a look at this next week, it makes sense.

Well, on a second thought, it cannot be easily done. The whole PR revolves around the idea that there is always a buffer that can be used for response analysis - either the response buffer, if the command returns a response to the user; or the error buffer otherwise.

However, for the integer error value case, this buffer can be very small, just to hold a response like +CME ERROR: 1234. I can document this.

tests/drivers/at/main.c

keestux · 2024-02-14T21:18:50Z

drivers/at/at.c

+    size_t const cmx_prefix_len = strlen("+CMx ERROR: ");
+    if (strncmp("+CME ERROR: ", resp, cmx_prefix_len) &&
+        strncmp("+CMS ERROR: ", resp, cmx_prefix_len)) {
+        // A command may return either one or the other, we need not differentiate


The comment should mention that it is the "otherwise" condition.

In other words, it is NOT prefix "+CME ERROR: " AND it is NOT prefix "+CMS ERROR: ".

drivers/at/at.c

riot-ci · 2024-02-21T17:52:13Z

Murdock results

✔️ PASSED

c58b71b drivers/at: parse +CME/+CMS responses and save error value

Success	Failures	Total	Runtime
9997	0	9997	11m:00s

Artifacts

Documentation preview

benpicco

My concerns have been addressed, just some small comments.
CI is complaining about trailing whitespaces, you want to fix those.

drivers/at/at.c

drivers/include/at.h

benpicco · 2024-02-21T17:49:20Z

drivers/include/at.h

 *
 * @retval     success code UART_OK on success
 * @retval     error code UART_NODEV or UART_NOBAUD otherwise
 */
-int at_dev_init(at_dev_t *dev, uart_t uart, uint32_t baudrate, char *buf, size_t bufsize);
+int at_dev_init(at_dev_t *dev, at_dev_init_t const *init);


Suggested change

int at_dev_init(at_dev_t *dev, at_dev_init_t const *init);

int at_dev_init(at_dev_t *dev, const at_dev_init_t *init);

feels more idiomatic

I don't know about that. I use east const for consistency, e.g.

char const *const c;

is more consistent than

const char *const c;

in the sense that const applies to whatever is to the left

drivers/at/at.c

benpicco · 2024-02-21T17:53:58Z

drivers/at/at.c

+    size_t resp_len = strlen(resp);
+    if (resp_len + 1 > dev->rp_buf_size) {


Suggested change

size_t resp_len = strlen(resp);

if (resp_len + 1 > dev->rp_buf_size) {

size_t resp_len = strlen(resp) + 1;

if (resp_len > dev->rp_buf_size) {

is a bit nicer to read

It means response string length and I want to keep it consistent across the whole file, otherwise it might get confusing quickly.

drivers/at/at.c

drivers/include/at.h

benpicco

Please squash

derMihai requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners January 23, 2024 13:02

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

derMihai commented Jan 23, 2024

View reviewed changes

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

benpicco requested review from kaspar030 and keestux January 23, 2024 13:10

derMihai commented Jan 23, 2024

View reviewed changes

drivers/at/at.c Show resolved Hide resolved

derMihai commented Jan 23, 2024

View reviewed changes

drivers/at/at.c Show resolved Hide resolved

derMihai commented Jan 23, 2024

View reviewed changes

drivers/include/at.h Outdated Show resolved Hide resolved

derMihai commented Jan 23, 2024

View reviewed changes

keestux reviewed Jan 24, 2024

View reviewed changes

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

benpicco reviewed Feb 9, 2024

View reviewed changes

derMihai force-pushed the mir/at_cme branch from 8683588 to 499649a Compare February 12, 2024 07:25

benpicco reviewed Feb 12, 2024

View reviewed changes

keestux reviewed Feb 14, 2024

View reviewed changes

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

keestux reviewed Feb 14, 2024

View reviewed changes

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

keestux reviewed Feb 14, 2024

View reviewed changes

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

keestux reviewed Feb 14, 2024

View reviewed changes

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

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 21, 2024

benpicco reviewed Feb 21, 2024

View reviewed changes

benpicco reviewed Feb 22, 2024

View reviewed changes

drivers/include/at.h Outdated Show resolved Hide resolved

benpicco approved these changes Feb 22, 2024

View reviewed changes

drivers/at: parse +CME/+CMS responses and save error value

c58b71b

derMihai force-pushed the mir/at_cme branch from 67c6e04 to c58b71b Compare February 22, 2024 11:23

benpicco added this pull request to the merge queue Feb 22, 2024

Merged via the queue into RIOT-OS:master with commit cb83a89 Feb 22, 2024
25 checks passed

derMihai deleted the mir/at_cme branch February 23, 2024 07:18

benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Apr 5, 2024

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: parse +CME/+CMS responses and save error value #20289

drivers/at: parse +CME/+CMS responses and save error value #20289

derMihai commented Jan 23, 2024

derMihai commented Jan 23, 2024 •

edited

Loading

derMihai Jan 23, 2024

benpicco Feb 9, 2024

derMihai Feb 12, 2024 •

edited

Loading

derMihai Jan 23, 2024 •

edited

Loading

keestux Jan 24, 2024

derMihai Jan 23, 2024

keestux commented Jan 24, 2024

keestux commented Jan 24, 2024

derMihai commented Jan 25, 2024

derMihai commented Feb 1, 2024

benpicco Feb 9, 2024

benpicco Feb 9, 2024

derMihai Feb 12, 2024

benpicco Feb 9, 2024

benpicco Feb 12, 2024

derMihai Feb 14, 2024

benpicco Feb 14, 2024

derMihai Feb 14, 2024

derMihai Feb 20, 2024

keestux Feb 14, 2024

riot-ci commented Feb 21, 2024 •

edited

Loading

benpicco left a comment

benpicco Feb 21, 2024

derMihai Feb 22, 2024

benpicco Feb 21, 2024

derMihai Feb 22, 2024

benpicco left a comment

	int at_dev_init(at_dev_t dev, at_dev_init_t const init);
	int at_dev_init(at_dev_t dev, const at_dev_init_t init);

		size_t resp_len = strlen(resp);
		if (resp_len + 1 > dev->rp_buf_size) {

drivers/at: parse +CME/+CMS responses and save error value #20289

drivers/at: parse +CME/+CMS responses and save error value #20289

Conversation

derMihai commented Jan 23, 2024

Parse +CME/+CMS responses and save error value

Contribution description

Motivation

Contribution

Design considerations

Testing procedure

derMihai commented Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derMihai Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

derMihai Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keestux commented Jan 24, 2024

keestux commented Jan 24, 2024

derMihai commented Jan 25, 2024

derMihai commented Feb 1, 2024

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

riot-ci commented Feb 21, 2024 • edited Loading

Murdock results

Artifacts

benpicco left a 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

benpicco left a comment

Choose a reason for hiding this comment

derMihai commented Jan 23, 2024 •

edited

Loading

derMihai Feb 12, 2024 •

edited

Loading

derMihai Jan 23, 2024 •

edited

Loading

riot-ci commented Feb 21, 2024 •

edited

Loading