Conversation
MaticTonin
left a comment
There was a problem hiding this comment.
Overall looks good, put some general comments on what should be checked before even marking it as testable.
|
|
||
| # "full commit hash of device side binary" | ||
| set(DEPTHAI_DEVICE_SIDE_COMMIT "5aea90014bbf269c4f2e09243fcd502e28174be7") | ||
| set(DEPTHAI_DEVICE_SIDE_COMMIT "665f8773f7269292eabd2ee562debb2a778776a4") |
There was a problem hiding this comment.
Check what is latest develop, so we dont downgrade.
|
|
||
| # "version if applicable" | ||
| set(DEPTHAI_DEVICE_RVC4_VERSION "0.0.1+d5a6e66b0e2a72a400ac6a4cd1dc79eb033fa68a") | ||
| set(DEPTHAI_DEVICE_RVC4_VERSION "0.0.1+5ee962d36af9c2f04ec126aa10cc8083dba61dde") |
There was a problem hiding this comment.
Check what is latest develop, so we dont downgrade.
bindings/python/src/pipeline/datatype/AutoCalibrationResultBindings.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/pipeline/datatype/AutoCalibrationResultBindings.cpp
Outdated
Show resolved
Hide resolved
| : dataQuality(dataQuality), calibrationConfidence(calibrationConfidence), passed(passed), calibration(calibration) {}; | ||
| virtual ~AutoCalibrationResult(); | ||
|
|
||
| double dataQuality; |
There was a problem hiding this comment.
Same comments as above.
|
@JakubFara this doesn't have the packetization right? |
4ef8d97 to
610ef67
Compare
moratom
left a comment
There was a problem hiding this comment.
Thanks, exitcting!
Left a few comments/questions.
I think we should add packetization here as well (on the bridge created after Gate node).
src/pipeline/Pipeline.cpp
Outdated
| if(buildInternalQueue) { | ||
| // Starts pipeline, go through all nodes and start them | ||
| for(const auto& node : getAllNodes()) { | ||
| if(node->runOnHost()) { | ||
| node->buildInternalQueues(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need this internal queues?
Linking subnodes together without using createOutputQueue() doesn't work?
There was a problem hiding this comment.
I have a subnode, and I would like to create subnode->output.createOutputQueue() inside the parent node.
The problem is that if I use :
- parent::buildInternal(){subnode->output.createOutputQueue();}
The parent on the subnode is uninitialized and points to null.
- parent::start(){subnode->output.createOutputQueue();}
The pipeline is already build so I can not create the queue anymore.
If I shift the start() before the pipeline is build it works, but it could cause some issues with other pipelines.
There was a problem hiding this comment.
I have changed it to buildStage1, and it does the same.
There was a problem hiding this comment.
Could we use the "conventional" way and add inputs to the parent node and then do a normal link between the subnode and the parent node?
The why for my ask is mostly that createOutputQueue() approach won't work if we later mate this node device runnable on RVC4.
There was a problem hiding this comment.
Also - pipeline graph will show the structure much nicer in that case as the information gets lost with createOutputQueue() (the information on how the internal connections look like)
There was a problem hiding this comment.
I did not realize this option. Fixed here: 9041e3a67 Replace createOutput/InputQueue with linking
| auto outputCameraLeft = cameraLeft->requestIspOutput(20); | ||
| auto outputCameraRight = cameraRight->requestIspOutput(20); |
There was a problem hiding this comment.
Any specific reason to go with 20?
I think going with "default" makes more sense here, to avoid having troubles syncing.
There was a problem hiding this comment.
changed here: f94689e Set fps in ISP to default
|
I’ve identified a bottleneck in the current packetization logic. If the transmission time exceeds the message arrival rate, buffer overflows occur, leading to message loss. This is unintended behavior. |
95fcff9 to
c0a8b3a
Compare
Not sure I understand, XLink streams internally are blocking, where does the buffer overflow happen? Can you elaborate on which buffer overflows? |
If you send messages slower than they are coming, you have a problem (on the XLlink side, the input is not blocking, or on the sender side if it is blocking). Because of that, I think that the FPS should not be set in the XLinkOut stream. You can simply set it on the Gate side in this case, though. |
de12aa5 to
67272b5
Compare
src/pipeline/Pipeline.cpp
Outdated
| if(isBuild) return; | ||
| // start ---Add AutoCalibration block--- | ||
| std::unique_lock<std::mutex> lock(pipelineBuildMutex); |
There was a problem hiding this comment.
Was this intentional?
The idea before was to lock before isBuild is checked, to avoid a race condition here.
There was a problem hiding this comment.
I will move it before the check
There was a problem hiding this comment.
| if(isBuild) return; | ||
| // start ---Add AutoCalibration block--- | ||
| std::unique_lock<std::mutex> lock(pipelineBuildMutex); | ||
| auto autoCalibtationString = utility::getEnvAs<std::string>("DEPTHAI_AUTOCALIBRATION", ""); |
There was a problem hiding this comment.
We need to add this to the README.md
There was a problem hiding this comment.
e206ded to
c52f725
Compare
AutoCalibration node
Purpose
New AutoCalibration node
DEPTHAI_AUTOCALIBRATION=on.Remarks
Testing
tests/src/ondevice_tests/pipeline/node/auto_calibration_test.cpp