FactbirdHQ / atat

no_std crate for parsing AT commands
Apache License 2.0
115 stars 31 forks source link

Parsing AT+CMGR response, falling on empty ,, to option and trailing string #180

Closed dragonnn closed 11 months ago

dragonnn commented 11 months ago

I have already started poking inside serde_at to fix some issue. Here is the AT command response with escaped characters:

+CMGR: \"REC UNREAD\",\"+48123456789\",,\"23/11/21,13:31:39+04\"\r\nINFO

They are two issues when parsing it, first is the ,, with is a placeholder to put size of the read message when in PDU mode with I don't use, in text mode it is empty. So I put in the response struct: pub size: Option<usize>, but that was still falling, so I changed in serde_at deserialize_option to this:

        match self.parse_whitespace() {
            Some(b'+' | b',') | None => visitor.visit_none(),
            Some(c) => visitor.visit_some(self),
        }

Not sure if this is the best idea, but it seams to work and not break anything? But the second issue is bigger with I am stuck on with no idea how to best fix. After I fixed the first issue when I run my code I got this error: CustomErrorWithMessage("invalid length 4, expected struct Message with 5 elements") with as far I know does come from next_element_seed() not realizing the \r\nINFO is the last part of the response and should be parsed to INFO. The problem here is how to adjust that part of the code, because after parse_whitespace() it is already at I character because \r\n are omitted. I feel like serde_at should have "put everything left into the last struct field" but it needs to be aware of that INFO is just an example and it can contain ,\r\n basically any character because it is just the content of the SMS message received. Will be gratefully for any help dealing with that, I hope I only need help with the right direction to fix it and come up with a PR (and hope the first fix is fine too).

Almost forgot, here is the Message struct:

#[derive(Clone, Debug, Deserialize)]
pub struct Message {
    state: String<256>,
    pub sender: String<256>,
    pub size: Option<usize>,
    pub date: String<256>,
    pub message: String<256>,
}
dragonnn commented 11 months ago

Chaning next_element_seed() to this:

 fn next_element_seed<T>(&mut self, seed: T) -> Result<Option<T::Value>>
    where
        T: de::DeserializeSeed<'de>,
    {
        if let Some(b'\r' | b'\n') = self.de.peek() {
            self.de.eat_char();
            if let Some(b'\r' | b'\n') = self.de.peek() {
                self.de.eat_char();
            }
            self.de
                .parse_whitespace()
                .ok_or(Error::EofWhileParsingValue)?;
        } else {
...

Does work for that case but that is super ugly I am pretty sure wrong, and changing INFO to INFO,ttt does break it right away, and that should work fine.

MathiasKoch commented 11 months ago

Huh, interesting! Nice find on the empty parameters, don't think I have ever used that, so I probably haven't accounted for them when writing the parser. Your solution seems reasonable.

Regarding the other issue, the format looks super strange. I have never come across a command that mixes like that without it being some kind of newline separated list.

MathiasKoch commented 11 months ago

Ahh .. The contents of text messages..

MathiasKoch commented 11 months ago

No matter what, your message should probably be a Vec<u8> instead of String, as it is not quoted.

dragonnn commented 11 months ago

I am fine with using Vec or Bytes for it, that doesn't unfortunately fix the issue with parsing it, both still give the same error of invalid length

MathiasKoch commented 11 months ago

I am not sure what the correct way to handle this in serde deserializers would be. What we want here is to just take the buffer remainder as the last parameter, but I am not sure if that is safe to do generally..

dragonnn commented 11 months ago

Yes, that would be best way to handle it. Hmmm I am wondering if #[serde(skip)] on the last field would be a good idea and then allowing to be the last argument handled by AtatResp directly? Could be done before or after serde parsing, but I am not sure if serde has an option to output what was left after parsing so it might be better to do before? But that would require more parsing to happen inside AtatResp and only parse a slice of the read response to serde_at