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

📈 🎉 Pub joint temps #1

Merged
merged 5 commits into from
Oct 5, 2022
Merged

📈 🎉 Pub joint temps #1

merged 5 commits into from
Oct 5, 2022

Conversation

cgeering
Copy link
Collaborator

@cgeering cgeering commented Oct 5, 2022

Add joint temperature publishing capabilities to the ur_hardware_interface node.

Includes a new package -- ur_rtde_msgs -- for the joint temperatures message definition. Can move this into ChefAutonomy instead, but leaving in this repo for now.

Based off of these:

Testing:

  • Build without failure
  • Stack launch without failure
  • Joint temps are published to ur_hardware_interface/joint_temperatures

Copy link

@luisrayas3 luisrayas3 left a comment

Choose a reason for hiding this comment

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

I think the main bit of testing I kinda want to see, but think we can skip for this PR, is checking the effect on the loop timing. Hopefully we don't do too much patching but if we do we may need to invest a bit of time on validating this

@@ -0,0 +1,20 @@
<?xml version="1.0"?>
<package format="2">
<name>ur_rtde_msgs</name>

Choose a reason for hiding this comment

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

Theoretically I think we'd want these messages in ur_msgs (it has some similar messages). Given that that's yet another repo which we don't even currently track as a submodule I don't think it would make sense to actually fork it but can we call this package ur_extra_msgs (or supplement, plus, more) and reference that these are our own additions to ur_msgs?

Copy link
Collaborator Author

@cgeering cgeering Oct 5, 2022

Choose a reason for hiding this comment

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

Yeah, I did consider putting this in ur_msgs, but decided not to for the exact reason you laid out, as that would be yet another submodule to fork and track. Very open to moving this package and making it clearer that these are our own.

Are you suggesting we make a ur_extra_msgs (or whatever we call it) in ChefAutonomy or leave the package here in driver repo and just update the name, or something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed ur_rtde_msgs -> ur_extra_msgs.

ur_rtde_msgs/msg/JointTemperatures.msg Outdated Show resolved Hide resolved
@cgeering
Copy link
Collaborator Author

cgeering commented Oct 5, 2022

I think the main bit of testing I kinda want to see, but think we can skip for this PR, is checking the effect on the loop timing. Hopefully we don't do too much patching but if we do we may need to invest a bit of time on validating this

Good point about the loop timing and agree that we should monitor this going forward.

@cgeering cgeering merged commit 539a659 into chef/master Oct 5, 2022
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