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

Melodic devel #4

Merged
merged 7 commits into from Mar 13, 2019
Merged

Conversation

@romainreignier
Copy link
Contributor

romainreignier commented Nov 13, 2018

Add some fixes to use on Melodic with Ubuntu bionic, so a more recent version of gphoto2.
Plus various fixes.

I think a melodic-devel branch should be created to merge this PR.

@dejanpan

This comment was marked as resolved.

Copy link

dejanpan commented Mar 4, 2019

@iluetkeb is anyone still watching this repo and can review this PR?

@lorenzoriano

This comment was marked as resolved.

Copy link

lorenzoriano commented Mar 4, 2019

I don't know who's in charge of this whole organization.. @proan do you know who is in the robotics team?

@ralph-lange

This comment was marked as resolved.

Copy link

ralph-lange commented Mar 5, 2019

@romainreignier, thank you very much for contributing to this repo. As indicated in the previous comments by @dejanpan and @lorenzoriano, the maintenance of this repo is currently unclear. I'll check this with my colleagues and then come back to you in the next days.

Copy link
Contributor

Karsten1987 left a comment

@romainreignier thanks for the patch. I am requesting changes because of the segmentation fault issue when no cameras are available.

I just uploaded a melodic-devel branch. Please feel free to rebase this PR onto the new melodic branch.

CMakeLists.txt Outdated Show resolved Hide resolved
src/libphoto2/photo_camera.cpp Outdated Show resolved Hide resolved
src/libphoto2/photo_camera.cpp Outdated Show resolved Hide resolved
include/photo/photo_reporter.hpp Outdated Show resolved Hide resolved
include/photo/photo_reporter.hpp Outdated Show resolved Hide resolved
@romainreignier romainreignier force-pushed the romainreignier:melodic-devel branch from 48120b4 to c2582db Mar 11, 2019
@romainreignier romainreignier changed the base branch from hydro-devel to melodic-devel Mar 11, 2019
@romainreignier romainreignier force-pushed the romainreignier:melodic-devel branch from 83e748e to 7425493 Mar 11, 2019
@@ -219,7 +241,7 @@ int photo_camera::photo_camera_find_widget_by_name( std::string name, CameraWidg
* #include <boost/algorithm/string.hpp>
* boost::iequals( s1, s2 );

This comment has been minimized.

Copy link
@romainreignier

romainreignier Mar 11, 2019

Author Contributor

I have a patch to use boost::iequals as the comment requested but maybe it's better to wait for this PR to be merged first?

Copy link
Contributor

Karsten1987 left a comment

a few nitpicks, but looks good to me.
Thanks for iterating over this patch.




int photo_camera_find_widget_by_name( std::string param, CameraWidget **child, CameraWidget **rootconfig );
int photo_camera_find_widget_by_name( std::string param, CameraWidget **child,

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Mar 12, 2019

Contributor
Suggested change
int photo_camera_find_widget_by_name( std::string param, CameraWidget **child,
int photo_camera_find_widget_by_name( const std::string& param, CameraWidget **child,

This comment has been minimized.

Copy link
@romainreignier

romainreignier Mar 13, 2019

Author Contributor

This one is not that easy because the param is modified in the method.

See here for example:

https://github.com/romainreignier/photo/blob/c94034a08bc6298650cb03a99d08f5097172c43b/src/libphoto2/photo_camera.cpp#L199

If you prefer I can create a local variable. But in any case a copy is needed.

src/libphoto2/photo_camera.cpp Outdated Show resolved Hide resolved
src/libphoto2/photo_camera.cpp Outdated Show resolved Hide resolved
}
return true;
}




int photo_camera::photo_camera_find_widget_by_name( std::string name, CameraWidget **child, CameraWidget **root)
int photo_camera::photo_camera_find_widget_by_name( std::string name, CameraWidget **child,

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Mar 12, 2019

Contributor
Suggested change
int photo_camera::photo_camera_find_widget_by_name( std::string name, CameraWidget **child,
int photo_camera::photo_camera_find_widget_by_name( const std::string& name, CameraWidget **child,
@Karsten1987

This comment has been minimized.

Copy link
Contributor

Karsten1987 commented Mar 13, 2019

@romainreignier if you could accept the latest suggestions, I am happy to accept the PR and re-release the package in melodic.

@romainreignier romainreignier force-pushed the romainreignier:melodic-devel branch from 7425493 to 7f39f5b Mar 13, 2019
@Karsten1987 Karsten1987 merged commit 0e88c8f into bosch-ros-pkg:melodic-devel Mar 13, 2019
@Karsten1987

This comment has been minimized.

Copy link
Contributor

Karsten1987 commented Mar 13, 2019

@romainreignier for iterating over this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.