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

[ur_msgs] Fix domain constants in Analog.msg #8

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

galou
Copy link
Contributor

@galou galou commented Oct 12, 2020

CURRENT and VOLTAGE should be inverted.

Tested with URSoftware 3.8.0.61336.

Signed-off-by: Gaël Écorchard gael.ecorchard@cvut.cz

CURRENT and VOLTAGE should be inverted.

Tested with URSoftware 3.8.0.61336.

Signed-off-by: Gaël Écorchard gael.ecorchard@cvut.cz
@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

Cf. discussion in fmauch/universal_robot#8.

@fmauch
Copy link
Contributor

fmauch commented Oct 12, 2020

Yes, this fixes a mistake I made earlier. I checked the docs again and this seems to be the correct order.

Edit: E.g. see the RTDE Guide for field analog_io_types

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

lgtm

@gavanderhoorn
Copy link
Member

@galou: seems your @cvut.cz address is not recognised by GH.

Would you want to fix this before a merge?

@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

@gavanderhoorn : what do you mean "is not recognised by Github"?

@gavanderhoorn
Copy link
Member

sshot

I would expect to see your picture there, instead we see the default GH icon.

@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

Actually, the guide states {0=current[mA], 1=voltage[V]} for the field analog_io_types. What I did is launching ur_robot_driver as of commit 936ff8d27e and looked at the topic /joint_states which showed a domain value of 0 when the input type was set to Voltage on the pendant. I think a double-check on a real robot is necessary.

@gavanderhoorn
Copy link
Member

looked at the topic /joint_states

io_states I guess?

@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

@gavanderhoorn I added the unrecognized email address to my GH settings.

@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

I actually meant /ur_hardware_interface/io_states.

@gavanderhoorn
Copy link
Member

I won't be able to verify this with real hw for some time.

I do agree it should be verified though.

@galou
Copy link
Contributor Author

galou commented Oct 12, 2020

I'm sorry, I shouldn't do two things at once...

My patch does indeed agree with the doc.

What I saw (last week) on the real HW is what is on the patched version, sorry for the misinformation.

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Approving after checking the Real-Time Data Exchange (RTDE) Guide document.

@gavanderhoorn gavanderhoorn merged commit 3a0e181 into ros-industrial:melodic-devel Oct 12, 2020
@gavanderhoorn
Copy link
Member

Thanks @galou.

This will take some time to reach the repositories, as Melodic was just synced.

@gavanderhoorn
Copy link
Member

It would be really good if we could be vocal about this.

The values in the reported IO states were correct, but the constants in the message def weren't.

However, both consumers as well as producers could be affected by this change, as they could both be using the constants and then run into issues now the values have been switched.

@fmauch
Copy link
Contributor

fmauch commented Oct 13, 2020

It would be really good if we could be vocal about this.

The values in the reported IO states were correct, but the constants in the message def weren't.

However, both consumers as well as producers could be affected by this change, as they could both be using the constants and then run into issues now the values have been switched.

Verbal where? Inside the driver, this gets read as raw uint32_t and the value is copied directly inside the hardware interface.

Do you refer to other ROS nodes as producers and consumers?

@gavanderhoorn
Copy link
Member

Do you refer to other ROS nodes as producers and consumers?

Yes.

@gavanderhoorn
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants