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

chore(RPS-1004): updated to new wandelbots-nova library #3

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

biering
Copy link

@biering biering commented Dec 19, 2024

  • updated to new wandelbots-nova lib
  • updated notebook
  • added yamllint
  • using ruff

@biering biering requested a review from bompo December 19, 2024 12:00
@biering biering self-assigned this Dec 19, 2024
Copy link
Member

@bompo bompo left a comment

Choose a reason for hiding this comment

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

I would at least clean the ipynb, the rest of the comments are more api related

@@ -1,5 +1,5 @@
# provide the host where the nova api is running (e.g. your virtual dev instance)
WANDELAPI_BASE_URL="{{ .InstanceInformation.NovaApiHost }}"
Copy link
Member

Choose a reason for hiding this comment

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

It's still called like that in the typescript lib, for consistency we would have to change it there as well

Copy link
Author

Choose a reason for hiding this comment

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

Ok, we now have made it all like

  • NOVA_HOST
  • NOVA_PASSWORD
  • NOVA_USERNAME
  • NOVA_ACCESS_TOKEN

Would you change it in the ts-lib?

Copy link
Member

Choose a reason for hiding this comment

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

Ok

"id": "b74681ee",
"metadata": {},
"outputs": [],
Copy link
Member

Choose a reason for hiding this comment

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

I would clean the outputs before committing

"print(my_robot.tcps())\n",
"print([round(j,2) for j in my_robot.current_joints()])\n",
"print(my_robot.current_tcp_pose())"
"tcp_pose = await motion_group.tcp_pose(\"Flange\")\n",
Copy link
Member

Choose a reason for hiding this comment

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

I would fetch the TCP in this example, so it right away works with any attached robot

")"
"nova = Nova()\n",
"cell = nova.cell()\n",
"controller = await cell.controller(\"ur\")\n",
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to fetch a controller, so it works with every attached robot

examples/notebook.ipynb Show resolved Hide resolved
"from nova.core.movement_controller import move_forward\n",
"\n",
"async with controller[0] as motion_group:\n",
" home_joints = (await motion_group.joints(\"Flange\")).joints\n",
Copy link
Member

Choose a reason for hiding this comment

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

why does joints need a TCP?

" jnt(home_joints),\n",
" ]\n",
"\n",
" await motion_group.run(actions, tcp=\"Flange\", movement_controller=move_forward)"
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if the example took the planned motion and executes this directly. One could optimize the behaviour for planning several motions beforehand and executing them later one.

@biering biering changed the title chore: updated to new wandelbots-nova library chore(RPS-1004): updated to new wandelbots-nova library Dec 20, 2024
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.

3 participants