jiminy
jiminy copied to clipboard
duburcqa
WIP: [core] Add transmission abstraction.
start modifying AbstractMotor and BasicMotors, add initial files for transmission
https://github.com/duburcqa/jiminy/pull/432#discussion_r745625397
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..
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
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.
Do we still need Options for the transmission ? To set for example to mechanical reduction ratio in case of a simple transmission ?
Yes, not to mention custom, more advanced transmissions for which is it hard to predict what the parameters could be. Why not some backlash parameters for instance.
Ok, I'm trying to understand it using the AbstractMotor, and I'm wonderig why do we have baseMotorOptions_ and motorOptionsHolder_
one is Structure with the parameters of the motor and public and the other Dictionary with the parameters of the motor protected
one is Structure with the parameters of the motor and public and the other Dictionary with the parameters of the motor protected
Dictionary are convenient but manipulating them is painfully slow because they are weakly typed (like python), so you need to pay the cost to infer the type at runtime. On the contrary, structure are strongly typed and thus very fast but not generic. By giving access to the users to the structure, one can make sure the end-user is accessing the options through the right API.
In the meantime, the user is expected to get/set the options using a dictionary for convenience. (Nobody expects options to be strongly typed for the getter and setter in my opinion). The dictionary provided by the user is stored internally as a dict and returned as const by the getter. This way, it avoids having to implement a method to convert structure into dictionary for this, only the other way around is implemented.
To sum up. Dict is used for getter and setter, since taking time for this is not a big deal. It is only done seldomly. Then user is expected to access the values at runtime using the structure, which is crazy fast. It is the only way to get convenient setter/getter interface, while runtime access as fast as possible. It is true that storing the dict could be avoided by implementing a converter from struct to dict, but I think it is better to avoid it by storing the dict.
https://github.com/duburcqa/jiminy/pull/432#discussion_r770577643
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 */
https://github.com/duburcqa/jiminy/pull/432#discussion_r770589573
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>()))
https://github.com/duburcqa/jiminy/pull/432#discussion_r770581950
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
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.
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> >()))
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.
/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)
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.
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.
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.
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.
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.
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.