User Test

User Testers:

• Gil Jones
• Brian Gerkey

Question / concerns / comments

Brian - (This is a condensed version of the associated trac ticket #2880

• Would be nice to explain in the tutorials what genaction did. E.g., walk through the resulting msg/*.msg files.

• How often should you check for a pre-emption request? Is it my choice as action-writer?

• Method names are unusually long for C++, e.g., waitForActionServerToStart(), waitForGoaltoFinish(). I don't have concrete suggestions, other than to strive for shorter names.

  • Punting to after common-1.0

• Suggestion: SimpleActionClient::getTerminalState() -> getState(). There doesn't appear to be another getState() that would confuse things.

  • [Vijay] Definitely makes sense. I added in a getState to replace the more confusing getTerminalState

• It's a little annoying that the tutorial code defines class methods before class members that are used in those methods. Code can reasonably be written in either order, but for a tutorial, I think the members should come first; I found myself frequently scrolling to the bottom to find out what type a given member is.

• Any particular reason for the dereference on line 33 of http://wiki/actionlib_tutorials/Tutorials/SimpleActionServer(GoalCallbackMethod) ? Seems like you could just do goal_ = as_.acceptNewGoal()->samples;

• API Docs

  • What does SimpleActionClient::getGoalState() return if I haven't sent a goal? How about if somebody else sent a goal? Is that case (i.e., multiple clients to a single server) handled in general?

  • Suggestion: ClientGoalHandle::isExpired() -> ClientGoalHandle::isValid(). And ClientGoalHandle::reset() -> ClientGoalHandle::invalidate(). Or perhaps rename reset() to expire()?

    • [Vijay] This is now completely hidden from the SimpleActionClient user. I ok with the isExpired() and reset() calls, because it matches the calls and semantics of boost::shared_ptr. I'm definitely up for re-discussing this, once we have users of the base ActionClient, and we're planning on releasing that API.

  • Does the writer of an action client/server need to see a CommState?? Hopefully not, as it appears to be the details of the wire protocol, which the client and server machinery should take care of. Can the CommState? be hidden from the user?

    • [Vijay] This is now completely hidden from the user

Gil - (This is a condensed version of the associated Trac ticket #2982)

• (This step will hopefully become more streamlined if when integrate this step into the CMakeLists.txt file) -- this sentence makes no sense and is probably unneccessary.

• SimpleActionServer - ExecuteCallbackMethod

  • Writing a simple server

    • This includes <the> action message generated...

    • In the process of a running action...

      • This paragraph would make more sense as something like "An important component of an action server is the ability to allow an action client to request that the current goal execution be cancelled. When a client requests that the current goal be preempted the action server should cancel the goal, perform necessary clean-up, and call the function setPreempted(), which signals that the action has been preempted by user request. Setting the rate at which the action server checks for preemption requests is left to the implementor of the server."

    • The comment "//this sleep is not necessary" is not clear. I think it would be worth mentioning that the sleep in combination with the wait serves to limit the for loop to outputting one number a second rather than outputting the numbers as fast as possible. I think what you mean by the comment is that the action will function even if the sleep isn't there, but it is necessary if you want 1 hertz output for the action.

  • Compiling and Running the Action

    • This tutorial assumes that we've already created the package, and in the package creation instructions we were told to uncomment rosbuild_genmsg(), so this second call isn't necessary.

    • changed to rxgraph -t Certainly the rxgraph and maybe the other calls should go after running the stuff. It's a little disconcerting when nothing happens if you go in the order of the page.

• SimpleActionServer(GoalCallbackMethod)

  • The goalCB function is split into two parts in the code fragments - get rid of the destructor lines and just have the goalCB function, or if it's important to split into two parts get rid of the destructor lines.

  • I think it would be useful to mention the advantages/disadvantages of automatically spinning a thread versus creating the thread yourself. Just a sentence would be fine.

  • The rosbuild line for the client should be:

    • rosbuild_add_executable(averaging_client src/averaging_client.cpp)

  • Start the Client and Server - typo in 'rosrun learning_actionlib averaging_se<r>ver'

  • this can result in aborted or succeeded so either is fine The output line shows a status of ABORTED - is that what you want?

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

---