Two suggestions: slight mods to the resetting of commandAck; and packet .counter and .valid

[PaulZC]

Hi Nathan (@nseidle),
While I was digging through the code to diagnose the maxWait timeout problem (#60), I thought about the following two suggestions (but wasn't brave enough to include them in PR #62):

• I think setting commandAck to UBX_ACK_NONE in sendCommand is possibly redundant? It gets reset in waitForResponse and I think that's the right place to do it. (I see @matthewgkerr made the same point in Unnecessary reset of commandAck #35)

• the packet .counter and .valid parameters are reset together inside process. I wonder if they should be reset separately? Let's say we do a get of a value. We get both a packetCfg and packetAck in return. Let's say they are followed immediately by an AutoPVT packet, checkUbloxSerial (and possibly checkUbloxI2C) will keep processing bytes and process will reset .counter and .valid for both the Cfg and Ack packets. If you made the following change, it would allow the Ack.valid to remain true and so the sendCommand which started all this would return true instead of timing out?

I'm suggesting changing these lines from this:

    else if (ubxFrameCounter == 2)                     //Class
    {
      packetAck.counter = 0;
      packetAck.valid = false;
      packetCfg.counter = 0;
      packetCfg.valid = false;

      //We can now identify the type of response
      if (incoming == UBX_CLASS_ACK)
        ubxFrameClass = CLASS_ACK;
      else
        ubxFrameClass = CLASS_NOT_AN_ACK;
    }

to this:

    else if (ubxFrameCounter == 2)                     //Class
    {
      //We can now identify the type of response
      if (incoming == UBX_CLASS_ACK)
      {
        packetAck.counter = 0;
        packetAck.valid = false;
        ubxFrameClass = CLASS_ACK;
      }
      else
      {
        packetCfg.counter = 0;
        packetCfg.valid = false;
        ubxFrameClass = CLASS_NOT_AN_ACK;
      }
    }

Worth a thought?
Enjoy!
Paul

[nseidle]

1. Yep. Removing.
2. Yea, I was seeing this as well. Reseting both .valids to false was causing a core problem. What remains in my head as an issue is that checkUbloxI2C will process the everything sitting in the I2C buffer. This may contain multiple packets (for example a config response, then an ACK, then incoming PVT data). If we process the entire I2C buffer we run the risk of overwriting the packetCfg data with PVT data. If we stop processing the I2C buffer once a valid packet is detected (in this example, the config response) then we run the risk of loosing all the data sitting in the I2C hardware buffer (may be platform specific which is a whole other thing that I don't want to deal with).

So, how do we retrieve all the incoming bytes from I2C while making the library aware that things are happening? Rather than sticking to two ubxPackets (Config and Ack) perhaps we could create new ubxPacket structs as packets are validated, then have the library look at the array of ubxPackets to decide what to do with them.

I worry this would explode the RAM footprint of this library (that is already pretty big).

I'm going to try to fix the current lib using the two packet (config/ack) approach. Once we return to something that is stable, I'd love your help making it better.

[nseidle]

I've implemented your suggestions on this PR. It's a big one. Please let me know what you think.