Skip to content

Conversation

@nogakeren
Copy link

@nogakeren nogakeren commented Aug 8, 2025

Hi, this PR allows the SNR field to be a float (happens in some of the mediatek chipsets).

@hraftery
Copy link
Contributor

hraftery commented Aug 9, 2025

Note change 1 will be made obsolete by #90 .

@nogakeren nogakeren force-pushed the feature/handle-mediatek-nmea branch from 61b7853 to 98fecfe Compare August 9, 2025 09:41
@nogakeren
Copy link
Author

@hraftery removed first change

@nogakeren
Copy link
Author

@kosma hi, can you please review my PR?

@kosma
Copy link
Owner

kosma commented Aug 17, 2025

I'm looking at the PR and I'm slightly confused,

    if (!minmea_scan(sentence, "tiii;iiiiiiiiiiiiiiii",
    if (!minmea_scan(sentence, "tiii;iiifiiifiiifiiif",

The i format accepts an int *, while f accepts struct minmea_float *. If you change the format specifiers, you also have to change the field types here:

struct minmea_sat_info {
    int nr;
    int elevation;
    int azimuth;
    int snr;
};

Otherwise you're overwriting one byte past the structure and causing memory corruption. I'm surprised the tests pass. :o

@nogakeren nogakeren force-pushed the feature/handle-mediatek-nmea branch from 98fecfe to 7dd3c48 Compare August 22, 2025 18:29
@nogakeren
Copy link
Author

@kosma nice catch, pushed a fixed version.
Are you cool with this API break, or should I keep SNR as an int and scan into some temp variable?

@nogakeren
Copy link
Author

@kosma - can you please CR this change?

@kosma
Copy link
Owner

kosma commented Nov 3, 2025

@nogakeren

  1. I'm okay with the API change, it will cause a compiler error, so the user will know it needs updating.
  2. The example in README and in example.c needs to be updated for the API change.
  3. We should test parsing a float SNR value, for example in test_minmea_parse_gsv2.

 On some of Mediatek chipsets, the SNR field is float instead of integer.
@nogakeren nogakeren force-pushed the feature/handle-mediatek-nmea branch from 7dd3c48 to 14501f1 Compare November 7, 2025 13:09
@nogakeren
Copy link
Author

@kosma
2. done
3. added a new test - test_minmea_parse_gsv6

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

Successfully merging this pull request may close these issues.

3 participants