wpilibsuite / xrp-wpilib-firmware

6 stars 7 forks source link

Sequence number rollover logic is incorrect. #42

Open chauser opened 1 month ago

chauser commented 1 month ago

wpilibudp.cpp processPacket has logic to handle rollover of 16-bit udp packet sequence numbers in the protocol the robot uses to talk to the simulation.

// Check if the sequence number exceeds our latest seen seq number
  if (seq > currMaxSeq) {
    currMaxSeq = seq;
  }
  else {
    if (SEQ_MAX - seq < SEQ_FUDGE_FACTOR) {
      // Rollover
      currMaxSeq = seq;
    }
    else {
      // Not processing this
      return false;
    }

where SEQ_MAX == 65535 and SEQ_FUDGE_FACTOR == 5. The test if (SEQ_MAX - seq < SEQ_FUDGE_FACTOR) { will only be true if seq is very close to SEQ_MAX but also is only evaluated if seq <= currMaxSeq; so it's unlikely that the Rollover code is ever used. Even if it is used, it sets currMaxSeq to a large number. I believe the test should be

if (seq < SEQ_FUDGE_FACTOR)

This will allow currMaxSeq to go back to a value close to 0 when rollover occurs and also allow that to happen only when seq is close to zero.