Added parsing ZFO and ZTG messages #87
Conversation
| /// The number of milliseconds in a hour. | ||
| const MILLISECS_PER_HOUR: i64 = 3600000; | ||
|
|
||
| pub(crate) fn parse_duration_hms(i: &str) -> IResult<&str, Duration> { |
There was a problem hiding this comment.
From what I see, all values can be negative and we should check this case.
Another thing that would be good to do is avoid using i64 and only when creating the Duration to cast the value to i64.
This will give us only 1 place where we should check if we can cast e.g. u32, u64 to i64.
src/sentences/utils.rs
Outdated
| pub(crate) fn parse_duration_hms(i: &str) -> IResult<&str, Duration> { | ||
| map_res( | ||
| tuple(( | ||
| map_res(take(2usize), parse_num::<i64>), |
There was a problem hiding this comment.
- Please use a different value for the number, e.g.
u8will also be sufficient.
There was a problem hiding this comment.
i64 makes easier following conversion to Duration (as Duration accepts i64).
There was a problem hiding this comment.
It would be better to fail value parsing as early as possible.
And Yes, Duration takes i64, however, we could easily convert it with From: https://doc.rust-lang.org/std/primitive.i64.html#impl-From%3Cu8%3E-for-i64
It's better to parse a mostly correct value, instead of very wrong value. parsing i64 could also take longer and can have bigger impact on constraint device since this first parser can actually succeed event with e.g. negative value.
Even if the architecture is 32 bit, 64 bit numbers are still supported but not by the hardware itself, so are less performant.
src/sentences/utils.rs
Outdated
| if sec >= 60. { | ||
| return Err("Invalid time: sec >= 60"); | ||
| } |
There was a problem hiding this comment.
Also check for < 0.0 and NaN (https://doc.rust-lang.org/nightly/std/primitive.f64.html#method.is_nan)
|
Thank you for your PR! I've left some improvements notes, which should be reviewed and applied to the rest of the sentences as well. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 80.22% 80.26% +0.04%
==========================================
Files 31 33 +2
Lines 1153 1196 +43
==========================================
+ Hits 925 960 +35
- Misses 228 236 +8
☔ View full report in Codecov by Sentry. |
Added tests for ParameterLength
|
Thank you once again for your contributions @taavit ! |
Based on:
https://gpsd.gitlab.io/gpsd/NMEA.html#_zfo_utc_time_from_origin_waypoint
https://gpsd.gitlab.io/gpsd/NMEA.html#_ztg_utc_time_to_destination_waypoint