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

New message definition for the ToolData #272

Closed
wants to merge 4 commits into from
Closed

New message definition for the ToolData #272

wants to merge 4 commits into from

Conversation

NikolasE
Copy link

This PR contains a new message definition for the ToolData that contains the analog inputs from the tool which where not available so far. The message contains numerical constants so that the user can understand the flags and modes easier.

int8 analog_input_range3 # one of ANALOG_INPUT_RANGE_*
float64 analog_input2
float64 analog_input3
float32 tool_voltage_48V
Copy link
Member

Choose a reason for hiding this comment

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

ROS naming conventions: the V should be lower case.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@NikolasE
Copy link
Author

NikolasE commented Dec 8, 2016

@gavanderhoorn: Could you approve the PR? I could then open a PR to integrate the related parsing code into the ur_modern_driver

uint8 TOOL_RUNNING_MODE = 253
uint8 TOOL_IDLE_MODE = 255

uint8 tool_mode # one of TOOL_BOOTLOADER_*
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment should read: # one of TOOL_*?

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@gavanderhoorn
Copy link
Member

A comment on this PR: thinking about it a bit, I'm actually not really a fan of adding these kind of messages. I'm rather a fan of having ROS drivers publish messages that are interpretable on their own, instead of needing another bit of information to be able to understand their contents.

In this case, I'd rather the ur_driver / ur_modern_driver publish values for the analogue inputs / outputs that are already converted (using the data in the ToolData packet in the binary stream coming from the controller). That way we don't shift the responsibility of doing that to consumers of the data.

I'll still merge it, but I think we should take a good look at how we want to handle these kind of cases in the future.

@gavanderhoorn
Copy link
Member

@ThomasTimm: fyi.

@gavanderhoorn
Copy link
Member

@NikolasE: I've had to update your indigo-devel branch to merge in some commits from #275.

If you have the time (and opportunity), it would be nice if we could squash all your commits into one, and do a rebase on top of current indigo-devel. That would avoid the merge commit as well.

@gavanderhoorn
Copy link
Member

@NikolasE: ignore my last comment.

I've squashed your commits and merged them in b26b61d.

Thanks for 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