From: Alex B. <a.b...@ac...> - 2008-02-22 02:59:54
|
Hey Michael, Thanks for the review. Comments inline. > ---------------------------------------------------------------------- > -Relevance: high, there are many sensors w. serial interface > -Functionality: works reliably; used for NovatelSpan driver in > Orca/Hydro, which I maintain. > -Criticism: used/tested with a limited set of hardware, > doesn't seem to work in 500k mode (sick), > "laser-centric", i.e. current main purpose is > to run a sick LMS Re: 'laser-centric': we actually use it for a bunch of other devices on the Segway project. > I've prefixed all my comments with {*|-|.}, meaning: > * I think this needs to be fixed before acceptance. > - This could be better and I'd strongly recommend fixing it, but I'd > vote for acceptance regardless. > . This is merely a suggestion which the author is free to ignore. > ? Not sure what's going on. > > > =========== > = General: > =========== > API: > . read() handles timeouts differently (throws immediately) from > readFull() and readLine() (return -1 after timeout) Fair point -- eveything now returns '-1' on timeout. > . setDebugLevel() implies multiple levels, comment implies on/off, code > says: 0 -> off, 1 -> function calls, 2 -> data, 3 -> additional > control flow (levels are additiv) Yep, better comment now. > . several (debug) outputs with hardcoded file/function/linenr info (some > wrong). I'd recommend replacement with __FILE__ __LINE__ __func__ > __pretty_func__ macros, yes, some of them are gnu only, but some are > standard (__func__?) and this _is_ a Linux only lib at the moment. I don't agree with the 'linux-only lib at the moment' argument: I don't wanna have to go through and change everything back (and possibly introduce bugs in the process) some day. But __func__ seems pretty standard, so I'll go with that. Personally I hate usage of __FILE__ because it gives you the full path, so it makes the output unreadable. > ? Why are you using void *buf in all read functions? As opposed to what? char *buf? I think I was copying the Linux ::read() call. > Timeout: > . wouldn't it be more elegant to handle blocking/non-blocking with > select as well? Huh? I don't understand. > Exceptions: > . I'm fairly certain that I've seen string and or char* thrown from > serial in the past; > could these be caught internally and rethrown as SerialException? It should _only_ throw SerialException (and I can't find any instances where it throws anything else). > Examples: > . should the test programs show up as examples somewhere in the docu? Yep, added. > ============ > = Specific: > ============ > serial.cpp: > - 407: cout can throw afaik, making the destructor throw, not sure > whether that applies with a string constant. Not sure... I'll catch there just in case. > - 501: it's standard procedure to get the current termios settings, and > only change what one is interested in. However, I think it's also > wrong. > Consider: During testing you have enabled some arcane setting > manually (setserial and friends), which happens to > be pivotal to get your device to work. If termios attributes are > set additively it will work perfectly, > but only on the particular serial port with the manual setting. > This could be frustrating to users (dev answer to bug report: > works perfectly for me). > It can also bite the developer, if something changes (new > computer, reinstall, ...). If the manual setting has > long been forgotten debugging becomes a lot of fun. > The latter has happened to me once upon a time and cost me two > very frustrating days. I see the point. But I'm not so keen on touching anything here, for fear of breaking something, either on my system or somebody else's. > serial.h > . 13-18 too many includes? termios.h, types.h, fcntl.h not needed at removed. > all, lockfile.h not needed with forward declaration? I prefer it in the header if it doesn't pull in too much, forward declarations can make it hard to find dependent headers. > doc.dox > ? 35 What does the serial standard (rs232/422) have to do with the > driver? Don't know, but I've never tested on rs422. > ? License: refers to "LICENSE file included in this distribution", > orca/hydro left over? This is right isn't it?? There's a LICENSE file at the top level of GearBox. > lockfile.cpp > . 44 wouldn't it be better if these guys were strings (call .c_str() > where a char* is needed)? That would work too, but I don't see a problem as is. > . 113 shouldn't this be a positive test (errno == ESRCH), i.e. err on > the side of safty (not unlocking) Sounds good. Not sure what else might happen here, so there's now an exception thrown if it's anything else. > . 179 dev_.c_str() couldn't this throw? Not sure, catching now anyway. |