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

adapt to calculator #24

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

ipa-rwu
Copy link

@ipa-rwu ipa-rwu commented Nov 23, 2020

I created a calculator ros package. But I need to modify a little bit of this package.
It seems that this package is still functional. But please check it again.

@@ -40,6 +37,9 @@ def _run(self):
self._rate.sleep()

def start(self):
self._pub_diag = rospy.Publisher(
'/diagnostics', DiagnosticArray, queue_size=10
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't you be using output_topic_name and msg_type here?

Copy link
Author

Choose a reason for hiding this comment

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

I rewrite this function in my code.

Copy link
Owner

Choose a reason for hiding this comment

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

self._pub_diag = rospy.Publisher(
            '/diagnostics', DiagnosticArray, queue_size=10

Should be

self._pub_diag = rospy.Publisher(
            output_topic_name, msg_type, queue_size=10

isn't it?

@hsd-dev
Copy link
Owner

hsd-dev commented Nov 24, 2020

Have you made any changes in monitor.py? It looks similar to the existing monitor script.

@ipa-rwu
Copy link
Author

ipa-rwu commented Nov 24, 2020

I didn't change any in monitor.py. I just rewrite some functions in my code.

@hsd-dev
Copy link
Owner

hsd-dev commented Nov 24, 2020

I didn't change any in monitor.py

Then can you remove monitor.py from the PR?

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