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

Refactor configuration structure #652

Merged
merged 7 commits into from
Apr 10, 2017

Conversation

fmessmer
Copy link
Member

@fmessmer fmessmer commented Apr 5, 2017

@fmessmer fmessmer changed the title Refactor configuration structure [WIP] Refactor configuration structure Apr 5, 2017
@ipa-nhg
Copy link
Member

ipa-nhg commented Apr 6, 2017

Sorry, where's the advantage here? This just add new subfolders...

@fmessmer fmessmer force-pushed the refactor_configuration_structure branch from 97713f4 to e3736cc Compare April 6, 2017 06:35
@fmessmer
Copy link
Member Author

fmessmer commented Apr 6, 2017

Sorry, where's the advantage here? This just add new subfolders...

It's sync'ing the structure with the nav configuration and allows for a configuration within a single package, e.g. for demos, while keeping the various config sources grouped semantically without mixing them, i.e. script_server configs in its folder, nav configs within its folder...

eventually, #437 will introduce a components sub-folder beside robots (in cob_hardware_config) where we can move component-specific, but robot-independent configs...

@fmessmer
Copy link
Member Author

fmessmer commented Apr 6, 2017

I consider this done so far....
In https://github.com/ipa320/msh/pull/684, I am testing against all related feature branches...

@ipa-mig @ipa-fmw @ipa-nhg
please review

@fmessmer
Copy link
Member Author

fmessmer commented Apr 6, 2017

maybe we should rebase on top of #645 after that has been merged...

Copy link
Member

@mgruhler mgruhler left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes. However, for functionality, I guess we have to trust in travis :-)

endforeach()
install(DIRECTORY robots
DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}
)
Copy link
Member

Choose a reason for hiding this comment

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

This simplification is a nice side effect :-)

@ipa-nhg
Copy link
Member

ipa-nhg commented Apr 7, 2017

I understand the point of unify the configuration but I don't think this structure is the right one for cob_hardware_config, For this package make not sense, and it is the main configuration package of Care-O-bot. Last time we discussed the structure of cob_robots together, we decided to move into a new structure where we will only have component/module specific configuration and not robots anymore. And this pull request is actually doing the opposite.

@@ -1,6 +1,6 @@
moveit_setup_assistant_config:
URDF:
package: cob_hardware_config
relative_path: cob4-7/urdf/cob4-7.urdf.xacro
relative_path: robots/cob4-7/urdf/cob4-7.urdf.xacro
SRDF:
relative_path: config/cob4-7.srdf
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 you keep only for cob_moveit_config the structure package_name/$(arg robot)/...

@fmessmer fmessmer force-pushed the refactor_configuration_structure branch from 2d538e3 to cf67d80 Compare April 7, 2017 11:38
@fmessmer fmessmer self-assigned this Apr 7, 2017
@fmessmer fmessmer force-pushed the refactor_configuration_structure branch from cfaf230 to 461b121 Compare April 10, 2017 14:30
@fmessmer
Copy link
Member Author

cob_moveit_config has been restructured to the same layout for consistency...

@fmessmer fmessmer changed the title [WIP] Refactor configuration structure Refactor configuration structure Apr 10, 2017
@fmessmer fmessmer merged commit c6bacae into ipa320:indigo_dev Apr 10, 2017
@fmessmer fmessmer deleted the refactor_configuration_structure branch April 10, 2017 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants