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

first version of transmissions for mock hardware #15

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

Conversation

mamueluth
Copy link
Member

@mamueluth mamueluth commented Feb 14, 2024

Can be tested with StoglRobotics-forks/ros2_control_demos#10
TODOS:

  • Set initial values in on_init
  • Add other Transmission types
  • Add tests

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0bdcd41) 47.53% compared to head (5b406aa) 46.38%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   47.53%   46.38%   -1.15%     
==========================================
  Files          41       39       -2     
  Lines        3547     3253     -294     
  Branches     1930     1749     -181     
==========================================
- Hits         1686     1509     -177     
+ Misses        459      458       -1     
+ Partials     1402     1286     -116     
Flag Coverage Δ
unittests 46.38% <ø> (-1.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

mock_hardware/include/mock_components/generic_system.hpp Outdated Show resolved Hide resolved
Comment on lines 104 to 122
// used for the Transmission pass through.
// read: actuator_interface.state_->Transmission->joint_interface.state_
// write: joint_interface.command_->Transmission->actuator_interface.command_
// And StateInterface(joint_interface.state_)
struct InterfaceData
{
// TODO(Manuel) set initial to NaN and on_init initialize to given value in info or 0.0
explicit InterfaceData(const std::string & name)
: name_(name),
command_(0.0), // command_(std::numeric_limits<double>::quiet_NaN()),
state_(0.0), // state_(std::numeric_limits<double>::quiet_NaN()),
transmission_passthrough_(
0.0) // transmission_passthrough_(std::numeric_limits<double>::quiet_NaN())
{
}

std::string name_;
double command_;
double state_;
// this is the "sink" that will be part of the transmission Joint/Actuator handles
double transmission_passthrough_;
};
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this structure. but also it looks like something that might be added to .cpp file. Especially, since we have this game with 0.0 and quiet_NaN().

I have to check the implemenation to tell more about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

what about the struct? need it in the .hpp since we define the lists with it

mock_hardware/src/generic_system.cpp Show resolved Hide resolved

transmission_interface::JointHandle joint_handle(
joint_info.name, hardware_interface::HW_IF_POSITION,
&joint_interface->transmission_passthrough_);
Copy link
Member

Choose a reason for hiding this comment

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

why do we need transmission pass_though?

Copy link
Member Author

Choose a reason for hiding this comment

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

have to check this maybe using directly state_, command_ is fine but i think overriding was a problem

Copy link
Member Author

Choose a reason for hiding this comment

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

need it because we would override the handle otherwise (Handle for actuator/joint needs a storage, but we have state and command. We could only give one of the two, so if we call the functions actuator_to_joint or joint_to_actuator we would involunatry override )

std::for_each(
actuator_interfaces_.begin(), actuator_interfaces_.end(),
[](auto & actuator_interface)
{ actuator_interface.transmission_passthrough_ = actuator_interface.state_; });
Copy link
Member

Choose a reason for hiding this comment

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

Ah OK, this is just a storage. Can't we use directly actuator_interface.state_?

Copy link
Member Author

Choose a reason for hiding this comment

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

no see above comment

[](auto & actuator_interface)
{ actuator_interface.command_ = actuator_interface.transmission_passthrough_; });

// simulate motor motion
Copy link
Member

Choose a reason for hiding this comment

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

Is this important for something?

Copy link
Member Author

Choose a reason for hiding this comment

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

sry what do you exactly mean? i dont understand the question

Copy link

mergify bot commented Feb 19, 2024

This pull request is in conflict. Could you fix it @mamueluth?

Copy link

mergify bot commented Apr 29, 2024

This pull request is in conflict. Could you fix it @mamueluth?

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.

3 participants