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

Support sdk2 #32

Merged
merged 5 commits into from
Aug 19, 2020
Merged

Support sdk2 #32

merged 5 commits into from
Aug 19, 2020

Conversation

apartridge
Copy link
Collaborator

PR for updating ROS driver to support SDK 2.

nedrebo
nedrebo reviewed Jul 9, 2020
View reviewed changes
Copy link
Contributor

@nedrebo nedrebo left a comment

Choose a reason for hiding this comment

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

I browsed through the diff, no major comments from me, but someone else should do the proper review.

zivid_camera/src/settings_generator.cpp Show resolved Hide resolved
@runenordmo
Copy link

As far as I can see, only the last commit in the PR is related to updating to sdk 2.0.

I guess there in the same PR because you wanted those clean-ups in first, without the overhead of creating another PR that needed to be merged first?

@apartridge
Copy link
Collaborator Author

As far as I can see, only the last commit in the PR is related to updating to sdk 2.0.

I guess there in the same PR because you wanted those clean-ups in first, without the overhead of creating another PR that needed to be merged first?

Yes, this could have been split. To save time I think we can just review and merge them together.

@runenordmo
Copy link

image

Dynamic reconfigure looks nice 👍

What is the reason we do not have processing as a namespace in the same way as acquisition?

  • Is it because settings is the top level namespace, and that it could contain other things than processing? Is it a better match for the settings datamodel?

@runenordmo
Copy link

runenordmo commented Aug 11, 2020 •
edited
Loading

Btw, in my previous comment the defaults look correct, but for instance the range for the brightness looks wrong (I captured with an Amber, the range should be [0.0, 1.0]. Similar for exposure time).

The generated SettingsAcquisitionConfigUtils.h does look correct, though.

It looks like these values are the ones generated to be in SettingsAcquisition.cfg.

@apartridge
settings_generator: Speed up build by not modifying unchanged files
0a9d408 
The settings_generator step produces some .cfg files and header
files which is used by the zivid_camera library. The generator
would always overwrite the output files, even if the contents was
identical to before. This would cause an unnecessary rebuild
of the zivid_camera node.

Instead, we can check if the file contents are different before
modifying the output file. This will significantly speed up
subsequent builds without changes to settings_generator output.
@apartridge
zivid_camera.cpp: Use sensor_msgs::PointField enum instead of magic v…
6972ed7 
…alue
@apartridge
zivid_camera: Do publishing of messages inside the methods
e0d08e1 
A bit tidier this way.
@apartridge
Remove buggy zivid_camera_with_settings.launch file
9682e3d 
This launch file tried to set 3-frame HDR; but once in a while it
would not update all of the settings. It appears that one or more
of the dynparam nodes being created end up hanging forever (they
remain running in rosparam list). I am not sure why this is. For now
I am removing this sample .launch file. I have made issue 31 to for
reintroducing this sample.
@apartridge
Copy link
Collaborator Author

image

Dynamic reconfigure looks nice

What is the reason we do not have processing as a namespace in the same way as acquisition?

  • Is it because settings is the top level namespace, and that it could contain other things than processing? Is it a better match for the settings datamodel?

Yes, I considered separate namespace for processing, even implemented it at one point, but I decided on doing it like this. Because it requires fewer changes if we add something other than processing/ on the top level. Now it will just work. Otherwise, we would need to add new code to make a dynamic_reconfigure server for that new top level node, which I think is not worth it. Also we would end up with one more server per new top-level type (possibly small performance hit).

@apartridge
Copy link
Collaborator Author

Btw, in my previous comment the defaults look correct, but for instance the range for the brightness looks wrong (I captured with an Amber, the range should be [0.0, 1.0]. Similar for exposure time).

The generated SettingsAcquisitionConfigUtils.h does look correct, though.

It looks like these values are the ones generated to be in SettingsAcquisition.cfg.

Yeah it could be that the ranges are not exposed correctly, I will look into it. It will fail when capturing if outside of the amber-range, but it is still an issue if the range is wrong.

@runenordmo
Copy link

Have looked through the entire PR now. Looks solid 👍
Will have a look when you have gone through the comments and added the fixup commits too.

What changes will be made going from supporting beta to official 2.0 release? I guess at least updating the readme and what version azure pipelines uses?

@apartridge
Copy link
Collaborator Author

Have looked through the entire PR now. Looks solid
Will have a look when you have gone through the comments and added the fixup commits too.

What changes will be made going from supporting beta to official 2.0 release? I guess at least updating the readme and what version azure pipelines uses?

azure pipelines is already using the official 2.0 release. I don't expect any changes except changing the Changelog to remove beta.

@runenordmo runenordmo mentioned this pull request Aug 17, 2020
Closed
@runenordmo
Copy link

Have looked through the entire PR now. Looks solid
Will have a look when you have gone through the comments and added the fixup commits too.
What changes will be made going from supporting beta to official 2.0 release? I guess at least updating the readme and what version azure pipelines uses?

azure pipelines is already using the official 2.0 release. I don't expect any changes except changing the Changelog to remove beta.

Is there any reason the PR is still a draft?

@apartridge apartridge marked this pull request as ready for review August 17, 2020 08:20
runenordmo
runenordmo previously approved these changes Aug 17, 2020
View reviewed changes
@apartridge
Update ROS wrapper to version 2.0, with support for SDK 2.0
5363126 
This commit adds support for the Zivid 2.0 SDK.

This commit also contains some adjustments to the API of the ROS
driver, including renaming of topics, some new topics, and renaming
of dynamic_reconfigure nodes, etc. See CHANGELOG.md for full list
of changes in this release.

Internally, most of the changes are to work with API 2.0. There is also
some cleanup and refactoring. For example, handling of Settings and
Settings2D configuration is moved to a shared class
capture_settings_controller.h. This ensures that the settings and
settings_2d dynamic_reconfigure trees are consistent.

The depth image topic has been renamed from "depth/image_raw" to
"depth/image". Since this image contains depth as 32-bit floats, it
should be named "image" to follow the recommendation in
[REP-118: Depth Images](https://www.ros.org/reps/rep-0118.html).

test_zivid_camera.cpp now does output testing by capturing using the
SDK directly, and comparing the output from the SDK with the output
from ROS. Previously, testing was using hard-coded values for
MiscObjects.zdf. However, when testing ROS we can assume that the
output from SDK is good.
@apartridge
Copy link
Collaborator Author

@runenordmo can you re-approve this PR?

runenordmo
runenordmo approved these changes Aug 19, 2020
View reviewed changes
Copy link

@runenordmo runenordmo left a comment

Choose a reason for hiding this comment

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

Yes, re-approved 👍

@apartridge apartridge merged commit b4aa18b into master Aug 19, 2020
@apartridge apartridge deleted the support_sdk2 branch August 19, 2020 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nedrebo nedrebo nedrebo left review comments

@runenordmo runenordmo runenordmo approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apartridge @runenordmo @nedrebo