platform/ATCmdParser

[Crzyrndm]

Description

Relevant links:

• https://github.com/ARMmbed/mbed-os/blob/master/platform/ATCmdParser.h
• https://github.com/ARMmbed/mbed-os/blob/master/platform/ATCmdParser.cpp

Into my third usage/re-implementation/derivation of this parser, the following is based on experience in implementation and API consumption. If features are considered desirable, PR('s) may follow.

A little background

I use mbed entirely offline with ARM GCC 7.2/IDE (VS Code mostly). Most mbed classes are not used directly in my code (either wrapped for additional type/lifetime safety or portability, or reimplemented for reduced locking / additional features / simplicity / etc.). ATCmdParser is one of the ones that eventually got reimplemented (twice. In C the second time due to limitations on project tooling)

API

As a user, ATCmdParser was exactly what I needed when I got started. For sending commands I still can't fault the simple printf style approach, the simple "recv" seems to work really well, and it handles URC/OOB messages cleanly.

"recv" is definitely the weak link here though, and for a couple of reasons.

1. Most obvious: responses with multiple possible formats, handling failure
   -- How to write a recv call that matches "OK\r\n" or "0\r" (success with verbose on/off on the modems I was targeting). I never did work this one out, you had to write against a single command mode (which isn't that bad, but...)
   -- Errors / multiple possible responses. Got an Error instead of an OK when you've got a long timeout command? Requires multiple "abort" URCs to cover the possible responses. Rare but not exactly nice to try and handle.
2. The whole "this isn't a greedy parser" thing. I see that the current impl can somewhat avoid that by adding a trailing '\n' (which doesn't work with Telit/Ublox/? modems in ATV0 by the way. Response codes are (by default) only trailed by a '\r' character only).
   -- For what reason doesn't it read an entire line before attempting a match? (I'm assuming there is a modem which doesn't put a newline before a large chunk of data in some response).
   -- Greedy line simplifies oob handlers IMO (pass the line to the handler instead of the handler having to read the rest of the line) and less likely to accidentally start doing recursive work
   -- my workaround was to end the format with "%d%*[\r]" (match int then match and discard '\r'). The '\n' is definitely more obvious, but greedy by default wins for ease of use and minimal surprises.
3. Inability to override URC handlers. "AT+CGREG?" returns a slightly different format to the URC (the current mode is the first digit, followed by the URC format). This made the URC more complicated (it had to handle both read and URC formats) and the program flow less obvious (read command, no check for read response, check for "OK", use variables updated by response).
   -- Dead simple fix with a greedy/line by line receiver (check the recv format before checking urc's)
4. Finally, a nice to have. In several places I just needed to "read a single line" (primarily for dealing with identification commands (e.g. +GMM/+CGMR) which don't have a prefix in the response) and then parse myself. It's easy enough to wrap over recv once you work out how to make it greedy, but you still can't use the internal buffer to do so (%s requires a buffer to copy into). My newer versions now have a read_line command which (effectively) returns a struct { const char*, int } for a simpler and more stack efficient method.

Other minor annoyances

• the presence of the scanf method is confusing and we had several bugs caused by accidentally using it instead of recv or vice versa. It shouldn't be used outside URC handlers in my experience (to get the rest of the line), but I had to learn that the hard way
• There is no way to provide a pre-allocated buffer (even when dynamic allocation is allowed, static RAM usage is much easier to track and manage).
• No obvious way to partial match a recv (I think if you did it in multiple recv calls it would work, but I don't know if someone who hasn't dug right through it would guess that). I haven't needed this yet, but I could see it happening

Implementation

Some suggestions from my implementation experience.

1. Greedy parsing leads into many simplifications/optimisations. Even if it can't always be used, having it available as a seperate method ("recv_line") might be worth considering. It may require an API change for OOBs though (passing a pointer to the read line) which would make this difficult for backwards compatibility :(
   -- fixes: some scanf hackery
   -- enables: recv overriding registered oob, oobs having to read remainder of line (tempting recursive parsing -> stack overflow)
   -- optimsation: single pass for each received line helps when lots of OOBs are registered
2. "recv_match" matching using callbacks <signature: int(const char*, int, va_list)> instead of scanf
   -- enables: allowing for multiple responses to be matched successfully
   -- callback takes ptr/len for received char array + va_list to scanf into if required (void* is probably better than va_list, but I haven't got around to trying it yet). Int return instead of bool allows the match to be ID'd if required (e.g. was this an error or OK). I've used a zero return to indicate no-match to the [arser but not entirely happy with that yet (I strongly prefer types rather than convention when I can)
3. A dedicated "End of command" predicate to allow detection of unexpected responses (as opposed to a slew of standard URC handlers that still can't handle some cases).
   -- bool(const char*, int); // return true if the received line indicates "end of command"
   -- my check order after receiving a line is: format parameter -> OOBs -> end of command
   -- recv returns an enum (match / timeout / eoc) wrapper which converts to bool (true for match only)
4. Escaping the format string and the double scanf are avoidable. Just count the number of args to match and comparing that to the scanf return with a strcmp fallback for trailing characters (finding the end of the last format specifier is relatively easy). This both reduces memory requirements and improves ease of debugging

Is there interest in taking any of this further?

EDIT
Note that I use Oob and URC interchangably to refer to the same concept (URC is how most modem docs refer to these messages)

Reading through this again, there are two major points/questions that almost everything else becomes a sub-question of

• Why not a greedy parser? (This is likely to be difficult to change, but I pose the question anyway)
• Why limit recv to a direct scanf? (It works OK for the simple cases but...)

Issue request type

[X] Question
[X] Enhancement
[ ] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-828

[Crzyrndm]

Critiques of the "line by default" from co-worker

• optional response variables have to be handled by a predicate (as opposed to multiple recv calls)
• 1 extra function required ("greedy mode") for prompts that don't have line termination

Basically: recv handles everything in 1 function vs. 2/3

[cmonr]

@ARMmbed/mbed-os-wan Fyi

Possibly also @kjbracey-arm and @bulislaw / @donatieng

[kjbracey]

Some good thoughts here - the ATCmdParser is an old design and has had relatively little work on it. What it has had have basically been backwards-compatible extensions. The answer to almost all your "why?" questions at this point is "backwards compatibility" and "lack of interest in improving it".

It would certainly be nice to not have to install loads of oob handlers for every possible response. That oob facility was added first for actual URCs, and the abort call was a minimal addition to make the same mechanism useable for alternate responses. A deeper solution better tailored to responses would I'm sure be welcomed.

On line endings, the intent is that any type of line ending (CR, LF, CR+LF, LF+CR) gets normalised automatically to \n as it's placed in _buffer, so that "OK\n" or "Greedy:%s\n" should always work in a recv call, even if the modem sends OK[CR] or Greedy:Foo[CR]. Is that not working? (And in the event that there's binary data, you'd have to use read to get it raw).

1 extra function required ("greedy mode") for prompts that don't have line termination

Not quite clear on this. There must be some termination condition of a greedy match on a stream, right? It's either a delimiter character, or the end of line itself. Are you wanting to terminate and match an incomplete line just on a timeout?

[Crzyrndm]

Some good thoughts here - the ATCmdParser is an old design and has had relatively little work on it. What it has had have basically been backwards-compatible extensions. The answer to almost all your "why?" questions at this point is "backwards compatibility" and "lack of interest in improving it".

I had assumed the backwards compatibility was a major factor (as I was writing the set of questions it very quickly became clear that it could be quite difficult to make several of the changes in a backwards compatible manner. Something I didn't think about until putting thoughts on paper so to speak)

It would certainly be nice to not have to install loads of oob handlers for every possible response. That oob facility was added first for actual URCs, and the abort call was a minimal addition to make the same mechanism useable for alternate responses. A deeper solution better tailored to responses would I'm sure be welcomed.

This for me has been one of the best changes I've made, although I did figure out the URC/abort thing (again during/after putting thoughts on paper). The difficulty would be what to return from recv when it triggers (is it the same as a timeout?).

This could be a relatively simple modification (add one function to set it, one callback to store it, add a call into recv if not NULL, and maybe add a default handler) if treating it the same as a timeout.

On line endings, the intent is that any type of line ending (CR, LF, CR+LF, LF+CR) gets normalised automatically to \n as it's placed in _buffer, so that "OK\n" or "Greedy:%s\n" should always work in a recv call, even if the modem sends OK[CR] or Greedy:Foo[CR]. Is that not working? (And in the event that there's binary data, you'd have to use read to get it raw).

I probably miss-understood something in the implementation. Most of my experience is with a very out of date version of the Parser and I was checking issues I found against the current implementation by eye. Looking again, I was likely incorrect :/

1 extra function required ("greedy mode") for prompts that don't have line termination

Not quite clear on this. There must be some termination condition of a greedy match on a stream, right? It's either a delimiter character, or the end of line itself. Are you wanting to terminate and match an incomplete line just on a timeout?

I think I used the incorrect wording there (not greedy). Responses can be unterminated and this caused trouble for my greedy implementation until I wrked out what was going on

---

I can't see a reasonable way to do a full line-greedy implementation without breaking backwards compatibility (due largely to how URC handlers are set up). Even with a seperate function, URC's still have to be checked and the recv halted for a matching line.

I will see what I can do about cleaning up I2 (recv_match) & I3 (end of command predicate) which can probably avoid any backwards compatibility issues. I4 (no copying/reformatting the recv format / double scanf) is worth exploring but likely has too many edge cases (the best solution is probably a simplified scanf alike function that reports the required information, but that is quite a task)

[kjbracey]

The difficulty would be what to return from recv when it triggers (is it the same as a timeout?).

It can only trigger if you've requested it it, so there's no backwards compatibility problem with adding new response values if necessary.

Responses can be unterminated

Again, how can a response be unterminated? If there's no termination, how do you know the response is complete, and we need to go back to standard parse state?

The only possibilities I'm aware of are:

• Terminated by end-of-line, so recv("+VALUE=%s\n")
• Delimited by some characters, so recv("+VALUE=\"%s\",")
• Length known up-front, eg from something read using recv, so read(len).
• Some more custom framing which would be beyond the scope of a simple AT handler - eg at the extreme, take over from the ATCmdParser and start running PPP on the stream.

I can't currently think of a case that the parser doesn't handle, or isn't just rather out-of-scope. You've always got the freedom to run your own interpretation code on the stream for a custom termination condition before going back to using the parser.

[0xc0170]

@Crzyrndm Shall this issue be closed (no update since the last response) ?