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

WIP: [core] Add transmission abstraction. #432

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

fabinsch
Copy link

start modifying AbstractMotor and BasicMotors, add initial files for transmission

@fabinsch fabinsch changed the title init commit transmission [Draft] init commit transmission Oct 29, 2021
@fabinsch fabinsch force-pushed the integrate_transmission branch 3 times, most recently from 7117ab5 to 614e7cc Compare October 29, 2021 14:39
@fabinsch
Copy link
Author

Todo:

  • energy dissipation in transmission
  • transmission constraints

@fabinsch fabinsch force-pushed the integrate_transmission branch from 614e7cc to de6a9b5 Compare October 29, 2021 15:53
@fabinsch fabinsch marked this pull request as draft October 29, 2021 15:57
@fabinsch fabinsch force-pushed the integrate_transmission branch from de6a9b5 to 3047673 Compare October 29, 2021 16:18
@duburcqa duburcqa changed the title [Draft] init commit transmission [Draft] Add transmission abstraction. Oct 29, 2021
@duburcqa duburcqa changed the title [Draft] Add transmission abstraction. WIP: [core] Add transmission abstraction. Oct 29, 2021
@duburcqa duburcqa linked an issue Oct 29, 2021 that may be closed by this pull request
@fabinsch fabinsch force-pushed the integrate_transmission branch 2 times, most recently from a9a2c74 to 225724e Compare November 8, 2021 17:24
@fabinsch fabinsch force-pushed the integrate_transmission branch from 225724e to 52aae4e Compare November 8, 2021 17:30
core/include/jiminy/core/robot/AbstractMotor.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/AbstractMotor.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/AbstractMotor.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/AbstractMotor.h Outdated Show resolved Hide resolved
bool_t isAttached_; ///< Flag to determine whether the motor is attached to a robot
std::weak_ptr<Robot const> robot_; ///< Robot for which the command and internal dynamics
std::function<hresult_t(AbstractMotorBase &)> notifyRobot_; ///< Notify the robot that the configuration of the sensors have changed
std::function<hresult_t(AbstractMotorBase &)> notifyRobot_; ///< Notify the robot that the configuration of the motors have changed
Copy link
Owner

Choose a reason for hiding this comment

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

same.

core/src/robot/BasicTransmissions.cc Outdated Show resolved Hide resolved
core/src/robot/BasicTransmissions.cc Outdated Show resolved Hide resolved
core/src/robot/Robot.cc Outdated Show resolved Hide resolved
core/src/robot/Robot.cc Outdated Show resolved Hide resolved
core/src/robot/Robot.cc Outdated Show resolved Hide resolved
@fabinsch fabinsch force-pushed the integrate_transmission branch from ed03d33 to 500b569 Compare November 9, 2021 13:22
@fabinsch
Copy link
Author

fabinsch commented Dec 7, 2021

#432 (comment)

auto robot = robot_.lock();
        for (std::string const & jointName : jointNames_)
        {
            std::vector<int32_t> jointPositionIdx;
            if (!robot->model.existJointName(jointName))

I fixed the way how to access the robot, but it does not have a member model. I'm a bit lost how to get access to the model at this point..

@duburcqa
Copy link
Owner

duburcqa commented Dec 7, 2021

it is pncModel_ not model. Similar to python.

@fabinsch
Copy link
Author

fabinsch commented Dec 7, 2021

I still have sharedHolders all over the place in the AbstractTransmission. We agreed that we do not need this for them right ? So I will get rid of them

@duburcqa
Copy link
Owner

duburcqa commented Dec 7, 2021

yes, there is no shared memory for them. since nobody wants to access the state of all the transmissions at once. It is only the case for the motors and sensors, and system state.

core/include/jiminy/core/robot/AbstractTransmission.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/AbstractTransmission.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/BasicMotors.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/BasicTransmissions.h Outdated Show resolved Hide resolved
core/include/jiminy/core/robot/BasicTransmissions.h Outdated Show resolved Hide resolved
core/src/robot/BasicMotors.cc Outdated Show resolved Hide resolved
core/src/robot/BasicTransmissions.cc Outdated Show resolved Hide resolved
core/src/robot/BasicTransmissions.cc Outdated Show resolved Hide resolved
python/jiminy_pywrap/src/Motors.cc Outdated Show resolved Hide resolved
python/jiminy_pywrap/src/Transmissions.cc Show resolved Hide resolved
@fabinsch
Copy link
Author

#432 (comment)

maybe you can explain me quickly what do you mean exactly with this "copy on purpose". I tried to do it in a same way then AbstractMotor.h
virtual hresult_t computeEffort(float64_t command) = 0; /* copy on purpose */

@fabinsch
Copy link
Author

#432 (comment)

why in this case you propose to do
bp::return_value_policy<result_converter<false> >()))

whereas the other getters have
bp::return_value_policy<bp::copy_const_reference>()))

@fabinsch
Copy link
Author

#432 (comment)

why you think we shouold q and v in this case? I thought the simple transmission is just a basic reduction ratio that is state independent

@duburcqa
Copy link
Owner

maybe you can explain me quickly what do you mean exactly with this "copy on purpose".

The point is to write "copy on purpose" when you do a copy on purpose. In your example computeEffort(float64_t command) indeed you do a copy, on purpose by definition because by default you should do computeEffort(float64_t const & command). So the point is to mention when a copy is made on purpose to not remove it afterward during review or whatever thinking it is a mistake.

@duburcqa
Copy link
Owner

You can have a look at the definition of result_converter. It is used to decide how to handle eigen arrays returned by C++ to Python. If it is false, it means by reference, if it is true then it is by copy. For other objects other return policies must be used. For instance bp::copy_const_reference is to copy const reference that are return by C++ to Python (if it is directly convertible, otherwise it would fail.)

bp::return_value_policy<result_converter<false> >()))

@duburcqa
Copy link
Owner

why you think we shouold q and v in this case? I thought the simple transmission is just a basic reduction ratio that is state independent

Yes indeed, but you never comment variable names in declaration. Only in definitions.

@fabinsch
Copy link
Author

/home/fabian.schramm/wdc_workspace/src/jiminy/core/src/engine/EngineMultiRobot.cc: In member function ‘const vectorN_t& jiminy::EngineMultiRobot::computeAcceleration(jiminy::systemHolder_t&, jiminy::syst
emDataHolder_t&, const vectorN_t&, const vectorN_t&, const vectorN_t&, jiminy::forceVector_t&)’:
/home/fabian.schramm/wdc_workspace/src/jiminy/core/src/engine/EngineMultiRobot.cc:3690:14: error: ‘pinocchio::Data’ {aka ‘struct pinocchio::DataTpl<double>’} has no member named ‘rotorInertia’
 3690 |         data.rotorInertia = system.robot->getArmatures();
      |              ^~~~~~~~~~~~

I think this is quite werid as I did not modify something related to pinocchio:Data (that's why I had a comment with wtf before)

@duburcqa
Copy link
Owner

duburcqa commented Dec 16, 2021

It is expected. it should be model not data.

@fabinsch fabinsch force-pushed the integrate_transmission branch from 47b72b9 to 2b5dc0d Compare December 17, 2021 15:47
@duburcqa
Copy link
Owner

I was think, and there is still an issue with the API of the transmission. It is not possible to define a transmission only by its transform matrix (jacobian) because it only applies on spatial motion and force vectors (velocity and upward) but not on the configuration. For this, you should provided another methods, converting back and forth configuration vectors from joint space to motor space. There is no other choice to be generic.

@duburcqa
Copy link
Owner

After doing the math, I realized you should remove the acceleration from motor state, and from transmission forward/backward. It is more tricky to handle acceleration (it requires any additional user specified matrix) and not necessary in practice in 99% of the usecases.

@duburcqa
Copy link
Owner

duburcqa commented Dec 21, 2021

You should also limit yourself to invertible transmissions,. Which are n-to-n motor to joint mappings. And rename the abstract case accordingly. Non-invertible transmission must be modelled through constraints instead.

@duburcqa
Copy link
Owner

It is necessary to access the motors directly at robot level for several reasons, including the calling the *All methods of motors (such as resetAll ), which cannot be done at transmission level since there is no sharedHolder for them. So I suggest still dealing with the motor directly at robot level, instead of doing it through the transmission as we suggested. Similarly, I tend to think it may be better to remove again notifyRobot, and just create and return the list when the getter is called (no longer returning an attribute but a temporary). For me using notifyRobot is too much of a hassle just to provide a convenience getter. Also, you should add a reset method to the transmission, and reset them alongside the motors.

@duburcqa
Copy link
Owner

duburcqa commented Jan 4, 2022

Cassie could be the first robot using an advanced transmission in Jiminy. Currently it is done by solving a optimization problem, which is likely to be more costly. Could be interesting to compare both. Transmission should be unbeatable. Ideally we should also implement joint spring. So we could compare the "exact" model with the simplified one with linearized transmission.

@duburcqa
Copy link
Owner

duburcqa commented Jan 5, 2022

rotorInertia should be a config option of the motors, and the transmission inertia should be accessed by the robot through the transmissions. This is necessary for advanced transmission. Ideally, once simple transmission are implemented, it would be great to add exact rotor inertia modelling instead of relying on the classical approximation by diagonal terms on the mass matrix.

@duburcqa duburcqa force-pushed the master branch 3 times, most recently from bc0276f to 2d753b8 Compare March 11, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Add support of motors to joints transmissions
2 participants