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

Important modifications #116

Open
florent-lamiraux opened this issue Jun 23, 2023 · 6 comments
Open

Important modifications #116

florent-lamiraux opened this issue Jun 23, 2023 · 6 comments

Comments

@florent-lamiraux
Copy link
Member

I open this issue in this package since this is the upstream package,
but the issue concerns the following packages:

  • dynamic-graph,
  • dynamic-graph-python,
  • sot-core,
  • sot-dynamic-pinocchio,
  • dynamic_graph_bridge,
  • roscontrol_sot,
  • sot-universal-robot.

I have made important modifications in these packages that I will submit in several pull requests and I suggest to include these modifications in a version 5 of the software.

Motivation and first modifications

The first motivation came from the observation that initializing a robot in a
ROS environment was difficult since two models of the robot coexist:

  • one in DynamicPinocchio that reads a URDF string from
    robot_description parameter,
  • one in Device that reads some ROS parameter to known which joints are
    actuated via roscontrol.

The two models do not necessarily have the same dimension. For example, Franka
robot have 7 joints controlled via roscontrol and 2 gripper joints controlled
via ros actions.

Following a long discussion among users of the software, and in order
to make initialization more general, I first extracted the integration of the
velocity from the Device and created a special entity called Integrator.

The second motivation came from the observation that on some robots, the
control loop is not regularly called as it should be, but the time since last
call is passed to the controller (https://github.com/stack-of-tasks/sot-core/blob/d49cab80bc8160dcb9da10afdd6a8e1f51e37553/include/sot/core/abstract-sot-external-interface.hh#L59).

The problem was to pass this time to the new integrator. I decided to use the
signal time to do so, converting the period into microseconds. Between two
evaluations of the integrator output, the time is now incremented by a number
much bigger than 1 (as previously).

The modifications described here are merged in the devel branches, but not
yet released. I think we should not release them.

Further modifications

Then I realized that the type for signal time (int) is encoded on 32bits. I naively thought that the size of int had changed to 64bits on modern CPUs. As a
result, everything worked fine for 35 minutes and then the time was overflown.

To fix the overflow issue, I then changed the signal type into long int
(or int64_t). This change modifies most of the API and I think we should switch
to version 5 since the overall modifications is important.

Mainly,

  • I defined a typedef dynamicgraph::size_type as Eigen Index type,
  • I defined a typedef dynamicgraph::int64_t sigtime_t,
  • I changed most occurences of int to size_type,
  • I changed most occurences of unsigned int to std::size_t,
  • I simplified SignalPtr class: it is not anymore possible to plug a
    Signal<A> into a SignalPtr<B> if B is not A (formerly, A could
    derive from B). This feature is not anymore used.
@nim65s
Copy link
Contributor

nim65s commented Jun 23, 2023

Ok, thanks for this detailed explanation.

You are suggesting to bump the major version number of the project in the next release, but at the same time you think we should not make a release. What are we waiting for exactly ?

@florent-lamiraux
Copy link
Member Author

We should not make a release until I make the PR and they are merged. The current devel branch is not good.

@nim65s
Copy link
Contributor

nim65s commented Jun 23, 2023

Ok, there was a conflict in your message between:

I will submit in several pull requests

And

are merged in the devel branches

And I parsed that in the wrong way. Now this is more clear.

Following question: Could we make a non-breaking latest release before we merge the PR you are describing here ?

Also, could you link those breaking PR to this issue, so we can easily track them ?

@florent-lamiraux
Copy link
Member Author

Following question: Could we make a non-breaking latest release before we merge the PR you are describing here ?

I do not think it is necessary. Only

  • 9d8ba7f in dynamic-graph could be useful, but it does not deserve a full release.

@nim65s
Copy link
Contributor

nim65s commented Jun 23, 2023

I need one to workaround some breaks introduced by the latest eigenpy release

@olivier-stasse
Copy link
Member

I would recommend to use uint64_t instead of unsigned long which is more ambiguous.

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

No branches or pull requests

3 participants