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

Refactor code into a Python module #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nickswalker
Copy link

This PR will let people import the RespeakerInterface and RespeakerAudio classes in code they write inside other packages. This is accomplished by using catkin's support for installing a package's Python modules.

Move classes into their own files in a module
Adopt standard catkin support for the module so that other packages can use the wrappers directly
@furushchev
Copy link
Owner

@nickswalker Thank you for sending PR! I still don't try your patch, but isn't this PR deleting executable from the package, which means the node is unable to be executed from launch file or with rosrun call?


import rospy
from respeaker_ros import RespeakerNode

if __name__ == '__main__':
rospy.init_node("respeaker_node")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@furushchev Most of the code in this file was moved into node.py inside the module (to let people import and customize the node in their own packages). Because the diff was large GitHub collapsed it making it look like the file was removed completely, but it's still here, just very small now.

nickswalker added a commit to hcrlab/respeaker_ros that referenced this pull request Aug 29, 2023
Matches the API of furushchev/respeaker_ros#19, but bumped to Python3 and
with less string-y handling of audio data
Drops dynamic reconfigure, as params are dynamic by default in ROS 2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants