Remove Talker ID test from verify

[da2x]

Turns out Talker IDs don’t have to start with "$G" as I first believed. BeiDou identifies as either "$GB" or "$BD". Likewise, QZSS as either "$GP" (shared ID with GPS) or "$QZ" . Other Talker IDs less relevant to positional data that don’t start with G also exist.

[zeenix]

It sucks a bit that we have less things to check now but what can you do. So good catch.

Other than actually parsing a full NMEA sentence and verifying it’s checksum? Not sure. This check can cause false positives, but it will also work with every device producing NMEA. I think it’s better than it works with more devices in more markets than guarding against it sometimes picking the wrong device.

Could use a regex like \$[A-Z]{5}, but that would be a lot slower depending on the number of connected devices.

We are not expecting a lot of devices. The typical case would be 1 device but I'd be surprised if users have more than 2-3. Still, best to have very fast detection so how about checking of ascii instead after the $?

I did not realized that in the last patch you introduced an 'unwrap()' call. 'unwrap()' should be avoided and when used, a comment briefly explaining why it's justified in this case would be good. If you keep the unwrap(), you could add the comment in a separate patch on top of this. If you decide to remove these calls, the patch to remove it, should go before this one.

Character 0 and 6 can’t be None as the buffer length is guaranteed to be 15+ characters. (Right?) Are there any other issues with unwrap() than guarding against None panics? It seemed like the simplest tool to get the job done.

Yeah I guessed that's your reason but it's best to add a comment briefly pointed this out so devs ignore the unwrap() call.

[zeenix]

Character 0 and 6 can’t be None as the buffer length is guaranteed to be 15+ characters. (Right?) Are there any other issues with unwrap() than guarding against None panics? It seemed like the simplest tool to get the job done.

Yeah I guessed that's your reason but it's best to add a comment briefly pointed this out so devs ignore the unwrap() call.

Actually in this case there is a way to avoid the unwrap calls completely:

if buffer.len() >=15 &&
  buffer.chars().nth(6).map(|c| c == ',').unwrap_or(false)

I know it's a bit verbose but let's avoid unwrap call if we can. If we had to check multiple characters like this, we could put this code in a small utility function.

Anyway, I already changed the code i master to do it this way so you just need to rebase & adapt your patch now. :)

[zeenix]

Now I realized it could be so simple :)

-                    buffer.chars().nth(6).map(|c| c == ',').unwrap_or(false)
+                    buffer.chars().nth(6) == Some(',')