URC and response conflict for same command

[dragonnn]

An issue I found, when you defined an Urc like that:

#[derive(Clone, AtatUrc, Format)]
pub enum Urc {
    #[at_urc("+CREG")]
    Creg(UrcCreg),
    #[at_urc("+CMTI")]
}

#[derive(Clone, AtatResp, Format)]
pub struct UrcCreg {
    #[at_arg(position = 0)]
    pub registered_state: NetworkRegistration,
    #[at_arg(position = 1)]
    pub location_code: Option<String<4>>,
    #[at_arg(position = 2)]
    pub cell_id: Option<String<7>>,
    #[at_arg(position = 3)]
    pub access_technology: Option<AccessTechnology>,
}

But also have a command that looks like that:

#[derive(Clone, AtatCmd, Format)]
#[at_cmd("+CREG?", CregState, timeout_ms = 200, attempts = 10)]
pub struct GetCregState;

#[derive(Clone, Debug, AtatResp, Format)]
pub struct CregState {
    #[at_arg(position = 0)]
    pub enabled_network_registration: EnabledNetworkRegistration,
    #[at_arg(position = 1)]
    pub registered_state: NetworkRegistration,
    #[at_arg(position = 2)]
    pub location_code: Option<String<4>>,
    #[at_arg(position = 3)]
    pub cell_id: Option<String<7>>,
    #[at_arg(position = 4)]
    pub access_technology: Option<AccessTechnology>,
}

When you run GetCregState it never does success because the response is feed into Urc with shows an error failed to parse URC. Then URC response looks like that:

+CREG: 1,"D1CF","3A3C",0

and the command response looks like that:

+CREG: 2,1,"D1CF","6D17",0

Could command response parsing have priority over Urc parsing? As far I have looked into it then Urc is first, then are command responses. Another solution would be to always try to run both.

[MathiasKoch]

Hi.

We are seeing the same in our application, and it bothers us! But we have been unable to find the correct approach to solving it.

My take on it would be to have the derived nom based URC parser check through argument types and numbers in addition to the prefix + line ending it currently does, such that it can be absolutely sure it matches the URC. Issue is it makes the parse much more complex, and that it requires modelling the URC fully in the URC enum (currently they can be unit variants if you are not interested in the parameters)

[dragonnn]

I don't have myself experience with nom yet, so I will be probably not a big help for that but I am not sure if that is the best approach, forcing the response parameters to be fully in the Urc enum might be not that ergonomic for some cases :/.

[dragonnn]

Wondering if the AtatResp trait could contain additional const that could help the parse qualify if the string readed does belong to it or not? This could be auto generated by the AtatResp derive. At first I was thinking about just adding ARG_COUNT but that doesn't work with additional arguments with is the case here already

[dragonnn]

Not a solution but might be useful for other user as a workaround:

async fn ingress_task(
    mut ingress: Ingress<'static, DefaultDigester<Urc>, Urc, INGRESS_BUF_SIZE, URC_CAPACITY, URC_SUBSCRIBERS>,
    mut rx: RingBufferedUartRx<'static, USART6, DMA2_CH1>,
) {
    info!("modem ingress_task started");
    loop {
        let buf = ingress.write_buf();
        match select(rx.read(buf), INGRESS_TASK_STOP.wait()).await {
            First(Ok(readed)) => {
                if let Some(subset_index) = buf.find_subset(b"+CREG") {
                    let subset = &mut buf[subset_index..];
                    if let Some(subset_end_index) = subset.iter().position(|&b| b == b'\r') {
                        let subset = &mut subset[..subset_end_index];
                        if subset.iter().filter(|&b| *b == b',').count() == 3 {
                            subset[1] = b'U';
                        }
                    }
                }

                ingress.advance(readed).await;
            }
            First(Err(err)) => {
                warn!("ingress_task read error: {:?}", err);
            }
            Second(_) => break,
        }
    }
}

This changes +CREG: 1,"D1CF","3A3C",0 to +UREG: 1,"D1CF","3A3C",0 before it enters the Ingress reader but leaves +CREG: 2,1,"D1CF","6D17",0 unattached

[MathiasKoch]

Hah, thats a neat hack xD

[ijager]

I ran into the same and found another workaround that works in case the URC and the response have different struct definitions. I haven't found a case yet where the response and URC have exactly the same format and are both enabled.

The fix/workaround is to publish the bytes as a response when parsing the URC fails In the Ingress implementation.

// ingress::advance

(DigestResult::Urc(urc_line), swallowed) => {
    if let Some(urc) = Urc::parse(urc_line) {
        debug!(
            "Received URC/{} ({}/{}): {:?}",
            self.urc_publisher.space(),
            swallowed,
            self.pos,
            LossyStr(urc_line)
        );

        if let Err(urc) = self.urc_publisher.try_publish(urc) {
            self.urc_publisher.publish(urc).await;
        }
    } else {
        warn!("Parsing URC FAILED: {:?}", LossyStr(urc_line));
        // Publish urc_line as a response instead.
        if let Err(frame) = self.res_publisher.try_publish(Ok(urc_line).into()) {
            self.res_publisher.publish(frame).await;
        }
    }
    swallowed
}

Not sure if this has unwanted side effects but it seems to work well for us. Maybe there could be an additional check before publishing as a response, or maybe it could be opt-in as a config param for Ingress?

[dragonnn]

I am all for it, since I had the same idea but didn't yet had time to dive into the digest parser to find out how to do it.

[ijager]

I have opened a PR #188 to discuss this particular solution further.