Add ISP 3A camera controls #9
Conversation
Address review comments.
| std::string cmdfull = CameraControl::createCommandStr(camera_id, command_id, extra_args); | ||
| StreamData cmd_data; | ||
| cmd_data.packet_number = 0; | ||
| cmd_data.data = (void*)cmdfull.c_str(); |
There was a problem hiding this comment.
Having the nice structure for the CameraControl, I'd opt for serialization (either memcpying or rather json -> msgpack / bson) and then the device side can use that data in any way desired - which also hides that from host as it doesn't really need to know.
There was a problem hiding this comment.
Also is extra args actually a "string" or would std::vector<std::uint8_t> be more suitable?
There was a problem hiding this comment.
Yeah, extra_args is expected to be a string, space separated integers. I was going this way as there is already a camera-command string parser implemented on device side.
At some point we also need to merge the CaptureMetadata functionality with this one. Should we go like there? But need then to create separate functions for each command.
There was a problem hiding this comment.
If this CaptureMetadata is subset of ISP 3A then do integrate it.
One issue with just having these extra args there is that they are meaningless to users and they must seek description of this.
Maybe in core create another class which actually knows how to deal with this data, which is then exposed to users.
Eg. CameraControl in depthai-shared is only a means of describing data which is transferred between host & device and then in depthai-core create meaningful functions which use CameraControl as a means of sending type-safe commands&data
If you go about integrating CaptureMetadata, it will be an example of the need of having this extra layer IMO
There was a problem hiding this comment.
Thanks both! Agreed good call to reduce support load by making these self-descriptive/discoverable.
So what I'm thinking is we add these improvements to the roadmap to do after we get subpixel (luxonis/depthai#163), LR-Check-median(luxonis/depthai#219), Extended Disparity(luxonis/depthai#163), bilateral support(luxonis/depthai#215) released (and we're thinking, direct to Gen2).
The why is we are getting increasing pressure to get these out from a variety of customers, and the ISP-controls capability has already served the purposes of the two customers who needed it (in fact they don't need this power, they just needed EV compensation control on grayscale only).
Thoughts?
Depends on luxonis/depthai-shared#5