API review

Proposer: Andreas Paepcke

Present at review:

• Tully Foote
• Blaise Gassend
• Melonee Wise

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.

Tully

• in Wiimote msg
  • The button and LED statuss could be bools
  • the battery data should not be an array. (document units and range)
  • why isn't the zeroing time a ros/Time?
  • Document the covariance units/axes.
  • a variable length array of x, y, size for the points might be better.
    • point is not appropriate unlesss it has units of meters
    • the length of the array will be resized to show the number of points tracked.
    • the possible value ranges should be documented
    • the units should be documented
• int arrays are inconsistent. -1 0 1 sometimes mean leave, sometimes mean other etc.
• it seems that the timed switch could be used for turning on and off and leaving in current state
  • [ 1 ] , [0, 1], [] respectively
• documentation note it's sensor_msgs/Imu I believe

Melonee

• LEDControl.msg

  • the switch_array and the timed_switch_array seems redundant, it might be best to just have the timed_switch_array. switch_mode seems to accomplish the same things as switch_array.

• TimedSwitch.msg

  • This seems like a good message

• RumbleControl.msg

  • This seems like a good message

• Wiimote.msg

  • maybe theses should be bools
    • rumble
    • LEDs
  • zeroing_time should be ros/Time
  • make battery: raw_battery and percent_battery
  • document error codes in wiki or in msg as well as in code
  • i'm not sure I really understand what the ir_size does

Andreas Clarification Requests to Tully and Melonee

Tully:

Regarding:

• "a variable length array of x, y, size for the points might be
  • better. "
    • You mean I would separately define something like IR_source_info.msg, as:
      • float64 x float64 y int64 size
      Then, in Wiimote.msg replace ir_positions and ir_sizes with the single field:
      • IR_source_info[] ir_tracking
      Do I have that right?
      • Tully: yes that would be better.

Regarding:

• "int arrays are inconsistent. -1 0 1 sometimes mean leave, sometimes
  • mean other etc."
    • You are referring to the following?:

      • TimedSwitch.msg: switch_mode: -1: run a pattern, as opposed to switching

      • TimedSwitch.msg: num_cycles: -1: run till further notice

      • LEDControl.msg: switch_array: -1: leave in current state
      Would this confusion be avoided if I introduced constants?
      • Tully Constants would be good.

Regarding:

• "it seems that the timed switch could be used for turning on and off and leaving in current state
  • [ 1 ] , [0, 1], [] respectively"
    • Not sure I understand. You are referring to LEDControl.msg : switch_array? Is this concern similar to Melonee's observation that switch_array is redundant?
      • yes, I think that the timed control is simple enough that people could use it by default. Especially if there's a tutorial for doing so.

Melonee:

Regarding LEDControl.msg:

• "the switch_array and the timed_switch_array seems redundant, it
  • might be best to just have the timed_switch_array. switch_mode seems to accomplish the same things as switch_array."
    • I'm happy to do that. Before I change it, let me explain this redundancy and see whether you still feel the same: The reason for the int8[4] (now bool[4]) switch_array is to make it as simple as possible

      to just switch the LEDs on or off. The TimedSwitch in the timed_switch_array requires more thinking for this simple task. If you feel that it's fine for programmers to have to deal with TimedSwitch when they don't need the timing functionality, then I'll change it.

    • My only concern was which takes precedence when commanding? the switch_array or the TimedSwitch?

Regarding Wiimote.msg:

• "zeroing_time should be ros/Time"
  • Is that roslib/Time? When I run rosmsg show ros/Time I fail. When I run rosmsg show roslib/Time I get something reasonable.
  • yes you're right that was a typo on my part.

Thank you both!

Andreas

Blaise

• I fixed the names of the topics (they were currently set equal to the message type).
• It would nice to support the calibrate/is_calibrated API for zeroing the device. Currently you have to restart the node to zero it.
• You sure took the trouble to make a nice API for controlling the LEDs and rumble!
• It might make more sense to have constants in the message rather than in wiimoteConstants.py because if they are in the message they can be accessed from C++ or any of the other languages that ROS works with.
• It might be nicer to have the raw/processed battery values be in separate floats. As it is, it looks like readings from two batteries. I would prefer something like battery_percent and battery_raw.
• It would be preferable to use bool rather than int8 for booleans.
• It would be preferable to use ros/Time rather than float64 for a time.
• Does ir_sizes really not fit in an int32?

Resolution of Tully's and Melonee's Items

All items have been resolved.

Meeting agenda

Plan is to do the review without a meeting. We'll see.

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved