Feature/dynamic model for mock interface by pvonwirth · Pull Request #14 · Duatic/duatic_ros2control

pvonwirth · 2026-06-17T11:41:52Z

Task: https://app.clickup.com/t/86ca8y8tj

Depending on whether you like what you see or not, we may wanna have some discussion about Coding Style, Naming, Dos, Don'ts, what so ever ...

…celeration integration

firesurfer · 2026-06-17T12:40:44Z

 {
  using kinematics_translator = kinematics::KinematicsTranslator<kinematics_mapping>;

+  static constexpr bool IS_MOCK_ = is_dua_drive_interface_mock_v<DriveTypeT>;


please do not use upper case letters as this will create confusion with macros.

Also (my style) I would move it into the public part of the class and remove the trailing _

firesurfer · 2026-06-17T12:41:33Z

    // In case there are predefined positions for mock operation we enforce them
    // This is a pure compile time statement
-    if constexpr (std::is_same_v<DriveTypeT, DuaDriveInterfaceMock>) {
+    if constexpr (IS_MOCK_) {


good idea to replace it like that

firesurfer · 2026-06-17T12:43:01Z

+namespace duatic::duadrive_interface::dynamic_model
+{
+
+void DynamicModelNon::on_init(


I dont like the name. It is rather undescriptive.

Also I think for a single function I would not add a cpp file and keep it simply in the header

other suggestions?

The DynamicModelNon is bascially just for the standard mock case.

I would just call it like that ?
DynamicModelMock.

Please also see my argument below how I would actually setup the whole structure.

firesurfer · 2026-06-17T12:57:10Z

+namespace duatic::duadrive_interface::dynamic_model
+{
+
+void DynamicModelPinocchio::on_init(const hardware_interface::HardwareComponentInterfaceParams& system_info)


You do not need to resemble in your architecture the way a ros2 hardware interface component is built.
The reason the DuaDriveInterfaces are built like this is because they actually need to resemble the same lifestyle model.

But for this dynamic pinocchio model I don't think this is needed (it also creates quite a bit of overhead)

Also I think you could directly pass the urdf model which is created here:

duatic_ros2control/duatic_duadrive_interface/include/duatic_duadrive_interface/coupled_kinematics_hardware_interface.hpp

Line 160 in 261a3f4

urdf::Model urdf_model;

Pass-by-reference, i.e. no overhead.
(Unlike the logger, which is copied all the way down to the base class)
The idea here was to just pass the entire description because we have and may one day wanna use other information inside ...

But I change it to pass the urdf

Do as you find it fitting. My argument about overhead was about creating the urdf::Model twice btw.

Da hast du recht, das wird zweimal erzeugt.
Ist geändert.

firesurfer · 2026-06-17T12:58:46Z

+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "duatic_duadrive_interface/dynamic_model/dynamic_model_pinocchio.hpp"


In general just as a suggestion: I often avoid creating for every header file a cpp file especially if there is just like one function in there
Reason is: a) amount of files to manage b) in this case one could also argue performance

@firesurfer Compile-time performance will be better with dedicated cpp files.
If you think about run-time performance, we can link with lto enabled (Link-time-Optimization) to allow function inlining across compilation units.
However, from what I see in the rest of the code, optimizing a single, non-conditional function call is absolutely not the level of optimization that is currently required or matters with respect to optimization potentials throughout the rest of the code.
However, I'm happy about your opinion.

I think I just come from the other way around. I tend to only add cpp files if I have to. It is more about code organisation overhead.

I can image that LTO might be troublesome if used in a plugin architecture.

In dem Moment wo du Header-Implementierungen schreibst zwingst du den Linker sowieso schon dazu LTO zu machen um den ganzen doppelten Code zu entfernen.
Also ich glaube was LTO angeht kann man dem Compiler durchaus trauen. (Hab ich aber selbst tatsächlich auch noch nie produktiv benutzt)

empty cpp removed

firesurfer · 2026-06-17T13:03:03Z

+void DuaDriveInterfaceMock::stage_mock_acceleration(const double acceleration)
+{
+  mock_acceleration_ = acceleration;
+}


Even though this is rather easy to implement this way I would say it might be better to implement it like this:

During creation of each DuaDriveInterfaceMock instance just pass a function pointer which allows each drive to obtain the necessary information from the dynamic model. I would also consider not just returning the acceleration but just the whole state for that specific actuator in that function. This already introduces the necessary infrastructure code for more complex simulation approaches

@firesurfer
The Problem here is, that the drives themselves have nothing to do with the dynamic model.
Especially, due to the fact that the drives drive coupled kinematics and the model models decoupled kinematics.

By using a function pointer you fully decouple it. Given that you also create a DynamicNonModel (which is basically -> do nothing) I would argue it is even more consistent to actually just have the Drive instances get that information from the Model and have less stuff in the DuaDriveInterfaceMock. This would even make it more consistent with the real hardware one could argue.

firesurfer · 2026-06-30T06:29:42Z

+  /**
+   * @brief Register a mock dynamics model with the mock hardware
+   */
+  void register_mock_dynamics(const std::function<double()>& acceleration_callback)


can you pass this via the ctr ?

firesurfer · 2026-06-30T06:31:39Z


-target_link_libraries(duatic_duadrive_interface ${YAML_CPP_LIBRARIES})
+target_link_libraries(duatic_duadrive_interface ${YAML_CPP_LIBRARIES}
+  pinocchio::pinocchio


move the pinnochio::pinocchio update to the target_link_libraries in line 40

In case we are on older platforms ament_target_dependencies should do the job

firesurfer

Looks good to me. See my small comments of stuff to fix.

Before merging I would suggest we test this out with real hardware just to make sure that we didn't break anything (what I dont expect btw).

Also please test this change against all Duatic hardware components.
Is your plan to ship the DynamicModel as default ? What are the necessary steps in a downstream component to support this ?

…_for_mock_interface

pvonwirth added 7 commits June 15, 2026 15:42

initial compiling structure

db4b06b

acceleration-integration is moving the robot

e91f06d

added previously missed joint coupling before simulating the drive ac…

c330cd1

…celeration integration

enforce freeze mode if enforced by command

ca33de1

format

d495831

format ...again?

a539a53

linting

0284499

pvonwirth requested a review from firesurfer June 17, 2026 11:41

firesurfer reviewed Jun 17, 2026

View reviewed changes

pvonwirth added 5 commits June 22, 2026 14:43

simulation velocity limits introduced and urdf double creation avoided

b2a745b

mock simulation working again

7035c32

dynamic model classes renamed

a324e86

formatting

fd13e6b

linting

3376a5b

pvonwirth requested a review from firesurfer June 23, 2026 15:27

pvonwirth and others added 2 commits June 29, 2026 11:16

merged with main, conflicts resolved

56dad4e

Merge branch 'main' into feature/dynamic_model_for_mock_interface

55bb166

firesurfer reviewed Jun 30, 2026

View reviewed changes

pvonwirth added 5 commits June 30, 2026 13:50

Eigen and Pinocchio and CMake and formatting

ba73df7

and more formatting

f65b668

abort initialization if the model is evaluated incorrect

c146b43

format

2d0f934

merged with main

015d323

pvonwirth and others added 5 commits July 6, 2026 09:54

formatting

1ad977a

merged with main

7cc0e0d

Merge remote-tracking branch 'origin/main' into feature/dynamic_model…

4642942

…_for_mock_interface

formatting

8725606

Merge branch 'main' into feature/dynamic_model_for_mock_interface

f0197c5

Uh oh!

Conversation

pvonwirth commented Jun 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firesurfer Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firesurfer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

firesurfer Jun 22, 2026 •

edited

Loading