Add service to capture and save frame #68

torbsorb · 2022-04-21T13:37:18Z

It is very valuable to be able to save the frame
as a ZDF file, or export to other formats.
This commit introduces the service capture_and_save_frame
which both captures and saves the frame. It takes a
path as an argument. See README.md#capture_and_save_frame
for more information.

In order to show how to use this service we also add a
sample, both for Python and C++.

Closes #65

apartridge · 2022-04-21T13:43:02Z

README.md

+### capture_and_save_frame
+[zivid_camera/CaptureAndSaveFrame.srv](./zivid_camera/srv/CaptureAndSaveFrame.srv)
+
+Invoke this service to trigger a 3D capture and save the frame as ZDF file.


Consider to just refer back to the capture service section here?
Something ala "Does the same as the capture service, additionally it saves the frame to the [...]"
Then there is 1 less place to update when we add new topics etc.

apartridge · 2022-04-21T13:43:27Z

zivid_camera/src/zivid_camera.cpp

@@ -454,7 +479,7 @@ bool ZividCamera::isConnectedServiceHandler(IsConnected::Request&, IsConnected::
  return true;
 }

-void ZividCamera::publishFrame(Zivid::Frame&& frame)
+void ZividCamera::publishFrame(Zivid::Frame& frame)


You want this as const ref, does it not work?

didn't try, one sec

apartridge

All features we add in the ROS driver needs to have unit tests to ensure they work. So you should add some relevant tests for this new feature as well.

apartridge · 2022-04-21T13:46:12Z

zivid_camera/src/zivid_camera.cpp

+  serviceHandlerHandleCameraConnectionLoss();
+
+  const auto settings = capture_settings_controller_->zividSettings();
+
+  if (settings.acquisitions().isEmpty())
+  {
+    throw std::runtime_error("capture called with 0 enabled acquisitions!");
+  }
+
+  ROS_INFO("Capturing with %zd acquisition(s)", settings.acquisitions().size());
+  ROS_DEBUG_STREAM(settings);
+  auto frame = camera_.capture(settings);


This part is duplicated now (up to serviceHandlerHandleCameraConnectionLoss();). Can we make a "invokeCaptureAndPublishFrame" helper that does these lines? It can return the Frame object as well, for future saving down here?

already thought about it while writing, but I wanted to test and check that it works first. Then I got excited and pushed it :)

Change looks good. But now I am thinking maybe just "captureAndPublishFrame" is better name as invoke doesn't add much.

apartridge · 2022-04-21T13:48:27Z

zivid_camera/src/zivid_camera.cpp

@@ -240,6 +240,8 @@ ZividCamera::ZividCamera(ros::NodeHandle& nh, ros::NodeHandle& priv)
      nh_.advertiseService("camera_info/serial_number", &ZividCamera::cameraInfoSerialNumberServiceHandler, this);
  is_connected_service_ = nh_.advertiseService("is_connected", &ZividCamera::isConnectedServiceHandler, this);
  capture_service_ = nh_.advertiseService("capture", &ZividCamera::captureServiceHandler, this);
+  capture_and_save_frame_service_ =
+      nh_.advertiseService("capture_and_save_frame", &ZividCamera::captureAndSaveFrameServiceHandler, this);


We also discussed just capture_and_save, did you prefer keeping frame? Another slightly more verbose option is capture_and_save_to_file

I tried to stay a little bit consistent with other similar services. "frame" seems to be a valid common concept.

capture_and_save can open up questions about what? Same with capture_and_save_to_file.

apartridge · 2022-04-21T13:49:43Z

zivid_samples/scripts/sample_capture_and_save_frame.py

+    def capture(self):
+        rospy.loginfo("Calling capture_and_save_frame service")
+        samples_path = rospkg.RosPack().get_path("zivid_samples")
+        file_path = samples_path + "/capture_py.zdf"


I don't think we want to save the file into the samples directory. Then I would rather save into /tmp/ and then the customer can update based on what they want?

Sure, I don't care that much

apartridge · 2022-04-21T16:37:05Z

zivid_samples/src/sample_capture_and_save_frame.cpp

+  zivid_camera::CaptureAndSaveFrame capture_and_save_frame;
+  std::string samples_path = ros::package::getPath("zivid_samples");
+  std::string file_path = samples_path + "/capture_cpp.zdf";
+  ROS_INFO("and save frame to: %s", file_path.c_str());


This will be a separate log line, so it looks better with just "Saving frame to %s" or something

I will remove it. Didn't add it in the Python code, and the service will log it.

torbsorb · 2022-04-22T10:07:06Z

We already do the same in testCapturePublishesTopics. Can you extract that to a helper class, something like a AllCaptureTopicsSubscriber, which then subscribes on all of them and have a method to check the current count? Then it can be reused both places.

I'm a little unsure how I should extract it. The way it's used in testCapturePublishesTopics is that it's subscribed once, and tested multiple times. Should I create a std::vector<?> that can be passed around? Or, can I add the subscribers as members of a subclass of ZividNodeTest that is initialized when the class is created?

apartridge

You are pushing zivid_samples/capture_py.zdf and zivid_samples/capture_cpp.zdf now, they should not be included.

apartridge · 2022-04-26T12:32:48Z

zivid_samples/src/sample_capture_and_save_frame.cpp

+
+  enable_first_acquisition();
+
+  ROS_INFO("Calling capture_and_save_frame");


You are already logging inside of capture_and_save_frame, so maybe remove this log.

torbsorb · 2022-05-13T09:32:47Z

@apartridge, can we revisit this and maybe merge it?

robertkt1 · 2022-08-08T09:51:38Z

Any updates on this one?

apartridge · 2022-09-09T09:42:11Z

README.md

@@ -618,6 +628,27 @@ rosrun zivid_samples sample_capture_with_settings_from_yml_cpp
 rosrun zivid_samples sample_capture_with_settings_from_yml.py
 ```

+### Sample Capture and Save Frame


Consider to remove Frame here just for consistency with the sample name.

apartridge · 2022-09-09T09:44:10Z

zivid_camera/test/test_zivid_camera.cpp

@@ -77,6 +78,7 @@ class ZividNodeTest : public ZividNodeTestBase
  const ros::Duration medium_wait_duration{ 1.0 };
  const ros::Duration dr_get_max_wait_duration{ 5 };
  static constexpr auto capture_service_name = "/zivid_camera/capture";
+  static constexpr auto capture_service_and_save_name = "/zivid_camera/capture_and_save";


this should be "capture_and_save_service_name".
It is the service name for "capture_and_save".

apartridge · 2022-09-09T09:45:15Z

zivid_camera/test/test_zivid_camera.cpp

+      ASSERT_FALSE(boost::filesystem::exists(file_path));
+    }
+    short_wait_duration.sleep();
+    allCaptureTopicsSubscriber.assert_num_topics_received(1);


I would throw allCaptureTopicsSubscriber.assert_num_topics_received(0); before the service is invoked above. Then we know that the topics were sent as a result of the capture you invoked.

It is very valuable to be able to save the frame as a ZDF file, or export to other formats. This commit introduces the service capture_and_save_frame which both captures and saves the frame. It takes a path as an argument. See README.md#capture_and_save_frame for more information. Enable diagnostics mode in the capture so that Customer Success can better analyze the data. In order to show how to use this service we also add a sample, both for Python and C++. We also include a test. Closes #65

torbsorb · 2024-01-22T10:54:42Z

zivid_samples/scripts/sample_capture_and_save.py

+        settings_client = dynamic_reconfigure.client.Client("/zivid_camera/settings/")
+        settings_config = {"diagnostics_enabled": True}
+        settings_client.update_configuration(settings_config)


Is this all that is required to enable diagnostics?

torbsorb requested a review from apartridge April 21, 2022 13:37

apartridge reviewed Apr 21, 2022

View reviewed changes

torbsorb force-pushed the issue_67_add_capture_and_save_frame_service branch from 58c3688 to 38f9eaa Compare April 21, 2022 18:19

torbsorb requested a review from apartridge April 21, 2022 18:19

torbsorb force-pushed the issue_67_add_capture_and_save_frame_service branch 3 times, most recently from fdbd4c5 to 44fd5ea Compare April 22, 2022 13:55

apartridge reviewed Apr 26, 2022

View reviewed changes

apartridge mentioned this pull request Sep 3, 2022

Convert pcl::PointCloud or sensor_msgs::PointCloud2 to Zivid::PointCloud #73

Open

torbsorb force-pushed the issue_67_add_capture_and_save_frame_service branch 2 times, most recently from d509646 to aae958d Compare September 9, 2022 06:45

apartridge reviewed Sep 9, 2022

View reviewed changes

apartridge previously approved these changes Sep 9, 2022

View reviewed changes

torbsorb dismissed apartridge’s stale review via a3aad46 September 9, 2022 10:59

apartridge force-pushed the issue_67_add_capture_and_save_frame_service branch from c290ceb to ead1a3e Compare January 12, 2024 15:21

apartridge force-pushed the issue_67_add_capture_and_save_frame_service branch 3 times, most recently from 8e1768a to 193578f Compare January 22, 2024 10:45

apartridge force-pushed the issue_67_add_capture_and_save_frame_service branch from 193578f to 05964d6 Compare January 22, 2024 10:52

torbsorb commented Jan 22, 2024

View reviewed changes

apartridge approved these changes Jan 22, 2024

View reviewed changes

apartridge merged commit 043bc83 into master Jan 22, 2024
20 checks passed

apartridge deleted the issue_67_add_capture_and_save_frame_service branch January 22, 2024 12:40

apartridge mentioned this pull request Jan 22, 2024

Enable diagnostics mode in the sample which saves frame #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service to capture and save frame #68

Add service to capture and save frame #68

torbsorb commented Apr 21, 2022

apartridge Apr 21, 2022

apartridge Apr 21, 2022

torbsorb Apr 21, 2022

apartridge left a comment

apartridge Apr 21, 2022 •

edited

Loading

torbsorb Apr 21, 2022

apartridge Apr 21, 2022 •

edited

Loading

apartridge Apr 21, 2022

torbsorb Apr 21, 2022

torbsorb Apr 21, 2022

apartridge Apr 21, 2022

torbsorb Apr 21, 2022

apartridge Apr 21, 2022

torbsorb Apr 21, 2022

torbsorb commented Apr 22, 2022

apartridge left a comment

apartridge Apr 26, 2022

torbsorb Apr 26, 2022

torbsorb commented May 13, 2022

robertkt1 commented Aug 8, 2022 •

edited

Loading

apartridge Sep 9, 2022

torbsorb Sep 9, 2022

apartridge Sep 9, 2022

torbsorb Sep 9, 2022

apartridge Sep 9, 2022

torbsorb Sep 9, 2022

torbsorb Jan 22, 2024

apartridge Jan 22, 2024


		enable_first_acquisition();

		ROS_INFO("Calling capture_and_save_frame");

Add service to capture and save frame #68

Add service to capture and save frame #68

Conversation

torbsorb commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apartridge left a comment

Choose a reason for hiding this comment

apartridge Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apartridge Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torbsorb commented Apr 22, 2022

apartridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torbsorb commented May 13, 2022

robertkt1 commented Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apartridge Apr 21, 2022 •

edited

Loading

apartridge Apr 21, 2022 •

edited

Loading

robertkt1 commented Aug 8, 2022 •

edited

Loading