API review

Proposer: Michael Ferguson

Present at review:

• Adam Stambler
• Dariush Forouher
• Tully Foote

This review is to establish the API for the rosserial stack, specifically the rosserial_arduino C/C++ API and rosserial_python node API. Additionally, this entails reviewing the message API of rosserial_msgs. We are not at this time reviewing rosserial_xbee.

Links

Open Tickets (kforge)

Newly proposed, mistyped, or obsolete package. Could not find package "rosserial_msgs" in rosdoc: /var/www/docs.ros.org/en/api/rosserial_msgs/manifest.yaml

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.

• (Mike) Should we handle user assignable baud rates? How to handle this on the PC-side? (I don't like the idea of having a baud parameter on the PC).

  • (Dariush) An atmega can handle up to 500k/1M easily, and there is certainly use for higher baud rates. Regarding the additional parameter - you need to specify the device node, anyway. I don't see how another parameter will hurt.

  • (Damon) +1 There's a lot of specific information already required on the host. Any burden associated with knowing the baud rate on the host is made up for by the added flexibility. For example, with adjustable baud rates, it should be possible to build baud rate negotiation on top of the existing protocol.

  • (Mike) I apparently added a baud parameter to the python node about 4 weeks ago. We'll add the ability to set baud on the Arduino side.

  • (Adam) fixed via this NodeHandle rewrite https://kforge.ros.org/rosserial/trac/ticket/27

• (Dariush) In rosserial_arduino, strings should be stored as signed char.

  • (Mike) I concur, ticketed.

• (Adam) Instead of fx_putc, rosserial should pass a pointer to its output buffer and a number of bytes (For example : write (buffer,size) ). This makes it much easier to handle packetized communication like Xbees or Android ADK. Without an explicit packet being transmitted, the underlying putc implementation needs to buffer the data.

  • (Adam) fixed via nodehandle rewrite https://kforge.ros.org/rosserial/trac/ticket/27

• (Adam) fx_putc , fx_getc, fx_open should not be in the global namespace. They should either be part of a special class or they should have their own namespace. They could also be static members of the NodeHandle class.

  • (Wolfgang) I'd even go a step further, and suggest handing fx_putc and fx_getc either as function ptrs or via an object to the NodeHandle constructor, so that different implementations are easy to use; fx_open might then be removed altogether and would be a responsibility of the code using rosserial_arduino.

  • (Adam) fixed via NodeHandle rewrite https://kforge.ros.org/rosserial/trac/ticket/27

• (Damon) The multiplexing part of the protocol should be defined as a separate protocol from the sync flag and checksum. Other protocols that could take advantage of topic multiplexing may have different requirements.

• (Damon) There should be support for /rosout style logging.

• (Wolfgang) The ROS_CALLBACK macro should be changed as it currently has an imbalance of curly braces, which could be removed by making the message definition explicit. Since it is already unsafe to deserialize concurrently (for each callback there is a single message object), the most user-friendly way would be using a single, global, static variable for the message in all callbacks. This would remove the message object declarations from the global namespace. Implementation would probably either involve placement new or a not so straightforward implementation using a union.

  • (Adam) Agreed. I just implemented a change that should work. I should be pushing this out after the API review. See this : ticket https://kforge.ros.org/rosserial/trac/ticket/25

    • (Dariush) Yes, that looks nice.

• (Tully) The header files do not conform to our naming conventions they should be a layer lower so you include them by rosserial_arduino/ros.h for example

  • (Mike) However, these aren't being included in normal nodes -- the usual use case is that the entire ros_lib directory is copied into your Arduino Sketchbook -- and in that case, rosserial_arduino would actually cause the build to fail within the Arduino IDE.

• (Tully) How is it envisioned that multiple arduinos connect to one computer? Can a single rosserial_python node connect to multiple serial ports?

  • (Adam) Each arduino has its own rosserial_python node. The rosserial_python node acts like a bridge between the two. It is not meant to multiplex nodes.

• (Dariush) Is there a technical reason why the arduino node doesn't publish on topics until synched? Because synching can take quite a while (see ticket https://kforge.ros.org/rosserial/trac/ticket/16), and there is value in being able to publish early on (especially for logging boot messages). If synching is indeed essential before publishing topics can resume, I would therefore suggest the following: The arduino node forces a resync with the python node during node init. Something like "bool NodeHandle::isConfigured()" is added, so that the user can block the boot, until the node is fully up. This would also have the benefit that the time will be defined more earlier.

  • (Mike) Until you are synced the python node has no idea what to do with incoming messages (so even if you do publish them, they won't go anywhere). I suppose you could create a queue of previous messages, wait until synced and then send them on (although, all of them would have invalid timestamps since the clock on the Arduino would be ~0).

    • (Dariush) Unless the python bridge was synched already before and stays synched during a reset of the arduino. Which can happen quite often: E.g. the watchdog resets the atmega, or it is switched off for a short time. In this situation currently it takes many seconds until the arduino node is resynched. Forcing a sync from the arduino side might bring the resync time down to well under a second.

  • (Mike) On a separate note, adding an accessor method for configured status should be done -- ticketed as #28.

Meeting agenda

TBD

Conclusion

Stack status change mark change manifest)

• Need to fix lack of a null terminator on strings ticket.

• Major issues that need to be resolved