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

Completed the setup for Franka Emika Panda Robot #15

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

Conversation

f4rh4ng
Copy link

@f4rh4ng f4rh4ng commented Mar 5, 2024

No description provided.

Copy link
Contributor

@matteolucchi matteolucchi left a comment

Choose a reason for hiding this comment

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

Hello,

Thank you very much for submitting this merge request! It is great to see people working on robo-gym and it is great to see that you managed to integrate the Panda robot.

The code looks good to me but unfortunately I have no way to test it personally right now.

From my side there are 2 things that need small changes:

  • please take another look at the print statements left behind and remove them
  • please edit the comments that include your name, it is not common practice in open source repositories, each comment and line of code that you added will this merge request is signed with your git account so there is no need to add that to text of the comment

Thank you very much for working on this.
Matteo

# Robot frames
self.reference_frame = rospy.get_param('~reference_frame', 'base')
#print(self.reference_frame)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this was forgotten and can be removed right?

Copy link
Author

Choose a reason for hiding this comment

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

done.

@@ -167,6 +182,14 @@ def get_state(self):

def set_state(self, state_msg):
# Set environment state
if self.real_robot: # Farhang: when using real robot. the velocity message should not suddenly stop publishing.
Copy link
Contributor

Choose a reason for hiding this comment

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

When merged the commit will have you as author, so I would not add the your name to the comment :)

Copy link
Author

Choose a reason for hiding this comment

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

sure thing! It was a reminder for myself to know which lines I was working on. Then just forgot to remove them.

@@ -244,11 +246,14 @@ def set_state(self, state_msg):
rospy.set_param(param, state_msg.float_params[param])

# UR Joints Positions
print(state_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Print statement forgotten?

Copy link
Author

Choose a reason for hiding this comment

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

yup

@f4rh4ng
Copy link
Author

f4rh4ng commented Mar 7, 2024

Hi Matteo,

Thanks for the quick respond. I enjoyed a fair amount of time last year working on robo-gym as a part of my thesis project; However as I was not much familiar with how Github works, it took me some time to commit it here. Thank you for developing this toolkit. 🙂

I adjusted the code according to your suggestion, sorry for messing up parts of your code. I probably was testing some stuffs back then and just forgot deleting the extra prints and comments. Also I was not sure if I would get a respond for my pull request as there was no new commits in some years, so I thought I first just send a request to see how it goes. 😁

One more thing. I also have managed to integrate the Panda robot in robo-gym side and got some interesting results. But as I borrowed some codes from some other repositories like D4PG (https://github.com/schatty/oprl/tree/legacy_d4pg), I wasn't sure if making a pull request make sense. But I made a fork which you can find in following link:

https://github.com/f4rh4ng/robo-gym-panda

Please let me know if you would like to merge this too, and if yes which parts would make more sense for merging. 🙂

Best
Farhang

@matteolucchi
Copy link
Contributor

Hello Farhang,

Sorry for the late reply.

I am glad to hear that our toolkit was useful too you :) No worries at all, it happens to everyone. Everything looks good to me so far and I think it is ready to merge. Unfortunately I do not have the rights to merge pull requests anymore since I parted ways with Joanneum Research, but I hope someone will take a look at it soon and approve it. Maybe @jr-b-reiterer ?

Your repository look very interesting, and I think it could be interesting to merge the environment part into the main repository, but before you put any more work into that I would wait and see if we get a reply from someone at JR.

I hope this gets merged soon, but in any case people will still be able to see your pull request and be able to see your addition.

All the best,

Matteo

@jr-b-reiterer
Copy link

Hello @f4rh4ng,

thanks a lot for your efforts!
As the community surely has guessed in the meantime, since the departure of the original robo-gym team including @matteolucchi, resources to work on robo-gym have been scarce. Extra thanks to him for still taking the time to occasionally answer questions here.

We are currently setting up a project in which we are going to reactivate our own work on robo-gym that is supposed to start within Q2 of 2024. Not set in stone yet, but considered preparatory steps include a set of significant refactorings that are expected to make development and usage more sustainable in the long run. But this may lead to the creation of a set of successor repositories in order not to break setups based on existing versions that can't handle breaking changes from upstream.

Of course we will still have to manage our resources carefully, and as the training and testing with a real robot in this context is non-trivial, I cannot say yet at which stage the Panda will fit with our priorities. But I am sure we will be able to integrate your developments at some point not too far away, also regarding your work in your fork of the robo-gym repo. Possible that it's not going to be by accepting the merge request, but by a more selective process (acknowledging contributions where due). Your d4pg examples might be useful for others, too, but as you indicated yourself, license and/or attribution issues might need to be solved. As you surely have guessed from the pre-existence of Panda-related code in the robot servers repository, there were previous activities in that direction at our place that have not made it to the github repos yet. When we get to the stage in which we want to go through with integrating the Panda, we will also need to compare this with your suggested changes and find the best of both.

Thanks again and stay tuned!

BR,
BR

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.

None yet

3 participants