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.

Sign up for GitHub

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

Jump to bottom

Improvment of service manager #257

Merged
merged 22 commits into from
Jan 20, 2015
Merged

Improvment of service manager #257

merged 22 commits into from
Jan 20, 2015

Conversation

dwlee
Copy link
Collaborator

@dwlee dwlee commented Jan 19, 2015

The PR is to support local solution instance from default solution set. It includes.

  • local solution cache creation. under ROS_HOME/rocon/
  • new ROS api in service manager to configure service paremeterisation.

Detailed design discussion in #254

This was referenced Jan 19, 2015
Merged
Merged
Merged
@dwlee
Copy link
Collaborator Author

dwlee commented Jan 19, 2015

@dwlee dwlee mentioned this pull request Jan 19, 2015
Merged
@jihoonl jihoonl changed the title Admin app patch Improvment of service manager Jan 19, 2015
@jihoonl
Copy link
Collaborator

jihoonl commented Jan 19, 2015

https://github.com/robotics-in-concert/rocon_concert/blob/admin_app_patch/concert_service_manager/src/concert_service_manager/load_params.py#L20-L51

these two methods can share one logic.

@jihoonl
Copy link
Collaborator

jihoonl commented Jan 19, 2015

There are lots places where calls rocon_python_utils.ros.find_resource_from_string but do not catch rospkg.ResourceNotFound exception. It should be handled somewhere.

jihoonl
jihoonl reviewed Jan 19, 2015
View reviewed changes
concert_service_manager/src/concert_service_manager/service_cache_manager.py
with open(parameters_yaml_file) as f:
parameters_yaml = yaml.load(f)
loaded_profile['parameters_detail'] = parameters_yaml
except rospkg.ResourceNotFound as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which code does raise this exception?

@jihoonl
Copy link
Collaborator

jihoonl commented Jan 19, 2015

https://github.com/robotics-in-concert/rocon_concert/pull/257/files#diff-ea49e765092ef39b9b7b499bfacd4e17R99

We may want to split this as load_from_file and load_from_resource.

jihoonl
jihoonl reviewed Jan 20, 2015
View reviewed changes
concert_service_manager/src/concert_service_manager/service_cache_manager.py
]

def __init__(self, concert_name, resource_name, modification_callback=None):
self._concert_name = concert_name.strip().lower().replace(' ', '_')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this use https://github.com/robotics-in-concert/rocon_tools/pull/73/files#diff-f384ea7500354bbc728b8b10d20f119bR38?

@jihoonl jihoonl mentioned this pull request Jan 20, 2015
Open
@jihoonl
Copy link
Collaborator

jihoonl commented Jan 20, 2015

👍 I have created #261 to follow up minor changes later.

jihoonl added a commit that referenced this pull request Jan 20, 2015
@jihoonl
Merge pull request #257 from robotics-in-concert/admin_app_patch
9b4774e 
Improvment of service manager
@jihoonl jihoonl merged commit 9b4774e into indigo Jan 20, 2015
@jihoonl jihoonl deleted the admin_app_patch branch January 20, 2015 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dwlee @jihoonl