Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequence number rollover logic is incorrect. #42

Open
chauser opened this issue May 31, 2024 · 0 comments
Open

Sequence number rollover logic is incorrect. #42

chauser opened this issue May 31, 2024 · 0 comments

Comments

@chauser
Copy link

chauser commented May 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant