-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for defining and playing songs #37
Add support for defining and playing songs #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
ca_driver/src/create_driver.cpp
Outdated
@@ -222,6 +224,16 @@ void CreateDriver::undockCallback(const std_msgs::EmptyConstPtr& msg) | |||
robot_->setMode(create::MODE_FULL); | |||
} | |||
|
|||
void CreateDriver::defineSongCallback(const ca_msgs::DefineSongConstPtr& msg) | |||
{ | |||
robot_->defineSong(msg->song, msg->length, &(msg->notes.front()), &(msg->durations.front())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function returns a bool. At the very least we can check the return type and output an error message on failure. Something like ROS_ERROR_STREAM("[CREATE] Failed to define song " << msg->song << " of length " << msg->length);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It would be nice to add more error checking to libcreate create::Create::defineSong. Perhaps that is a 2nd PR.
ca_driver/src/create_driver.cpp
Outdated
|
||
void CreateDriver::playSongCallback(const ca_msgs::PlaySongConstPtr& msg) | ||
{ | ||
robot_->playSong(msg->song); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with the bool value returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ca_msgs/CMakeLists.txt
Outdated
@@ -11,6 +11,8 @@ add_message_files( | |||
Bumper.msg | |||
ChargingState.msg | |||
Mode.msg | |||
PlaySong.msg | |||
DefineSong.msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetize this list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ca_msgs/msg/DefineSong.msg
Outdated
@@ -0,0 +1,5 @@ | |||
Header header # ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the header field if it's not useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ca_msgs/msg/PlaySong.msg
Outdated
@@ -0,0 +1,2 @@ | |||
Header header # ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, remove this header too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Looks great 👍 Finally, could you squash the latest commits and update the README (adding the new topics to the subscribers section). Cheers! |
OK! README updated and commits squashed. Thanks. |
Add support for defining and playing songs. Tested on a Create 2.
The Create 2 Open Interface Spec is somewhat confusing: it says that you can create "up to four songs," but then suggests that song numbers can be "0 - 4". In my tests the only valid song numbers are 0, 1, 2 and 3.