Thread: [Gpsbabel-code] Portable serial port
Brought to you by:
robertl
From: Andy A. <an...@he...> - 2006-07-12 22:38:55
Attachments:
gbser.patch
|
Here's an implementation of gbser (portable serial port support). So far it's been tested on Windows and OS X both with a WBT-200 and a little test suite / loopback cable. The test program isn't included in the patch - if anyone has two serial ports and a null modem cable to connect them and would like it let me know. |
From: Andy A. <an...@he...> - 2006-07-13 01:29:17
|
On 12 Jul 2006, at 23:38, Andy Armstrong wrote: > Here's an implementation of gbser (portable serial port support). > So far it's been tested on Windows and OS X both with a WBT-200 and > a little test suite / loopback cable. Tested and working on Linux now too (2.6.17.4 kernel, cp2101 support + WBT-200). Oh, I didn't just post this patch out of the blue by the way - Robert suggested I should :) -- Andy Armstrong, hexten.net |
From: Robert L. <rob...@us...> - 2006-07-13 02:08:16
|
I now have time to review this stuff and sourceforget is down. Again. Grrr. > Here's an implementation of gbser (portable serial port support). So Cool. Comments below. - inifile.o garmin_fs.o gbsleep.o units.o @GBSER@ \ + inifile.o garmin_fs.o gbsleep.o units.o gbser.o \ We've waffled on this a few times through the years. I think it really is best on files like this where we have essentially function() { #if WINDOWS do something #else do something about exactly different. #endif } for every function in the file to just split them. Yes, I know the arguments like 'what if the signature of function() changes' but the reality is that the inline approach doesn't scale well. If some masochist decides to port it to Palm/OS or something, the file quickly gets out of hand. Obviously, it's not a hard rule (gb_sleep, for example, would just be pointless to split that way - it's one function...) but I think the serial stuff is complicated enough to justify keeping it in separate files. I don't even mind a gbser_common.c that wraps gbser_foo and calls gbser_os_foo() to let preconditioning (as you have in your checks for word size and parity) and postconditioning work sensibly. - { { echo "$as_me:$LINENO: error: libusb >= 0.1.8 is needed" >&5 -echo "$as_me: error: libusb >= 0.1.8 is needed" >&2;} - { (exit 1); exit 1; }; } +# ,[AC_MSG_ERROR([libusb >= 0.1.8 is needed])] Is this just merge noise? +void *gbser_init(const char *port_name) { +#ifdef __WIN32__ + HANDLE comport; + const char *prefix = "\\\\.\\\\"; + gbser_handle* h = xcalloc(1, sizeof(*h)); + const char *xname = fix_win_serial_name(port_name); Prefix has been obsoleted here. + for (ap = 0; ap < sizeof(arg) / sizeof(arg[0]); ap++) { + unsigned t = 0; + int pl; + while (isspace(*spec)) { spec++; } Would this be easier to read if we copied the incoming string and called strtok or strchr to tokenize it and then let atoi/sscanf do the deed? I do like the API and the implementation looks right to me. The smooshing of all the OSes into one C file is my only real grumble with it. Thanx for tackling this! RJL |
From: Andy A. <an...@he...> - 2006-07-13 09:20:52
|
On 13 Jul 2006, at 03:08, Robert Lipe wrote: > We've waffled on this a few times through the years. I think it really > is best on files like this where we have essentially > > function() > { > #if WINDOWS > do something > #else > do something about exactly different. > #endif > } > > for every function in the file to just split them. Yes, I know the > arguments like 'what if the signature of function() changes' but > the reality is that the inline approach doesn't scale well. If some > masochist decides to port it to Palm/OS or something, the file quickly > gets out of hand. OK. In this case only fewer than half the functions contain conditionals - and some of them share common conditioning code at the top. It's useful to have the implementations side by side so I could easily verify that they're functionally equivalent. > Obviously, it's not a hard rule (gb_sleep, for example, would just be > pointless to split that way - it's one function...) but I think the > serial stuff is complicated enough to justify keeping it in separate > files. Probably three files then... > I don't even mind a gbser_common.c that wraps gbser_foo and calls > gbser_os_foo() to let preconditioning (as you have in your checks for > word size and parity) and postconditioning work sensibly. OK. I'm inclined to do the split once I know the code's settled down though... > - { { echo "$as_me:$LINENO: error: libusb >= 0.1.8 is needed" >&5 > -echo "$as_me: error: libusb >= 0.1.8 is needed" >&2;} > - { (exit 1); exit 1; }; } > +# ,[AC_MSG_ERROR([libusb >= 0.1.8 is needed])] > > Is this just merge noise? Odd. Yes - not supposed to be there. > +void *gbser_init(const char *port_name) { > +#ifdef __WIN32__ > + HANDLE comport; > + const char *prefix = "\\\\.\\\\"; > + gbser_handle* h = xcalloc(1, sizeof(*h)); > + const char *xname = fix_win_serial_name(port_name); > > Prefix has been obsoleted here. Ah yes :) > + for (ap = 0; ap < sizeof(arg) / sizeof(arg[0]); ap++) { > + unsigned t = 0; > + int pl; > + while (isspace(*spec)) { spec++; } > > Would this be easier to read if we copied the incoming string and > called strtok or strchr to tokenize it and then let atoi/sscanf do > the deed? Well /I/ find that easier to read - but then I like Perl too :) I've tended to avoid strtok() for years - it seemed to be a prime target for broken (or at least non-threadsafe) implementations - although I'm sure it's better now. I see it's being obsoleted by strsep() now too. All of which confusion is why I tend just to inline trivial parsing - and why I tend to be used to reading the resulting code. But that's all pretty much cargo culting on my part and I'll happily change it :) > I do like the API and the implementation looks right to me. The > smooshing of all the OSes into one C file is my only real grumble with > it. OK. Well my plan then would be to migrate the Magellan code to use it and test it thoroughly with that and then - assuming the API has settled down by then - move it into three files - one common, one posix, one Windows. OK? > Thanx for tackling this! -^4 metadata next :) -- Andy Armstrong, hexten.net |
From: Robert L. <rob...@us...> - 2006-07-13 16:02:29
|
> conditionals - and some of them share common conditioning code at the > top. It's useful to have the implementations side by side so I could > easily verify that they're functionally equivalent. For two serial layers, it's only moderately painful. It doesn't scale well. > > serial stuff is complicated enough to justify keeping it in separate > > files. > > Probably three files then... Please. Something like a gbser_common.c, gbser_posix.c, and gbser_windows.c should hit the spot. You don't even have to do the cute "register the entry points" thing that I did in the USB layer. If gbser_posix and gbser_windows both provide gbser_OS_write() and gbser_common knows that's the symbol's name and relies on Makefile/configure/MSVC configuration to bring in the right module, that's still cool. + if (!isdigit(*spec)) { break; } + while (isdigit(*spec)) { t = t * 10 + *spec++ - '0'; } > Well /I/ find that easier to read - but then I like Perl too :) It's not a comment on that code specifically, but I've spent too much of my life fixing code that basically looks like that. I find t = atoi(spec); much clearer than the above. If I need to know what I've consumed t = strtol(spec, &spec, 10) isn't bad, either. Scanf isn't without problems, but if you know your strings avoid the problems in it (and most do) it's easy to maintain. > I've tended to avoid strtok() for years - it seemed to be a prime > target for broken (or at least non-threadsafe) implementations - > although I'm sure it's better now. I see it's being obsoleted by > strsep() now too. All of which confusion is why I tend just to inline strsep isn't ansi C, but it is indeed increasingly pervasive. If strsep is what it takes to eliminate another open-coded parser, I'll add one. > and test it thoroughly with that and then - assuming the API has > settled down by then - move it into three files - one common, one > posix, one Windows. Please. RJL |
From: Andy A. <an...@he...> - 2006-07-13 16:35:58
|
On 13 Jul 2006, at 17:02, Robert Lipe wrote: > For two serial layers, it's only moderately painful. It doesn't scale > well. Scaling's kind of hypothetical right now though, no? Are we really going to find a new platform that isn't either Posix or Win32 any time soon? You mentioned Palm - and I know that was just an example - but Palm is a crazy enough environment that the configuration of the source for the serial port would be just about the last of anyone's worries. >>> serial stuff is complicated enough to justify keeping it in separate >>> files. >> >> Probably three files then... > > Please. Something like a gbser_common.c, gbser_posix.c, and > gbser_windows.c should hit the spot. OK, if you insist :) I'm inclined to wait until the code has settled though. It's much easier to maintain in its current form and the spectre of having to scale to another platform isn't pressing is it? >> Well /I/ find that easier to read - but then I like Perl too :) > > It's not a comment on that code specifically, but I've spent too > much of > my life fixing code that basically looks like that. > > I find > t = atoi(spec); > much clearer than the above. If I need to know what I've consumed > t = strtol(spec, &spec, 10) > isn't bad, either. Ok - I think there's still going to have to be some inline character- at-a-time parsing to skip any trailing spaces after numbers. -- Andy Armstrong, hexten.net |
From: Robert L. <rob...@us...> - 2006-07-13 17:43:40
|
> Scaling's kind of hypothetical right now though, no? Are we really > going to find a new platform that isn't either Posix or Win32 any They exist. There are a couple of guys on the list porting it to all kinds of wacky things. > > Please. Something like a gbser_common.c, gbser_posix.c, and > > gbser_windows.c should hit the spot. > > OK, if you insist :) Yes, I do. :-) > > t = atoi(spec); > > much clearer than the above. If I need to know what I've consumed > > t = strtol(spec, &spec, 10) > > isn't bad, either. > > Ok - I think there's still going to have to be some inline character- > at-a-time parsing to skip any trailing spaces after numbers. The next atoi/strtoul will consume them. RJL |
From: Andy A. <an...@he...> - 2006-07-13 17:55:50
|
On 13 Jul 2006, at 18:43, Robert Lipe wrote: >> Ok - I think there's still going to have to be some inline character- >> at-a-time parsing to skip any trailing spaces after numbers. > > The next atoi/strtoul will consume them. They're between a digit and a comma - like this 9600 ,8 -- Andy Armstrong, hexten.net |
From: Robert L. <rob...@us...> - 2006-07-13 19:32:31
|
> They're between a digit and a comma - like this > > 9600 ,8 Scanf approach: sscanf(t , "%d%[^,],%d", &x, &junk, &y)); Even if you don't do scanf, standard C is still our friend: x = strtol(t, &t, 10); t = strchr(t, ',') + 1; y = strtol(t, &t, 10); |
From: Andy A. <an...@he...> - 2006-07-13 19:35:22
|
On 13 Jul 2006, at 20:32, Robert Lipe wrote: > Scanf approach: > sscanf(t , "%d%[^,],%d", &x, &junk, &y)); > > Even if you don't do scanf, standard C is still our friend: > > x = strtol(t, &t, 10); > t = strchr(t, ',') + 1; > y = strtol(t, &t, 10); But then it doesn't reject syntax errors... -- Andy Armstrong, hexten.net |