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

New abstract class DQ_CoppeliaSimInterface #112

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

Conversation

juanjqo
Copy link
Member

@juanjqo juanjqo commented Aug 27, 2024

Description of changes

Hi @dqrobotics/developers

This PR aims to discuss strategies to implement a proposes an abstract class DQ_CoppeliaSimInterface. The idea is to have concrete classes with different Coppelia's API. For instance, the Legacy remote API (the one used by DQ Robotics, but currently deprecated), and the ZeroMQ remote API. This feature was internally discussed, and I am working on a prototype based on my own DQ Robotics zmq-based interface.

This PR proposes the following:

  • The abstract class DQ_CoppeliaSimInterface
  • Rename the current class DQ_VrepInterface to DQ_CoppeliaSimLegacyInterface, and now, this class inherits from DQ_CoppeliaSimInterface
  • Create the class DQ_VrepInterface, which inherits from DQ_CoppeliaSimLegacyInterface for backward compatibility.
classdef DQ_VrepInterface < DQ_CoppeliaSimLegacyInterface
    methods
        function obj = DQ_VrepInterface()
            obj@DQ_CoppeliaSimLegacyInterface();
        end
    end
end

I implemented a similar strategy concerning the following classes:

  • DQ_CoppeliaSimRobot()
  • DQ_SerialCoppeliaSimRobot()
  • YouBotCoppeliaSimRobot()
  • LBR4pCoppeliaSimRobot()
drawing

I'm planning to include the methods get_center_of_mass(), get_mass(), and get_inertia_matrix() (as suggested by @ffasilva) in the abstract class DQ_CoppeliaSimInterface() in future PRs since we are still working on them.

Best regards,

Juancho


Is this the best strategy? Please let me know your suggestions.

Alternatively, we could have the same behavior with fewer modifications in this way:

The current DQ_VrepInterface() class inherits form DQ_CoppeliaSimInterface

The DQ_CoppeliaSimLegacyInterface() class inherits from DQ_VrepInterface(). Therefore, no renaming is required.

drawing

PS
@bvadorno mentioned specific Matlab tools for managing aliases. But, I am not sure if this is available for C++/Python.

@ffasilva
Copy link
Member

Hi @juanjqo,

I don't see the problem with your first suggestion. A user trying to use DQ_VrepInterface would still have access to all its functionalities through DQ_CoppeliaSimLegacyInterface without ever knowing any renaming took place.

As a suggestion, I would only modify DQ_VrepInterface to clarify it is now deprecated.

% For backward compatibility only. Do not use this class.
classdef DQ_VrepInterface < DQ_CoppeliaSimLegacyInterface
    methods
        function obj = DQ_VrepInterface()
            warning('Deprecated. Use DQ_CoppeliaSimLegacyInterface instead.')
            obj@DQ_CoppeliaSimLegacyInterface();
        end
    end
end

Additionally, I believe the abstract class DQ_CoppeliaSimInterface should also have the methods get_center_of_mass(), get_mass(), and get_inertia_matrix().

Kind regards,
Frederico

@juanjqo
Copy link
Member Author

juanjqo commented Sep 10, 2024

@ffasilva thanks, I'm going to include the modifications you suggested.

@juanjqo juanjqo marked this pull request as draft September 11, 2024 08:46
@juanjqo juanjqo changed the title Discussion about the new abstract class DQ_CoppeliaSimInterface New abstract class DQ_CoppeliaSimInterface Sep 13, 2024
@juanjqo juanjqo marked this pull request as ready for review September 13, 2024 12:56
@mmmarinho
Copy link
Member

Hi @juanjqo could you please clarify who do you want to review this code and in what order.

@juanjqo
Copy link
Member Author

juanjqo commented Sep 16, 2024

@mmmarinho I don't have any specific preference. May I suggest the following order @ffasilva, @mmmarinho, and @bvadorno?

@bvadorno
Copy link
Member

@mmmarinho I don't have any specific preference. May I suggest the following order @ffasilva, @mmmarinho, and @bvadorno?

Sounds good to me. Please proceed.

Kind regards,
Bruno

Copy link
Member

@ffasilva ffasilva left a comment

Choose a reason for hiding this comment

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

Hi @juanjqo,

Thank you for the pull request. I tested the classes and found no problems with them. I just added two minor comments.

The one regarding the inclusion of the methods get_center_of_mass(), get_mass(), and get_inertia_matrix() is pending on #109. I'll work on this now.

Kind regards,
Frederico


% This method gets the joint torques of a robot in the CoppeliaSim scene.
joint_torques = get_joint_torques(obj,jointnames);
end
Copy link
Member

Choose a reason for hiding this comment

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

Please remember to add the methods get_center_of_mass(), get_mass(), and get_inertia_matrix().

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffasilva, thank you for the reminder.

Note for the future Juancho (since I am going to completely forget this): we discussed this internally. We decided to add those methods in a future PR since they are not available in the master version of the DQ Robotics. We are still working on it at #109

effector = 1 + E_*0.5*0.3*k_;
kin.set_effector(effector);
end
obj@YouBotCoppeliaSimRobot(robot_name, vrep_interface);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a warning message informing that the class is now deprecated:

warning('Deprecated. Use YouBotCoppeliaSimRobot instead.')

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffasilva good catch! I added the warning message.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffasilva I added the modifications you suggested. Please let me know if something else is required.

@ffasilva
Copy link
Member

Hi @juanjqo,

Thank you! I believe we're good to go. Do you have any comments on this PR, @mmmarinho?

Kind regards,
Frederico

@mmmarinho
Copy link
Member

Hi @juanjqo and @ffasilva

There are a lot of changes in this pull request so I will need a report showing that you tested this code with the examples that already exist.

We cannot have CoppeliaSim-enabled examples in GithubActions (yet?), so this needs to be tested manually.

@bvadorno
Copy link
Member

bvadorno commented Nov 2, 2024

Hi @juanjqo and @ffasilva

There are a lot of changes in this pull request so I will need a report showing that you tested this code with the examples that already exist.

We cannot have CoppeliaSim-enabled examples in GithubActions (yet?), so this needs to be tested manually.

@juanjqo, could you please use one of your computers (maybe the latest unmanaged one I gave you a couple of weeks ago) to set up CoppeliaSim-enabled examples in GitHub actions?

Thanks,
Bruno

@juanjqo
Copy link
Member Author

juanjqo commented Nov 4, 2024

@bvadorno Sure, I am going to check if it is possible.

Best regards,
Juancho

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.

4 participants