API review
Proposer: Bhaskara Marthi
Present at review:
- List reviewers
Question / concerns / comments
This is a review for the roslisp package. Please review the api as described in the overview. Please also leave your email address to be part of any following discussion.
Stu
The C++ implementation has moved onto using multiple NodeHandles in a process (instead of one node per process). My preference is that roslisp use this model too.
[Bhaskara] The main use for multiple NodeHandles in roscpp seems to be to put parts of the code into a namespace. Roslisp doesn't explicitly use node handles, but as suggested below, we could have a special variable (and a helper macro) to deal with this. In the example below, the first call to do-stuff would happen in namespace foo, and the second to do-more-stuff would happen in foo/bar
(in-namespace "foo"
(do-stuff)
(in-namespace "bar"
(do-more-stuff)))... this is now ticketed at https://code.ros.org/trac/ros/ticket/2897
- "Every topic gets its own thread": Why? This is different from both roscpp and rospy.
[Bhaskara] Because that was simplest to implement
I'm inclined to put off changing this until we see cases where it's a performance issue.
- Is there a way to unregister subscriptions?
[Bhaskara] No; I'll add that. https://code.ros.org/trac/ros/ticket/2898
- What data types are used for array parameters and for hash parameters?
[Bhaskara] Lists and s-xml-rpc:xml-rpc-struct respectively. I added documentation for this on the overview page.
N. Dantam
- Agree that multiple nodes per process would be good. I think most people tend to do lisp development using a single long-running image. In going through the ROSLisp docs previously, there seemed to be some assumption that the user would be starting and stopping multiple short-lived lisp images from the shell; personally, I never do this. It would be much more convenient to put all ROS nodes in a single image you can access through one SLIME session.
- [Bhaskara] Having multiple ros nodes (rather than just node handles of the same node) would be a fairly major change, so probably shouldn't be added to C-Turtle at this stage. But I'd be open to adding it to Diamondback or E. Do you have any suggestions for what the syntax would be?
- [ntd] If you allow multiple node handles, then each handle could have a reference to the node it belongs to. START-ROS-NODE could return a node value and maybe create a default handle. You could have a (CREATE-NODE-HANDLE NODE) function to make more handles. Keeping the current node handle in a special variable is probably easiest. Note that in SBCL, special variables are thread local.
I created a ticket to keep track of this at https://code.ros.org/trac/ros/ticket/2896
- [ntd] If you allow multiple node handles, then each handle could have a reference to the node it belongs to. START-ROS-NODE could return a node value and maybe create a default handle. You could have a (CREATE-NODE-HANDLE NODE) function to make more handles. Keeping the current node handle in a special variable is probably easiest. Note that in SBCL, special variables are thread local.
- [Bhaskara] Having multiple ros nodes (rather than just node handles of the same node) would be a fairly major change, so probably shouldn't be added to C-Turtle at this stage. But I'd be open to adding it to Diamondback or E. Do you have any suggestions for what the syntax would be?
- Could use a special variable for the node handle to avoid having to deal with an explicit parameter.
- [Bhaskara] Yes, that might be good. See above.
- For calling services, it might be nice to be able to pass in a continuation to be invoked when the remote call returns.
- [Bhaskara] Could you give an example?
- [ntd]
- [Bhaskara] Could you give an example?
(call-service-with-continuation (lambda (x) (foo x))
service-handle :arg1 42 :arg2 24)- ... [ntd] This would return immediately, and the ROS runtime would call the continuation when the RPC call returns.
- [Bhaskara] I'm inclined to hold off on this, because we're moving towards using actions rather than services, especially when the call is likely to take any significant amount of time.
- How is WITH-FIELDS different from WITH-SLOTS?
- Why use MAKE-MSG which takes a string for message type instead of MAKE-INSTANCE which takes a symbol?
- Suggest that you do not reinvent parts of the language unless absolutely necessary.
[Bhaskara] So initially all there was was make-instance and slot accessors and with-slots rather than make-msg and with-fields. But I found myself having to do things like
(setq m
(make-instance '<geometry_msgs-msg:PoseStamped>
:pose (make-instance '<geometry_msgs-msg:Pose>
:orientation (make-instance '<geometry_msgs-msg:Quaternion> :w 1))))... just to send a navigation goal, and similarly for extracting fields from messages. make-msg was to be able to replace the above by:
(setq m (make-msg "geometry_msgs/PoseStamped" (:w :orientation :pose) 1))
... Given that ros messages tend to have a lot of nesting, constructs to deal with that seem desirable. Having a string argument was just a convenience thing, but I could make it accept a symbol as well, in which case it would be a strict extension of make-instance. Thoughts?
- [ntd] Ah, I see what you've done now. MAKE-MSG would feel more lispy if it took a symbol argument, for whatever that's worth.
[Bhaskara] Sure, I can add that. https://code.ros.org/trac/ros/ticket/2899.
- [ntd] Ah, I see what you've done now. MAKE-MSG would feel more lispy if it took a symbol argument, for whatever that's worth.
Meeting agenda
To be filled out by proposer based on comments gathered during API review period
Conclusion
Package status change mark change manifest)
![/!\](./rostheme/img/alert.png "/!\")
Action items that need to be taken. 
Major issues that need to be resolved