From: Alex B. <a.b...@ac...> - 2008-02-25 11:59:50
|
Hey Geoff, > Here's my review of the serial library. Thanks. Comments inline. > -Nice to haves: > - Configurable data bits and other comms parameters. Yeah, possibly. I wouldn't say I'm a serial comms expert, so my intended strategy was to expand the set of configurable things as I encountered a need for them (and am forced to understand them). > -Other: Does the name have to start with "gbx"? Not necessarily. Personally when I read dependent code I like having a prefix that tells me where things come from. Does anyone have a strong preference/reason for or against? > Documentaiton: > . Documentation for the lockfile object isn't included in built > documentation. Would be nice to have it available. Done. > API: > - Could the constructor take a timeout value rather than just "enable" > which sets a default, with 0 being disable timeouts? Yep, fair enough, this is confusing. Done now. > ? Why can't timeouts be disabled/enabled after construction? En/dis-abling of timeouts effects the O_NONBLOCK flag that's passed to open() when the serial port is first opened. > - readLine returns data in a NULL-terminated void*, writeString takes a > NULL-terminated char*. This is confusingly inconsistent. But readLine() and writeString() do slightly different things: - readLine() reads a line of text (ie terminated by a '\n' - writeString() writes a C-style string (ie terminated by '\0') > Either readLine should put the data in a char* and renamed "readString", I don't much like readString(), because it's not clear how the string is delimited. readLine() makes sense to me because it's like the 'man readline' version (which reads things delimited by \n). To me readString says 'delimited by \0'. I know the delimiter's configurable, but a lot of serial devices deal in '\n'-terminated chunks, so I reckon readLine() is more readable. > or it should be > renamed to something like "readUntil" to more clearly illustrate that it > reads until a particular byte is received (I prefer the latter). I don't much like readUntil() either. You'd have to remove the default argument or dependent code would be unclear. And then I reckon "serial.readUntil( buf, BUF_SIZE, '\n' )" is not as nice as "serial.readLine( buf, BUF_SIZE )". I'd be happy maybe with: int readUntil(void *buf, int count, char termchar ); int readLine(void *buf, int count ) { return readUntil(buf,count,'\n'); } What about that? I don't have a strong preference for 'char*' vs 'void*'. The void* is there just to mimick the ::read() function. Not sure which is better. > - readLine claims to read a line, but in reality will muck up the end of > a line from a device that uses \r\n or \r as its line separator. Bugger. Agree that this would be nice, but I'm not too inclined to implement and test it until I have to. I'd call it a nice-to-have. > - const int's (or #define's) for debug levels rather than hard-coded > integer values spread throughout the source would make the debug level > code a bit more obvious. What do you mean? Like associate each debug level with some kind of meaning, like: #define DEBUG_OPEN_CLOSE 1 #define DEBUG_FUNCTIONS 2 #define DEBUG_DATA 3 or something like that? To be honest I hadn't put that much thought into what each level means... > - Install an example to share/gearbox/? Done. I copied your example, that's pretty sweet. But I didn't like the fact that it over-writes example.cmake, because I like to do in-source builds. So I added a parameter to GBX_ADD_EXAMPLE so it's now: # GBX_ADD_EXAMPLE( install_subdir makefile.in makefile.out [FILE0 FILE1 FILE2 ...] ) And updated your CMakeLists.txt accordingly. Happy with that? > =========== > = Specific > =========== > > serial.h: > * 50: Throws what exceptions? Clarify the comment. > - 153: The comment says it won't throw exceptions, but the function > doesn't have an exception specification guaranteeing this. Both done. > serial.cpp: > . 44: Is the swearing really necessary? :) I guess it probably was... > * 466: A TODO should be added to the documentation to make sure this > doesn't get forgotten. Does adding TODO really help? I imagine it will get ignored until it doesn't work for someone, and when that happens they're unlikely to grep for TODO as a first step... Added it anyway though. > - 772: Could writeString wait for the timeout before trying again if the > buffer is full? If it's still full after the timeout, then fail as it > currently does. I don't think we want to enforce that. And it's not clear what timeout to use, I'm not sure the read timeout is the right thing. A careful user will check the return code of writeString(), so the situation isn't terrible if this condition gets hit. > lockfile.cpp: > * 27: What happens if LOCK_DIR doesn't exist? Can this be made > configurable some how? At least document the need for that directory in > the library documentation. It's now documented in the header. Cheers, Alex |