★ wanayoo — archive 1999 https://github.com/AutonomyLab/create_autonomy/pull/37Nouvelle recherche | Portail wanayoo
Skip to content
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

Merged
merged 1 commit into from Apr 9, 2017

Conversation

@clydemcqueen
Copy link
Contributor

clydemcqueen commented Mar 27, 2017

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.

Copy link
Member

jacobperron left a comment

Thanks for the PR!

@@ -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()));

This comment has been minimized.

@jacobperron

jacobperron Mar 27, 2017 Member

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);

This comment has been minimized.

@clydemcqueen

clydemcqueen Mar 27, 2017 Author Contributor

Done. It would be nice to add more error checking to libcreate create::Create::defineSong. Perhaps that is a 2nd PR.


void CreateDriver::playSongCallback(const ca_msgs::PlaySongConstPtr& msg)
{
robot_->playSong(msg->song);

This comment has been minimized.

@jacobperron

jacobperron Mar 27, 2017 Member

Same here with the bool value returned.

This comment has been minimized.

@clydemcqueen

clydemcqueen Mar 27, 2017 Author Contributor

Done.

@@ -11,6 +11,8 @@ add_message_files(
Bumper.msg
ChargingState.msg
Mode.msg
PlaySong.msg
DefineSong.msg

This comment has been minimized.

@jacobperron

jacobperron Mar 27, 2017 Member

Alphabetize this list.

This comment has been minimized.

@clydemcqueen

clydemcqueen Mar 27, 2017 Author Contributor

Done.

@@ -0,0 +1,5 @@
Header header # ignored

This comment has been minimized.

@jacobperron

jacobperron Mar 27, 2017 Member

Remove the header field if it's not useful.

This comment has been minimized.

@clydemcqueen

clydemcqueen Mar 27, 2017 Author Contributor

Done.

@@ -0,0 +1,2 @@
Header header # ignored

This comment has been minimized.

@jacobperron

jacobperron Mar 27, 2017 Member

Similarly, remove this header too.

This comment has been minimized.

@clydemcqueen

clydemcqueen Mar 27, 2017 Author Contributor

Done.

@jacobperron
Copy link
Member

jacobperron commented Mar 27, 2017

Looks great 👍

Finally, could you squash the latest commits and update the README (adding the new topics to the subscribers section). Cheers!

@clydemcqueen
Copy link
Contributor Author

clydemcqueen commented Mar 28, 2017

OK! README updated and commits squashed. Thanks.

@jacobperron jacobperron merged commit 6aea09b into AutonomyLab:indigo-devel Apr 9, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.