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

Clean-up scheduling #71

Merged
merged 18 commits into from
Jun 4, 2020
Merged

Clean-up scheduling #71

merged 18 commits into from
Jun 4, 2020

Conversation

jbmouret
Copy link
Collaborator

See

  • examples/scheduler.cpp
  • robot_dart/scheduler.hpp

@costashatz
Copy link
Member

@jbmouret thanks for this! I added a reset() function so that one can use the scheduler in multiple different loops (not possible without calling the reset(), as we are comparing full times and not every step). I also put the scheduler in the python bindings and small stylistic changes. Let me know if this is okay for you..

@costashatz
Copy link
Member

I think we can merge this, no?

@jbmouret
Copy link
Collaborator Author

I have planned to use this API in the run() method first + remove the current scheduling code for control & graphics.

@costashatz
Copy link
Member

I have planned to use this API in the run() method first + remove the current scheduling code for control & graphics.

OK then! I'll let you finish this.. Thanks!

@jbmouret
Copy link
Collaborator Author

jbmouret commented Jun 2, 2020

Many changes here. A few questions:

  • should we sync by default when we have the graphics activated (this could be the expected behavior in graphics mode)
  • my examples/control_loop.cpp does seem to be able to send commands (and I do not know how to read the state easily)
  • I changes the step_once/step_world (please check)
  • they Python might need small update
  • I propose to set by default the control frequency equals to the physics frequency, and the graphics frequency to 40 (a call will be needed to change this)
  • I am not sure I am doing the right scheduling of gui_data

@jbmouret jbmouret changed the title proposal for a low-level scheduling class Cleaner scheduling Jun 2, 2020
@jbmouret jbmouret changed the title Cleaner scheduling Clean-up scheduling Jun 2, 2020
@costashatz
Copy link
Member

Nice work! This is going to be a nice feature of the library.

  • should we sync by default when we have the graphics activated (this could be the expected behavior in graphics mode)

Yes. I think that this makes sense.

  • (and I do not know how to read the state easily)

What do you mean? You can get the states by:

robot->positions(), robot->velocities() etc. or robot->positions({"dof_name1", "dof_name2"}), etc..

  • step_once/step_world (please check)

There are a few small things to fix. I will fix them tomorrow.

  • Python might need small update

Indeed. I will do it.

  • I propose to set by default the control frequency equals to the physics frequency, and the graphics frequency to 40 (a call will be needed to change this)

This makes sense. I agree.

  • I am not sure I am doing the right scheduling of gui_data

You're not, but I will fix that. It's minor.

@jbmouret
Copy link
Collaborator Author

jbmouret commented Jun 3, 2020

For the state question: I wanted to write a simple control loop in examples/control_loop.cpp but I did not know how to do it (it seems that sending random forces do not work).

@costashatz
Copy link
Member

For the state question: I wanted to write a simple control loop in examples/control_loop.cpp but I did not know how to do it (it seems that sending random forces do not work).

When using set_commands, you should not use step_robots(), only step_world(). I fixed a few small issues that you had in the updates/steps. I will update later the python-related stuff.

@jbmouret
Copy link
Collaborator Author

jbmouret commented Jun 3, 2020

What's left?

@jbmouret jbmouret closed this Jun 3, 2020
@jbmouret jbmouret reopened this Jun 3, 2020
@costashatz
Copy link
Member

I made a few small changes and a bigger one: we do not want to synchronize when using WindowlessGraphics (it is defeating the original purpose of the graphics). I am not sure how to handle this. Should we revert the default behavior back to not being synced? Or maybe adding a check within the constructor of the graphics classes to enable or disable the syncing (they have access to the simu model)? I would vote for the second option. What do you think?

What's left?

We should see how to handle the cameras. Either we make them independent of the gui::Base (and e.g., we make a new Sensor class) or make it follow the same interface as gui::Base. I vote for the first option. It's better to have all sensors inside a different API than the graphics (even for the cameras). What do you think? If you agree, we can put this separation in a different PR.

@jbmouret
Copy link
Collaborator Author

jbmouret commented Jun 4, 2020

You are right. Since the Graphics object stores a pointer to the simulator, maybe the graphics object could decide to be synchronized or not? (call set_sync() on the pointer to the simulation)

For the camera, I think we need an API for sensors: IMU, force-torque sensors, and cameras (at least). I don't know if we want to have a Sensor abstract class (all these sensors are quite different), but they should not "live" in the gui (even if they depend on Magnum too).

@costashatz
Copy link
Member

You are right. Since the Graphics object stores a pointer to the simulator, maybe the graphics object could decide to be synchronized or not? (call set_sync() on the pointer to the simulation)

I did this. In the process, I also simplified the Graphics classes to make them independent of the templates for the basic usages.

For the camera, I think we need an API for sensors: IMU, force-torque sensors, and cameras (at least). I don't know if we want to have a Sensor abstract class (all these sensors are quite different), but they should not "live" in the gui (even if they depend on Magnum too).

Let's handle this in a different PR.

@costashatz costashatz linked an issue Jun 4, 2020 that may be closed by this pull request
@costashatz
Copy link
Member

Merging...

@costashatz costashatz merged commit 3bb42a7 into master Jun 4, 2020
@costashatz costashatz deleted the scheduling branch June 4, 2020 19:58
@costashatz costashatz mentioned this pull request Jun 5, 2020
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.

Scheduling
2 participants