API review
Proposer: Eric Perko
Present at review:
- Jack O'Quin
- Mike Purvis
version of docs
Since the Hydro docs and Version macro are not yet up, I've put the Hydro version of the docs up at Proposed Hydro Docs
Question / concerns / comments
Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.
(Jack) Since we are changing the node name anyway, I recommend dropping the .py suffix. It seems better for the fact that the driver is written in Python not to be visible in the ROS API.
(Eric) Added as an Action Item
(Jack) There are other, non-serial interfaces that provide NMEA sentences, like from UDP packets. Is it reasonable to provide a Python interface for parsing data received via other means? What about for publishing the resulting ROS messages?
(Jack) I see you already have the nmea_topic_driver, that's even better.
(Eric) Yup, I fully intend on serial being just one of the ways that the driver can get NMEA strings. I don't know that we necessarily want to provide an NMEASentence publisher that listens on sockets in this package (at least I'm not aware of a common UDP/TCP protocol for sending NMEA strings), but I do want people to be able to write their own node that handles the "my own protocol->ROS NMEASentence" step and then can use a common driver to handle all of the translation of NMEA strings to standardized messages and diagnostics.
(Mike) My concept for this is in the fledgling nmea_comms package, which has serial and socket backends for a general nmea_msgs/Sentence message. I've written these against roscpp to get away from the frustrations of doing serial I/O from Python (and generally because we're looking to reduce CPU loading on our robot computers).
(Jack) I like the idea of a separate nmea_msgs package, it simplifies dependencies. Can the nmea_msgs/Sentence and nmea_gps_driver/NMEASentence messages be converged? I prefer the first of the two names. Mike, what do the talker and fields elements represent?
(Mike) talker="GP", sentence="RMC", fields=["081836","A","3751.65","S","14507.36"...] I'm not married to that separation, but the idea was for the sake of filtering, etc.
(Eric) Hmm... I was thinking the NMEASentence publishers would be kept as simple as possible - no parsing, checksum checking, etc required. Simply acquire a single sentence, timestamp it and push it out onto a ROS topic.
(Mike) I think it really depends where it can go with minimal repetition. My idea is to have the socket and serial adapter nodes plus a variety of interpreters which individually handle Twist, Fix, SVs, plus ones in external packages related to marine payloads and other uses. In an environment with many interpreters and few I/O adapters, it makes sense to push as much logic as possible on the adapters—but if the 99% use case is a single nmea navsat adapter which publishes Twist, Fix, and the SV message(s), then it may make more sense to put it there. Ultimately, it's only a few lines, and not really that error-prone, so it doesn't much matter.
(Mike) Another fun aspect of this is receivers which interleave binary messages with NMEA for the purposes of doing RTK or other stuff (u-blox does this, too). Neither proposal handles that scenario particularly well, though publishing bare strings would create the possibility of parameterizing additional header and footer strings for the node to divide on.
(Jack) How are the binary messages delimited? Is there a '$' prefix?
(Mike) No $ prefix. The protocol description is on page 78 of this pdf. In any case, if those messages are required, it would have to be a separate u-blox-specific node which listens to the serial device and extracts them, and then republishes the NMEA messages to be handled elsewhere (the possible existence of such a package is probably another solid argument for having the field separation and checksum logic on the interpretation side of things).
(Eric) Since these are not actually NMEA strings but appear to be a custom protocol, I don't think the device driver should even put these into NMEA Sentence messages on e.g. the /nmea_sentence topic, since it doesn't seem like they are NMEA sentences in any sense - the only common thing is that they are interleaved with NMEA sentences on the same serial port, right?
(Jack) Is there any error recovery the device interface can attempt if the checksum is wrong? Can it prompt the device to resend, or must it always ignore the message and wait for another?
(Eric) I assume it depends strongly on the device. I'm fairly certain that my test GPS (a !VenusXXX device from SparkFun) has a very minimal set of messages it can receive (and I don't think any of them are not chip specific binary messages), none of which can ask it to resend the last solution. I think if the user needs that level of functionality, they'll need to at least check the checksums when they read them from the device before publishing to the generic NMEA driver.
(Jack) This reminded me of another issue: Python3 is coming within the lifetime of this API. The std_msgs/String type is translated into str rather than bytes. NMEA seems to provide bytes with ASCII characters, not unicode. Something has to do the string conversion. Maybe we should specify uint8[] sentence in the message, and convert it in the parser. That's a bit of a pain, but it might be better not to make too many assumptions about what a device might send.
(Eric) Since the NMEA standard doesn't actually support binary format messages, is there any reason to _not_ publish the NMEA sentences as unicode strings? I'd like to avoid just a uint8[] since there shouldn't be any non-printable bytes in the message.
(Mike) I'm pretty sure NMEA does specify ASCII 32-127, though, so it shouldn't much matter. I think there are some devices who actually package binary blobs in NMEA sentences, but it's done by base64-encoding.
(Jack) As long as we are sure all the data are pure ASCII, the string representation is best. I suppose the senders must check their data if there is any question.
(Eric) It looks like NMEA0183 messages are all ASCII (see http://en.wikipedia.org/wiki/NMEA_0183 ). However there is NMEA2000 that is a binary protocol. We should simply specify that we are supporting NMEA0183 and that would constrain us to ASCII messages. We can either just note that in the message comments or actually change the name of the proposed message to nmea_msgs/NMEA0183Sentence or something.
(Mike) Just a heads-up; I've changed over my `nmea` repo to using a whole-sentence-as-string style message, as well as forked nmea_gps_driver to add a node which can accept it. Both Fuerte for now, unfortunately, but there they are. Using the C++ serial node paired with the Python interpreter seems to do good things for the overall processor load on Kingfisher's little Atom PC.
(Eric) Thanks for the info. Good to hear the proposed message format change worked out for you (as well as the newer, split up driver).
(Eric) Are there really enough NMEA specific messages to need a separate package or would it be better to simply create a gps_msgs or navsat_msgs package that could contain both an NMEASentence as well as messages to hold more detailed information regarding number of satellites and such?
(Mike) NMEA is (for better or worse) used for more than just GPS receivers. Our interest (at Clearpath) is because it gets used for a variety of other marine devices familiar to our survey users, including depth sounders, motion reference units, compasses, etc. There are even whole platforms controlled by proprietary NMEA messages (c.f. Bluefin's Standard Payload Interface (pdf), which is NMEA sentences over a TCP socket). My vote would be for a use-agnostic nmea_msgs package, and then stuff specific to satellite navigation added to sensor_msgs (as there are lots of GPS receivers which support better formats than NMEA). The nmea_msgs package would probably only ever just have the one message.
(Eric) I didn't realize NMEA was also being used as a control protocol... Since it is used for things completely unrelated to NavSat stuff, it makes sense to me to not put it in a navsat_msgs or gps_msgs package.
(Jack) That's news to me, too. It provides a convincing reason for a separate nmea_msgs package, even if it contains only one message. I no longer consider packages like that problematic, they are stable and easy to maintain. I would put it in a separate repository, and release it once for each ROS distro. Since it has almost no dependencies, other packages may freely use it.
(Jack) Is it really necessary for each NMEA source to parse the message before sending it? Don't they generally know whether they are talking to a GPS or a control interface?
(Eric) I don't think it's necessary, hence my preference for the nmea_msgs/Sentence to contain nothing more than a timestamp and a string containing one NMEA sentence.
(Jack) How about changing the nmea_topic_driver node name to nmea_gps_topic and the topic name to something like nmea_gps_sentence? That makes it clear what types of NMEA sentences are expected.
(Eric) Do you think it needs renamed given that it is a script inside the nmea_gps_driver and is not globally installed (or at least should not be)?
(Eric) Note that there may be non-GPS related messages that the driver could receive and reasonably understand - technically GLONASS or mixed GPS/GLONASS devices have separate talker IDs, as do INS systems (see Talker IDs on http://gpsd.berlios.de/NMEA.txt ). There isn't any reason why we can't read supported sentence types from a non-GPS device (e.g. GGA from a GLONASS device).
(Jack) Good point. In the sensor_msgs/NavSatFix review, we made a conscious decision to avoid the GPS acronym in favor of the more-general "navigation satellite" terminology, with "navsat" as an abbreviation. We should probably do that here, too. Even though most of receivers are GPS, NMEA itself is more general.
(Eric) Just to clarify, are you proposing changing the name of the package to nmea_navsat_driver (and other corresponding GPS to navsat where appropriate)?
(Jack) I guess I am, at least to the extent it makes sense. That proposal should be constrained by backwards compatibility requirements and the extent to which it's worth the effort involved.
(Eric) Since all of the node names are changing in this release, I think we can leave the existing deprecated nmea_gps_driver.py node in an nmea_gps_driver package and move all of the new nodes to nmea_navsat_driver. This new transitional nmea_gps_driver package may depend on nmea_navsat_driver for some of the private library functionality introduced under the hood in the Hydro branch.
(Mike) I'm not a fan of how the vel topic is produced. We have u-blox LEA6T units which, depending on their configuration, produce groundspeed without track, which causes an error on the existing driver. That may be an unavoidable quirk of our receiver, but it's lame.
(Eric) Can you open a defect on GitHub with a link to a bagfile with some sample messages here (preferably captured in a format compatible with the nmea_topic_driver.py node) and I'll put a fix in to prevent publishing vel in that case. I don't think we'll be able to get a vel message without the track since vel is a component velocity vector.
(Mike) Having looked again, it does seem like that receiver has different "modes", which cause these quirky outputs. For whatever reason, the default from the factory is Stationary, which leaves blank track and speed, and the Sea setting provides speed without track. Pedestrian and Automotive provide both, however, so that's what we're doing. At some point I'll capture bags of the other modes so that the nmea_navsat driver(s) can at least fail more gracefully than ValueErrors being thrown.
(Mike) How do we expose the SV data? New "Satellite" and "Satellites" messages in sensor_msgs?
(Eric) Definitely new messages, though I'm not sure if they'd be better in sensor_msgs or in a gps_msgs/navsat_msgs package
(Jack) Since the Hydro release looks like it's going to be delayed somewhat, we should hold a separate sensor_msgs review to define this, as soon as possible. I'll raise the issue on the ros-sig-drivers mailing list. Too many people are still using the old gps_common messages because they want additional satellite information.
(Mike) This is more implementation than API, but we found a performance improvement when we experimented with a blocking serial lines generator based on select(). You can try this in our fork here (for fuerte, unfortunately).
(Eric) Thanks for the heads up. Cedric also mentioned that issue in a previous conversation and I plan on making a fix for it sometime after the Hydro release. Since it isn't an API change and simply makes the driver more efficient, it can go in anytime. I have ticketed it at https://github.com/ros-drivers/nmea_gps_driver/issues/3 so that I don't forget
(Jack) The source link on the ROS wiki page still points to kforge.ros.org.
(Eric) I think that will update once the Hydro docs are actually generated, at least for Hydro. I've added an action to move the Fuerte/Groovy link to point to GitHub as well, since I intend on deprecating the kforge project soon.
(Jack) At least for GPS messages, the header.frame_id should be documented similarly to sensor_msgs/NavSatFix:
# header.frame_id is the frame of reference reported by the satellite # receiver, usually the location of the antenna. This is a # Euclidean frame relative to the vehicle, not a reference # ellipsoid.
(Eric) Do you mean in the documentation for the frame_id parameter of the driver or in the NMEA sentence message? Technically, any frame_id in the NMEA sentence is ignored right now in favor of whatever you specify as a parameter to the driver.
(Jack) If it's ignored, that should be documented instead. Some robots have multiple GPS antennae. I was thinking the frame_id could be useful in that case.
(Eric) I forgot about that. I like reading the frame ID from the NMEA message instead of having a frame_id parameter for the nmea_topic_driver. Unless there are any objections, I'll make that change, moving the frame_id parameter to the nmea_topic_serial_reader.
(Jack) +1, seems better to me.
Meeting agenda
To be filled out by proposer based on comments gathered during API review period
Conclusion
Stack status change mark change manifest)
![/!\](./rostheme/img/alert.png "/!\")
Remove .py extension from new node file names - Move Fuerte/Groovy pointers to GitHub
- Add nmea_msgs package with exiting nmea_gps_driver/NMEASentence message as Sentence (matches Mike's message).
- Update to include detailed comment regarding interpretation of header.frame_id (same as sensor_msgs/NavSatFix).
- Include comment language that the this new message only supports NMEA0183 messages which only supports ASCII messages.
- Move frame_id parameter to serial reading nodes and change topic driver node to use the frame_id available from the nmea_msgs/Sentence message.
- Move all new nodes to nmea_navsat_driver package. nmea_gps_driver will be updated and continue to contain the existing nmea_gps_driver.py node, but all of the new nodes (nmea_serial_driver, nmea_topic_driver and nmea_topic_serial_reader will be moved to the nmea_navsat_driver package.