SVO logging #37

ds58 · 2025-01-13T23:13:45Z

No description provided.

ds58 · 2025-01-14T17:20:35Z

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLoggerDispatcher.java


 public class YoVariableLoggerDispatcher implements DataServerDiscoveryListener
 {
+   private static final boolean ZED_SDK_LOADED = ZEDJavaAPINativeLibrary.load();


ZEDJavaAPINativeLibrary.load() only gets called once, it will return false if ZED SDK is not available on the system

Should this get called in the manager? That way we don't have to pass it all the way down.
I don't feel the user needs to be aware this is happening unless they go looking at the manager it self.

I was thinking that too when I first saw it

ds58 · 2025-01-14T17:21:14Z

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLoggerDispatcher.java

@@ -82,8 +91,7 @@ public void connected(HTTPDataServerConnection connection)
            {
               try
               {
-                  new YoVariableLogger(connection, options, (request) -> finishedLog(request));
-                  activeLogSessions.add(hashAnnouncement);
+                  activeLogSessions.put(hashAnnouncement, new YoVariableLogger(connection, options, this::finishedLog, ZED_SDK_LOADED));


We pass ZED_SDK_LOADED into YoVariableLogger so it knows if it should initialize ZEDSVOLoggerManager or not

ds58 · 2025-01-14T17:25:00Z

build.gradle.kts

@@ -39,7 +39,7 @@ mainDependencies {
   api("us.ihmc:ihmc-video-codecs:2.1.6")
   api("us.ihmc:ihmc-realtime:1.7.0")
   api("us.ihmc:ihmc-java-decklink-capture:0.4.0")
-   api("us.ihmc:ihmc-pub-sub:1.1.5")
+   api("us.ihmc:ros2-library:1.1.5")


Added because ZEDSVOLoggerManager uses a ROS2Node for subscribing to the ZEDSDKAnnounce topic.

ds58 · 2025-01-14T17:25:19Z

src/main/idl/ZEDSDK.idl

We define the messages in the logger as IDL, not .msg

Seems you generated these and then copied them in. I think it would make sense to add the Gradle task to generate these, (maybe even start from .msg, just to be the same as everywhere else.)

No, I manually wrote this idl. I don't want to convert everything here to .msg, that is a lot more work and unnecessary

Oh I see there's an existing generation step.

Yep you use gradle generateMessages from the project root dir

ds58 · 2025-01-14T17:29:35Z

Example log directory output with multiple SVOs. A new SVO is created each time the ZED SDK is restarted on the robot.

d@d-desktop2:~/robotLogs/20250114_105240_ExampleServer$ tree .
.
├── handshake.yaml
├── perception
│   ├── 20250114_105242_ZEDRecording.svo2
│   ├── 20250114_105331_ZEDRecording.svo2
│   └── 20250114_105343_ZEDRecording.svo2
├── robotData.bsz
├── robotData.dat
└── robotData.log

1 directory, 7 files

ds58 · 2025-01-14T17:35:15Z

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLoggerDispatcher.java

@@ -119,7 +130,8 @@ private void finishedLog(Announcement request)
         // Then remove this session from the list of active log sessions
         ThreadTools.sleep(2000);
         HashAnnouncement hashRequest = new HashAnnouncement(request);
-         activeLogSessions.remove(hashRequest);
+         YoVariableLogger logger = activeLogSessions.remove(hashRequest);
+         logger.destroy();


YoVariableLogger has a destroy() now, which gets called when a log has finished

ds58 · 2025-01-14T17:35:34Z

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLoggerDispatcher.java

+      Runtime.getRuntime().addShutdownHook(new Thread(() ->
+      {
+         activeLogSessions.forEach((hashAnnouncement, yoVariableLogger) -> yoVariableLogger.destroy());
+      }, getClass().getName() + "Shutdown"));


Calling YoVariableLogger#destroy() when the logger shuts down

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLogger.java

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java

Co-authored-by: Tomasz Bialek <[email protected]>

PotatoPeeler3000

This is pretty awesome. I've got some concerns about the ZedSVOLoggerManager and the ZEDSVOLogger. Some stuff about threading, and is all of its necessary. Let me know what you think. Can also tag up in person if that helps explain things.

PotatoPeeler3000 · 2025-01-27T15:12:11Z

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLoggerDispatcher.java


 public class YoVariableLoggerDispatcher implements DataServerDiscoveryListener
 {
+   private static final boolean ZED_SDK_LOADED = ZEDJavaAPINativeLibrary.load();


Should this get called in the manager? That way we don't have to pass it all the way down.
I don't feel the user needs to be aware this is happening unless they go looking at the manager it self.

PotatoPeeler3000 · 2025-01-27T15:14:47Z

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java

+      {
+         ZEDSVOLogger zedSVOLogger = zedLoggers.get(announceHash);
+
+         if (zedSVOLogger.completelyStopped() && !zedSVOLogger.failedBeyondRecovery())


Do we need to do this? Why do we care about removing it from the list? The zed logger doesn't get stopped from what I can tell either.

This is where I think we could check if things were stopped() rather then completelyStopped() and we could remove completely stopped. In general there shouldn't be a difference between stopped and completely stopped so I think we should only have one.

We want to keep it around if it is failedBeyondRecovery() so that it doesnt try to reconnect, which may cause a native crash (crashing the logger)

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java

PotatoPeeler3000 · 2025-01-27T15:18:58Z

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java

+   /**
+    * @return true if ZED SDK has completely closed the camera and stop() has completely finished
+    */
+   public boolean completelyStopped()


The difference between stopped and completely stopped isn't clear. It doesn't make sense to have both. I think we could change this to just have the stopped() check, and we can check that in the manager as well if needed.

I've renamed stopped() to stopRequested() so that it is more clear

PotatoPeeler3000 · 2025-01-27T15:35:15Z

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java

+         grabThread.startRepeating();
+      }
+
+      connectionWatchdogThread.setFrequencyLimit(Conversions.secondsToHertz(CONNECT_TIMEOUT));


Is it possible to remove the connectionWatchdogThread and check for a timeout within grabThread. Starting 2 threads in this class seems like too much.
The grabThread could have a different frequency. So its not unlimited, and we could have a counter that when it reaches a certain number it assumes a timeout. And each time we get a new frame, that timer gets reset. So its only ticking up when no frames are being received.

I haven't looked but do you know how the BlackMagicLogger times out after 5 seconds? We could pattern match that one. That may happen on the native side though I'm not sure. Starting 2 threads concerns me.

The grabThread is unlimited frequency to avoid any delay or latency. The ZED SDK handles throttling the frequency because sl_grab() is blocking. This is the best way to do it.

I'd rather not put the timeout stuff in the grabThread because the logic would be more complicated.

Starting 2 threads concerns me.

why?

PotatoPeeler3000 · 2025-01-27T15:39:26Z

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java

+   }
+
+   public void grab()
+   {


It the grab() method we should be checking if we can still grab a frame. So part of the connectionCheck() should happen in this method. That way we check if we can grab right before we attempt to grab a frame.

connectionCheck has to be asynchronous, because grab() is blocking

ds58 added 5 commits January 10, 2025 10:55

SVO logging

34455fd

Use instanceID, try to make things more crash safe

f0c1b7a

Remove instanceID usage

fc8acae

Try to be somewhat safer

d413711

Add ZEDSVOLoggerManager

21e405c

ds58 changed the title ~~[WIP] SVO logging~~ SVO logging Jan 14, 2025

ds58 marked this pull request as ready for review January 14, 2025 17:17

Move ZED SDK loading

e80dc5b

ds58 commented Jan 14, 2025

View reviewed changes

Add some javadoc

c3e7926

ds58 commented Jan 14, 2025

View reviewed changes

ds58 added 2 commits January 14, 2025 11:27

javadoc

6d9d9e3

javadoc

ddc8ec4

ds58 requested review from TomaszTB, PotatoPeeler3000 and calvertdw January 14, 2025 17:29

ds58 commented Jan 14, 2025

View reviewed changes

ds58 added 2 commits January 14, 2025 11:56

Call sl_unload_instance when shutting down ZEDSVOLogger

f5d6125

Add some static variables

a40eed6

ds58 commented Jan 14, 2025

View reviewed changes

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java Show resolved Hide resolved

calvertdw reviewed Jan 14, 2025

View reviewed changes

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java Outdated Show resolved Hide resolved

calvertdw reviewed Jan 14, 2025

View reviewed changes

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java Outdated Show resolved Hide resolved

calvertdw reviewed Jan 14, 2025

View reviewed changes

src/main/java/us/ihmc/robotDataLogger/logger/YoVariableLogger.java Outdated Show resolved Hide resolved

ds58 added 3 commits January 14, 2025 16:06

Dont transcode, remove extra check

b542a09

Change ROS2Node name

9d0d12f

Add message if ZED SDK not installed

11fc197

ds58 added 7 commits January 14, 2025 16:12

Formatting

d5b2164

Remove unused field

e70e055

Switch connection watchdog to RepeatingTaskThread

da17a3f

ConnectionWatchdog stuff

3672c3f

Formatting

af1c57b

Stop connection watchdog thread

4cfd3f3

Minor fix

94c3f8e

calvertdw approved these changes Jan 14, 2025

View reviewed changes

TomaszTB approved these changes Jan 15, 2025

View reviewed changes

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLogger.java Outdated Show resolved Hide resolved

src/main/java/us/ihmc/robotDataLogger/logger/ZEDSVOLoggerManager.java Outdated Show resolved Hide resolved

ds58 and others added 6 commits January 15, 2025 10:44

Use File.separator

cd3fb45

Co-authored-by: Tomasz Bialek <[email protected]>

Minor change

4bbc0c5

Add example SVO logger

7e961ba

Do not use ZEDSVOLoggerManager in ExampleSVOLogger

3d99601

Fix file extention

12eb493

Update javadoc

9dbb409

PotatoPeeler3000 reviewed Jan 27, 2025

View reviewed changes

ds58 added 5 commits January 27, 2025 13:14

Merge branch 'develop' into svo-logging

c03c88b

Move file creation

568b0d6

Rename stopped()

3ecf51e

Move ZED_SDK_LOADED

7a5320e

Move usage of ZED_SDK_LOADED

09d4435

ds58 requested a review from PotatoPeeler3000 January 31, 2025 18:21

PotatoPeeler3000 approved these changes Jan 31, 2025

View reviewed changes

ds58 merged commit c2258a5 into develop Jan 31, 2025
2 checks passed

ds58 deleted the svo-logging branch January 31, 2025 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SVO logging #37

SVO logging #37

ds58 commented Jan 13, 2025

ds58 Jan 14, 2025

PotatoPeeler3000 Jan 27, 2025

calvertdw Jan 27, 2025

ds58 Jan 14, 2025

ds58 Jan 14, 2025

ds58 Jan 14, 2025

calvertdw Jan 14, 2025

ds58 Jan 14, 2025

calvertdw Jan 14, 2025

ds58 Jan 14, 2025

ds58 commented Jan 14, 2025

ds58 Jan 14, 2025

ds58 Jan 14, 2025

PotatoPeeler3000 left a comment

PotatoPeeler3000 Jan 27, 2025

PotatoPeeler3000 Jan 27, 2025

PotatoPeeler3000 Jan 27, 2025

ds58 Jan 27, 2025

PotatoPeeler3000 Jan 27, 2025

ds58 Jan 27, 2025

PotatoPeeler3000 Jan 27, 2025

PotatoPeeler3000 Jan 27, 2025

ds58 Jan 27, 2025

ds58 Jan 27, 2025 •

edited

Loading

PotatoPeeler3000 Jan 27, 2025

ds58 Jan 27, 2025

SVO logging #37

SVO logging #37

Conversation

ds58 commented Jan 13, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds58 commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PotatoPeeler3000 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds58 Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds58 Jan 27, 2025 •

edited

Loading