drivers: initial commit of generic AT parser module by kaspar030 · Pull Request #7084 · RIOT-OS/RIOT

kaspar030 · 2017-05-20T14:24:00Z

This PR adds a module for interacting with devices using AT commands.

miri64 · 2017-05-20T15:54:17Z

@jia200x maybe useful for your #5427

vincent-d · 2017-05-22T15:08:06Z

I started to work on prototyping with our modem some weeks ago, so I gave a try to your code.

It worked good once I understood I had to add a \r to each of my commands ;)

This is much more simpler that the system I used on a previous project which worked with a lot of callbacks. But here it's hard to deal with responses you do not expect.

I'll give some more try!

jia200x · 2017-05-22T16:12:15Z

@miri64 Indeed. Thanks for pointing me to this.

Cheers

kaspar030 · 2017-05-22T22:23:16Z

It worked good once I understood I had to add a \r to each of my commands ;)

oh, I hoped I had found a combination of line endings to send / line endings to expect that works. The two modems I tried accept a line with just \n, but echo \r\n. They don't like to receive crlf, though. Maybe we need to parameterize the newline behaviour somehow?

kaspar030 · 2017-05-22T22:25:14Z

This is much more simpler that the system I used on a previous project which worked with a lot of callbacks. But here it's hard to deal with responses you do not expect.

Very good point. I planned to add support for that later, once I come up with a process that works and doesn't suck. Suggestions welcome!

vincent-d · 2017-05-23T07:33:29Z

Maybe we need to parameterize the newline behaviour somehow?

It would be handy, indeed.

I use a ublox modem, which needs a \r and echoes \r\r\n.

aabadie · 2017-06-20T19:24:25Z

Any update here ? @kaspar030

aabadie

Maybe the device descriptor pointers could be made const when possible ?

vincent-d · 2017-07-14T08:04:31Z

I gave another try and proposed some improvements in kaspar030#27

haukepetersen · 2017-08-01T11:25:03Z

drivers/at/at.c

+        return -1;
+    }
+
+    if (at_expect_bytes(dev, AT_END_OF_LINE "\r\n", sizeof(AT_END_OF_LINE) + 1, timeout)) {


shouldn't this be AT_END_OF_LINE "\n" as second parameter?

On my modem (and I think it works the same for others as well), if you send AT\r you get as a response:
AT\r <- echo
\r\nOK\r\n

maybe I missunderstand the line above, but AT_END_OF_LINE is

#define AT_END_OF_LINE "\r"

So the line above does make a string concatenation, leading to \r\r\n, right?!

And this is wanted? I don't quite see it, but if it is intended then never mind...

Yes, the AT_END_OF_LINE is expected after the command, the \r\n before the first response line.

It seems that some modems can use different end of line character than \r, that's why it's configurable.

alright, I see.

but just to make sure: wouldn't it make sense then, to make the characters that are expected before the first response line also configurable?

That would not hurt, indeed

aabadie

Found a minor typo.
Do you think it could be even more generalized to any string command controlled devices ? (not only AT). See #7505, where the device replies to commands like, sys reset\r\n or mac set deveui 0000000000000000\r\n with ok\r\n or invalid_param\r\n

aabadie · 2017-08-24T07:43:15Z

drivers/include/at.h

+ * @param[in]   len         len of @p buffer
+ * @param[in]   timeout     timeout (in usec)
+ *
+ * @returns     lenght of response on success


before I forget, there's a typo: s/lenght/length/. I saw it in other places.

fixed two times int at.h.

keestux · 2017-08-25T10:34:23Z

@aabadie Sorry, I don't have a LoRaBee at hand. I do have sodaq ONE and ExpLoRer board with the same RN2483 device but unfortunately I can't connect my Atmel ICE to these boards (so I can't load RIOT programs). :-(

keestux · 2017-08-26T18:30:38Z

drivers/include/at.h

+ *
+ * @param[in]   dev     device to operate on
+ * @param[in]   bytes   buffer containing bytes to expect
+ * @param[in]   len     number of bytes to expect


Why do we need 'len'? The length of 'bytes' should be leading, otherwise you need a sanity check in the function.

I don't quite get the question. len specifies the nr of bytes in bytes. Maybe the docs are misleading?

Why not rely on strlen(bytes) instead of having to pass a string and a length? In the implementation there is a while loop for 'len', but there is no sanity check whether there are that many chars in 'bytes'.

Without 'len' the usage could become as simple as:

if (at_expect_bytes(dev, AT_END_OF_LINE "\r\n", timeout)) {

Bonus: in this case getting rid of the confusing sizeof()+1

Why not rely on strlen(bytes) instead of having to pass a string and a length?

the function is mostly used by other functions (quick grep counts five times). relying on strlen would lead to counting strings multiple times, for strings that are often static. having len as parameter allows the API user to decide whether to use strlen or not...

there is no sanity check whether there are that many chars in 'bytes'

I'm not impressed by that argument.
You can change the while condition in at_expect_bytes() to

while (*bytes != 0) {

and then you won't need to do a strlen()

That actually seems like a good idea. done.

keestux · 2017-08-27T17:33:20Z

drivers/at/at.c

+
+    memset(resp_buf, 0, len);
+
+    while (len) {


What happens if the response string is longer than 'len' and the while loop is ended. If I understand this code well enough then I'm afraid there is no NUL byte terminating the string.

That's what the memset in line 210 should take care of?

No, not quite. Say there is a 'len' of 5 and on the input comes "abcdefg\r\n". The while loop ends as soon as "abcde" was read and resp_buf is completely filled up (no NUL byte anymore). No?

Ah, right. I guess -ENOSPC would be the desired result in that case?

Looking at the code again, if no full line could be read, the result of the function is -1. Shouldn't that be enough? Anyhow, I've added a condition setting the first byte of resp_buf to the null byte in that case.

I still don't see how the string is terminated with a nul-byte. In my example above resp_buf contains the 5 characters a, b, c, d and e. The return value is 5. Maybe you say that the caller is responsible to not treat the result as a nul-terminated string?

The return variable res, which defaults to -1, is only set to the number of bytes read if a full line could be read. In your example ([ a, b, c, d, e ]), the while loop would end with res == -1 and thus set *resp_buf = '\0'.

@keestux Does this address your concerns?

I can see that there can be no unterminated string, even when a longer line comes in. Good.

On the other hand, you take drastic measures to throw away the whole input (instead of truncating).

If this is what we want then OK.
BTW. The doc says "<0 on error", which is either a timeout (-ETIMEDOUT) or a too long input line (-1). Perhaps you want to put this in the function description (at.h).

smlng · 2017-08-28T07:48:41Z

drivers/include/at.h

+ * Most functions compare the bytes echoed by the device with what they
+ * intended to send, and bail out if there's no match.
+ *
+ * Furthermore, the library tries to copy with difficulties regarding different


/copy/cope/ ?

kaspar030 · 2017-08-28T09:12:49Z

Do you think it could be even more generalized to any string command controlled devices ? (not only AT).

It probably can even be used as is for devices not speaking Hayes. The actual AT commands are all built on top of at_expect_bytes(), at_readline() and at_send_bytes().

kaspar030 · 2017-11-01T13:23:59Z

Can we get forward with this? ping @haukepetersen @aabadie @keestux

aabadie

@kaspar030, @vincent-d this PR looks ok codewise. I'm +1 for merging it.
This way, we can move forward on this driver and related features.

aabadie

I did more testing with an Arduino mkrwan board. It embeds a LoRa module that is controlled by AT but not very standard apparently.
I could make it work with some adaption in this PR. See my comments below.

aabadie · 2018-05-22T22:22:07Z

drivers/at/at.c

+                print(resp_pos, read_res);
+            }
+            if (*resp_pos == '\r') {
+                continue;


I have a LoRa modem that is controlled using AT commands. The replies ends with a \r, so continuing here doesn't work with it.
What about adding a macro like AT_END_OF_RESP ?

aabadie · 2018-05-22T22:23:34Z

drivers/at/at.c

+    uart_write(dev->uart, (const uint8_t *)command, cmdlen);
+    uart_write(dev->uart, (const uint8_t *)AT_EOL, AT_EOL_LEN);
+
+    if (at_expect_bytes(dev, command, timeout)) {


My LoRa modem doesn't echo the command sent. I have a fix with another preprocessor macro: AT_CMD_ECHO which is equal to 1 by default. Would this be ok ?

aabadie · 2018-05-23T09:32:27Z

drivers/at/at.c

+    uart_write(dev->uart, (const uint8_t *)command, cmdlen);
+    uart_write(dev->uart, (const uint8_t *)AT_SEND_EOL, AT_SEND_EOL_LEN);
+
+    if (AT_SEND_ECHO) {


@vincent-d @kaspar030, here is the change that allows to skip the echo check at build time.

vincent-d · 2018-05-23T12:16:34Z

What about adding a macro like AT_END_OF_RESP

I would suggest to squash and merge as-is and improve in a follow-up PR.
I also have a couple of improvements I'd like to merge later on.

@kaspar030 could you squash?

vincent-d · 2018-05-23T12:17:28Z

mmh, first, we should fix Murdock:

drivers/include/at.h:52: warning: Member AT_SEND_ECHO (macro definition) of group drivers_at is not documented.

@aabadie ?

aabadie · 2018-05-23T12:38:23Z

we should fix Murdock

done! (sorry)

aabadie · 2018-05-23T12:57:39Z

cpu/atmega_common/periph/i2c.c

 #include "debug.h"
 #define ENABLE_DEBUG      (0)

-/* guard file in case no I2C device is defined */


This looks unrelated, weird.

yes, no idea how that commit sneaked in. I squashed it out.

aabadie · 2018-05-23T14:38:49Z

@vincent-d, ok to merge ? I think we can safely dismiss @haukepetersen's review

seems to be all good

aabadie · 2018-05-23T15:36:07Z

Cool !

kaspar030 · 2018-05-23T15:52:35Z

Thanks for helping with this!

This also was the first time that two other people were pushing into my branch. Feels .... funny. 😏

vincent-d · 2018-05-23T15:59:31Z

Great team work! Thanks all!

kaspar030 requested a review from haukepetersen May 20, 2017 14:24

kaspar030 force-pushed the add_at_parser branch from 55c38a7 to 6d6e0d8 Compare May 20, 2017 14:25

kaspar030 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label May 20, 2017

miri64 assigned haukepetersen May 30, 2017

aabadie requested changes Jun 22, 2017

View reviewed changes

aabadie added this to the Release 2017.07 milestone Jun 23, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

haukepetersen previously requested changes Aug 1, 2017

View reviewed changes

miri64 mentioned this pull request Aug 23, 2017

drivers/rn2xx3: basic initial implementation (without netdev) #7505

Merged

aabadie requested changes Aug 24, 2017

View reviewed changes

keestux reviewed Aug 26, 2017

View reviewed changes

keestux reviewed Aug 27, 2017

View reviewed changes

smlng reviewed Aug 28, 2017

View reviewed changes

kaspar030 force-pushed the add_at_parser branch from 9bcc58c to 842d050 Compare August 28, 2017 09:10

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

aabadie approved these changes May 22, 2018

View reviewed changes

aabadie reviewed May 22, 2018

View reviewed changes

aabadie force-pushed the add_at_parser branch from 1553db3 to 41beb14 Compare May 23, 2018 09:30

aabadie reviewed May 23, 2018

View reviewed changes

kaspar030 force-pushed the add_at_parser branch from c115394 to cf393e5 Compare May 23, 2018 12:51

aabadie reviewed May 23, 2018

View reviewed changes

kaspar030 and others added 9 commits May 23, 2018 16:08

drivers: initial commit of generic AT parser module

9c9fad6

drivers/at: make EOL char configurable

ee29b76

drivers/at: add at_send_cmd_wait_prompt and at_send_bytes

7340e77

drivers: at: many improvements

0199f36

tests: add at parser basic test app

e3f26b2

tests/at: add missing stdio.h include

331f853

tests/at: add nucleo32-f031 to BOARD_INSUFFICIENT_MEMORY

0b603a1

tests/at: improve shell and add README

0c9c00f

tests/driver_at: improve test app

88a978a

kaspar030 force-pushed the add_at_parser branch from cf393e5 to 88a978a Compare May 23, 2018 14:08

vincent-d merged commit 84874e6 into RIOT-OS:master May 23, 2018

kaspar030 deleted the add_at_parser branch May 23, 2018 15:51

aabadie mentioned this pull request May 23, 2018

tests/driver_at: fix wrong nucleo board name #9170

Merged

vincent-d mentioned this pull request May 24, 2018

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

Merged

aabadie mentioned this pull request Jun 4, 2018

drivers/at: make OK and ERROR replies configurable #9288

Merged

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

kaspar030 commented May 20, 2017

Uh oh!

miri64 commented May 20, 2017

Uh oh!

vincent-d commented May 22, 2017

Uh oh!

jia200x commented May 22, 2017

Uh oh!

kaspar030 commented May 22, 2017

Uh oh!

kaspar030 commented May 22, 2017

Uh oh!

vincent-d commented May 23, 2017

Uh oh!

aabadie commented Jun 20, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Jul 14, 2017

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!

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment • 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!

keestux commented Aug 25, 2017

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!

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!

aabadie left a comment •

edited

Loading

aabadie left a comment •

edited

Loading

aabadie May 22, 2018 •

edited

Loading