FactbirdHQ / atat

no_std crate for parsing AT commands
Apache License 2.0
109 stars 29 forks source link

Is it intended behavior of atat::helpers::get_line #120

Closed tomoyuki-nakabayashi closed 2 years ago

tomoyuki-nakabayashi commented 2 years ago

Hi, I'm trying to implement an AT command client for TI SimpleLink AT command firmware using atat. I found that currently the following test case about atat::helpers::get_line() is failed, not as I expected.

    #[test]
    fn test_ok_but_line_term_char_not_received_yet() {
        let mut buf = Vec::<u8, 64>::from_slice(b"OK").unwrap();
        let response: Option<Vec<u8, 64>> =
            get_line(&mut buf, b"OK", b'\r', b'\n', false, false, false);
        assert_eq!(None, response);
    }

Is it intended behavior? My expectation is that get_line() returns Some only if buf contains a terminated line, i.e., return Some for b"OK\r\n" but None for b"OK".

I suspect that get_line() should check returned buffer contains the line term char like below.

pub fn get_line<const L: usize, const I: usize>(
    buf: &mut Vec<u8, I>,
    needle: &[u8],
    line_term_char: u8,
    format_char: u8,
    trim_response: bool,
    reverse: bool,
    swap: bool,
) -> Option<Vec<u8, L>> {
    if buf.len() == 0 {
        return None;
    }

    // very Rough check but the above test case passed.
    if !buf.contains(&b'\r') {
        return None;
    }

This workaround works almost cases except for digest::test::data_ready_prompt, its test input is b"AT+USECMNG=0,0,\"Verisign\",1758\r>". It looks like get_line() for b">" returns None and test failed. I'm not sure about correct behavior for it.

BTW, I like atat software design very much :+1: It's well designed for the embedded system, no_std, no heap usage.

thanks in advance,

MathiasKoch commented 2 years ago

Hi @tomoyuki-nakabayashi

Thanks for each out! I don't currently have the time to take a look at your issue at hand as i just had a newborn 3 days ago, so things are a bit hectic at home. That said i would encourage you to check out the branch enhancement/nom-digest to see if that might solve your issue? It is a complete rewrite of the digester to a nom-based zero copy version. I am currently testing it out in my own applications, so it is a bit wip but it seems to work so far, and should also be more "correct" in its approach to parse AT commands.

Alternatively i will be back in the office on the 4. April, and would be happy to take a look at your issue at that time :)

tomoyuki-nakabayashi commented 2 years ago

Thanks for your reply. I'll try enhancement/nom-digest!

tomoyuki-nakabayashi commented 2 years ago

The new nom-digest works as expected. Great work! I added this test and it passed :)

    #[test]
    fn test_ok_but_line_term_char_not_received_yet() {
        let mut digester = NomDigester::default();
        let mut urc_matcher = DefaultUrcMatcher::default();
        let mut buf = Vec::<u8, TEST_RX_BUF_LEN>::new();

        buf.extend_from_slice(b"OK").unwrap();
        let (res, bytes) = digester.digest(&mut buf, &mut urc_matcher);
        assert_eq!((res, bytes), (DigestResult::None, 0));
    }

I look forward to seeing it merged into the master!

MathiasKoch commented 2 years ago

Awesome! It's my first priority in the coming week.. 👌

I'll add your test to the suite 👍