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

Topic/device urdfdom #136

Open
wants to merge 47 commits into
base: devel
Choose a base branch
from

Conversation

NoelieRamuzat
Copy link
Contributor

Add a generic device in sot-core without the integrator and using the YAML config files.
WARNING: Do not have the state signal anymore !

Currently I have some trouble with the python tests files so it is a draft PR.

NoelieRamuzat added 12 commits July 31, 2019 12:21
End of the integration removal (remove stateSOUT)
Add of force index 
Change the index of the control signal (defined by the yaml file with the key joint_names)
Add an example of yaml file config that can be use for the device (the same generated by the unitTest with the function CreateYAMLFILE()).
Add two yaml files which reflect how the data are split in two in the sot, these ones are used by the unitTest. 
The control is set has constant = -0.5 for all joints in position; the device output a control signal respecting the position limits of the joints.
These signals are taken by the device accordingly to the types of the actuators (defined by the yaml file). Add their gains signals in entry for a future use.
Signals are registered in the entity after the reading of the yaml file to register only the needed one.
@@ -535,4 +535,4 @@ void Device::cmdDisplay() {
std::cout << name << ": " << state_ << endl
<< "sanityCheck: " << sanityCheck_ << endl
<< "controlInputType:" << controlInputType_ << endl;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid this type of modification that pollutes the history.

Copy link
Member

Choose a reason for hiding this comment

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

This probably due to a trailing whitespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is due to a lack of newline at the end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didn't see that I removed the last line I will correct it with the other remarks.

@@ -36,4 +36,4 @@ RobotSimu::RobotSimu(const std::string &inName) : Device(inName) {
makeDirectSetter(*this, &this->timestep_, docstring));
}
} // namespace sot
} // namespace dynamicgraph
} // namespace dynamicgraph
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid this type of modification that pollutes the history.

@@ -123,4 +131,4 @@ ENDFOREACH(path ${tests})

IF(BUILD_PYTHON_INTERFACE)
ADD_SUBDIRECTORY(python)
ENDIF(BUILD_PYTHON_INTERFACE)
ENDIF(BUILD_PYTHON_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 avoid this type of modification that pollutes the history.

CMakeLists.txt Outdated
@@ -167,11 +170,18 @@ SET(${PROJECT_NAME}_SOURCES
src/utils/stop-watch
)

IF (YAML_CPP_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

if yaml-cpp is required (and I think it should be), this IF is useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree I will remove it.

@@ -95,6 +95,14 @@ SET(tests
matrix/test_operator
)

IF (YAML_CPP_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


std::string robot_description;
ifstream urdfFile;
std::string filename = "/opt/openrobots/share/simple_humanoid_description/urdf/simple_humanoid.urdf";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to avoid such full paths. if you just copy-pasted that from the older device test, it's ok in this PR, otherwise we should implement a bit of CMake logic for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not like this in the older device because we did not have to pass the urdf.
So yes it would be better to change the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

olivier-stasse and others added 15 commits June 30, 2020 11:45
and save it in the internal sot based parameter-server.
This reverts commit e5b7857.

Do not merge directly in master...
This is missing when compiling with clang on Mac OS Catalina.
Moreover it seems needed anyway.
…terface.

This avoid unnecessary target_include_directories.
Pointed out by G. Saurel and J. Mirabel.
Instance of the template is created in feature-pose.
extern tag added in feature-pose.hh
Update of test_feature_generic.
olivier-stasse and others added 20 commits July 11, 2020 08:58
Make it internal to the implementation.
Release of version 4.10.0.
Use CMAKE path to find the urdf
@NoelieRamuzat
Copy link
Contributor Author

I have taken into account the comments (tell me if I forgot something).
Still Travis is failing because of the yaml-cpp dependency which is not found.

@NoelieRamuzat NoelieRamuzat marked this pull request as ready for review July 31, 2020 08:57
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.

5 participants