fix garmin serial/usb device character encoding/decoding. #1117

tsteven4 · 2023-05-15T19:21:59Z

We got away with breaking this for a long time, but we finally got caught.

This resolves #1115

@svintuss has tested both read and write on his Garmin GPSmap 60cx using windows-1251.

the garmin writer is unchanged as of yet.

codacy-production · 2023-05-15T19:31:00Z

Coverage summary from Codacy

Merging #1117 (b4181cd) into master (488aa41) - See PR on Codacy

Coverage variation	Diff coverage
-0.02%	0.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`488aa41`)	24154	16374	67.79%
Head commit (`b4181cd`)	24161 (+7)	16374 (+0)	67.77% (-0.02%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1117)	44	0	0.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

GPSBabelDeveloper · 2023-05-15T19:49:27Z

On phone, but I think I like it.
Is there a way to connect this to the switch on device id that sets all the things we "know" about eath model like like waypoint length and lower case capability (a loose proxy for CP -125x if we have nothing better?
I think if we send a gpx with international characters to something like a gps II or a. 12 or a 3 or something, the device will crash,. So we should be conservative in doing that, but the USB -capable devices are less broken.

... And most of my hyperactive heart attack in this code is in the layer below. It's mostly the jeeps level of stuff that can break a quarter of the world. A streetpioli and a 60 are both USB, but very different in the wire. Heck, a 60c and 60cx are very different.

tsteven4 · 2023-05-15T20:04:57Z

By default the receiver charset is US-ASCII. The switch on gps_waypt_type and gps_save_id may update this to windows-1252. After all that if the codec option was used we will override the automatically selected codec with whatever was asked for in that option.

At this point on all platforms, with Qt5 or Qt6, we should have access to at least the minimal set of "supported encodings" promised by Qt5. This works on Qt6 because we are using the Qt 5 Core Compat module. Included in the minimal set of supported encodings is Windows-1250 to 1258.

If you feel like it checking the lengths of the converted strings in the writer might be helpful. Also looking for any implicit conversions in the reader I missed (I did catch a couple).

robertlipe · 2023-05-15T21:15:24Z

Is his ID missed in the switch that would have auto selected 1252? Maybe it's a localized device with a different ID. Garmin does that. We've had to special cases devices in the market for Japanese and such before. They would get the default, disallowing spaces and lowercase and such. We'll lose qt5cimoat with qt7 ... or before. Without that do we have at least minimal common codecs? (I don't much care about the full list of 200 or whatever that you just posted. 125x, latin1, utf8/16, and ...)

…

On Mon, May 15, 2023, 3:05 PM tsteven4 ***@***.***> wrote: By default the receiver charset is US-ASCII. The switch on gps_waypt_type and gps_save_id may update this to windows-1252. After all that if the codec option was used we will override the automatically selected codec with whatever was asked for in that option. At this point on all platforms, with Qt5 or Qt6, we should have access to at least the minimal set of "supported encodings" promised by Qt5 <https://doc.qt.io/qt-5/qtextcodec.html>. This works on Qt6 because we are using the Qt 5 Core Compat module. Included in the minimal set of supported encodings is Windows-1250 to 1258. If you feel like it checking the lengths of the converted strings in the writer might be helpful. Also looking for any implicit conversions in the reader I missed (I did catch a couple). — Reply to this email directly, view it on GitHub <#1117 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD33Q2R5ZU7UV3N3U3YLXGKD7LANCNFSM6AAAAAAYCT76PU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

tsteven4 · 2023-05-15T21:20:07Z

I don't think it is missed. Before I added the codec option he was seeing the line
fprintf(stdout, "reciever charset detected as %s.\r\n", receiver_charset);
print windows-1252.

I think were good with Qt5Compat until Qt7, then I think we will only have codecs if Qt is built with ICU support. And I don't think the Qt supplied builds don't have ICU support enabled on all platforms.

in garmin writer.

robertlipe

I quite like it. Merge it down whenever you're ready.

robertlipe · 2023-05-16T06:37:41Z

garmin.cc

+  assert(!QString(valid_waypt_chars).contains('^'));
+  assert(!QString(valid_waypt_chars).contains('-'));
+  assert(!QString(valid_waypt_chars).contains('['));
+  assert(!QString(valid_waypt_chars).contains(']'));


These could be a single QString::contains(const QRegularExpression &re, but it would admittedly be a quoting nightmare and wouldn't give you a single error message like this will. Your call.

Yes, and it would save a bunch of QString construction. But, the code is only run on init, and only in debug builds, so I thought clarity was better.

garmin.cc

reference/help.txt

garmin.cc

tsteven4 · 2023-05-16T18:47:54Z

@robertlipe,
This seems fishy to me. If we must upper, we do. Then we throw away everything but A-Z 0-9, and maybe space, + and -. If we don't force to uppercase we throw away a-z. Was that the intent or should a case insensitive compare have been done?

gpsbabel/garmin.cc

Lines 1012 to 1021 in 488aa41

    
           char* d = rte->ident; 
        
           for (auto idx : wpt->shortname) { 
        
             int c = idx.toLatin1(); 
        
             if (receiver_must_upper && isalpha(c)) { 
        
               c = toupper(c); 
        
             } 
        
             if (strchr(valid_waypt_chars, c)) { 
        
               *d++ = c; 
        
             } 
        
           }

codacy-production · 2023-05-16T20:40:00Z

Coverage summary from Codacy

Merging #1117 (ced87b9) into master (4aa594a) - See PR on Codacy

Coverage variation	Diff coverage
-0.02%	0.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`4aa594a`)	24140	16420	68.02%
Head commit (`ced87b9`)	24146 (+6)	16420 (+0)	68.00% (-0.02%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#1117)	43	0	0.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

robertlipe · 2023-05-17T01:03:31Z

You're right. That loop at 1012 is underachieving. We toss those those instead of "fixing" them, but such a receiver would have surely had 'must_upper' set, so it's presumably a very minority case.

I'd expect the right thing to do here is replacing this with a call to write_char_string after a conditional toUpper on the string if mustupper is set.

I think you're done here. Nice work!

We clear badchars for newer devices already, effectively not cleaning wpt names.

tsteven4 · 2023-05-19T18:28:44Z

@robertlipe are you good with this resolution of rtept names? For wpt names we defeat the cleaning function of mkshort when receiver_must_upper is false, see the similar comment in the code.

gpsbabel/garmin.cc

Lines 1058 to 1065 in 911ab99

    
             QString cleanname = wpt->shortname; 
        
             /* 
        
              * Until Garmin documents how to determine valid character space 
        
              * for the new models, we just release this safety check manually. 
        
              */ 
        
             if (receiver_must_upper) { 
        
               cleanname = cleanname.toUpper().remove(invalid_char_re); 
        
             }

robertlipe · 2023-05-19T18:32:42Z

Yes. It's definitely less wrong than what we already have, so commit this, please. There may be other dragons lurking, but these are dark, dark corners so let's not deadlock on "perfect". Thanx!

…

On Fri, May 19, 2023 at 1:28 PM tsteven4 ***@***.***> wrote: @robertlipe <https://github.com/robertlipe> are you good with this resolution of rtept names? For wpt names we defeat the cleaning function of mkshort when receiver_must_upper is false, see the similar comment in the code. https://github.com/GPSBabel/gpsbabel/blob/911ab99766b7eb84336be8863f857ffd872ae4f6/garmin.cc#L1058-L1065 — Reply to this email directly, view it on GitHub <#1117 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3ZEPB2DLKGVIQOD2V3XG63WPANCNFSM6AAAAAAYCT76PU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

* attempt to use correct codec with garmin reader. the garmin writer is unchanged as of yet. * fix garmin writer wrt encoding. * add garmin option to force codec. * update serialization ref files for garmin codec option. * fix potential overwrite bug with route/track names in garmin writer. * fix up for old Qt. * correct assertion * use static_assert to for fixed buffer size checks. * document garmin codec option * don't clean rtept/trkpt names for newer devices. We clear badchars for newer devices already, effectively not cleaning wpt names.

tsteven4 added 5 commits May 15, 2023 10:19

attempt to use correct codec with garmin reader.

37c1201

the garmin writer is unchanged as of yet.

fix garmin writer wrt encoding.

c1d5baa

add garmin option to force codec.

47159d5

Merge branch 'master' into garmincodec

1a82edb

update serialization ref files for garmin codec option.

2d5a206

tsteven4 mentioned this pull request May 15, 2023

Converting encoding of waypoints names #1115

Closed

tsteven4 added 4 commits May 15, 2023 17:00

fix potential overwrite bug with route/track names

9bea843

in garmin writer.

fix up for old Qt.

1e25ad2

correct assertion

9c1e1af

use static_assert to for fixed buffer size checks.

b4181cd

robertlipe approved these changes May 16, 2023

View reviewed changes

document garmin codec option

89b95cc

don't clean rtept/trkpt names for newer devices.

911ab99

We clear badchars for newer devices already, effectively not cleaning wpt names.

Merge branch 'master' into garmincodec

ced87b9

tsteven4 merged commit c3e7b7e into master May 19, 2023

tsteven4 deleted the garmincodec branch May 19, 2023 19:02

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix garmin serial/usb device character encoding/decoding. #1117

fix garmin serial/usb device character encoding/decoding. #1117

tsteven4 commented May 15, 2023

codacy-production bot commented May 15, 2023 •

edited

Loading

GPSBabelDeveloper commented May 15, 2023

tsteven4 commented May 15, 2023

robertlipe commented May 15, 2023 via email

tsteven4 commented May 15, 2023

robertlipe left a comment

robertlipe May 16, 2023

tsteven4 May 16, 2023

tsteven4 commented May 16, 2023

codacy-production bot commented May 16, 2023 •

edited

Loading

robertlipe commented May 17, 2023

tsteven4 commented May 19, 2023

robertlipe commented May 19, 2023 via email

fix garmin serial/usb device character encoding/decoding. #1117

fix garmin serial/usb device character encoding/decoding. #1117

Conversation

tsteven4 commented May 15, 2023

codacy-production bot commented May 15, 2023 • edited Loading

Coverage summary from Codacy

See your quality gate settings Change summary preferences

GPSBabelDeveloper commented May 15, 2023

tsteven4 commented May 15, 2023

robertlipe commented May 15, 2023 via email

tsteven4 commented May 15, 2023

robertlipe left a comment

Choose a reason for hiding this comment

robertlipe May 16, 2023

Choose a reason for hiding this comment

tsteven4 May 16, 2023

Choose a reason for hiding this comment

tsteven4 commented May 16, 2023

codacy-production bot commented May 16, 2023 • edited Loading

Coverage summary from Codacy

See your quality gate settings Change summary preferences

robertlipe commented May 17, 2023

tsteven4 commented May 19, 2023

robertlipe commented May 19, 2023 via email

codacy-production bot commented May 15, 2023 •

edited

Loading

codacy-production bot commented May 16, 2023 •

edited

Loading