sofie-atem-connection icon indicating copy to clipboard operation
sofie-atem-connection copied to clipboard

Add Camera Control

Open haydendonald opened this issue 3 years ago • 2 comments

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) It adds camera control functionality

  • What is the current behavior? (You can also link to an open issue here) It currently works to a basic level (but is good enough) and has been tested with blackmagic broadcast cameras. There may still be bugs as i have not been able to fully test as i've run out of time for this project at the moment.

  • What is the new behavior (if this is a feature change)? Provides camera control of BlackMagic cameras over SDI. Normal operations are tested and working including CCU control.

  • Other information:

haydendonald avatar May 26 '21 23:05 haydendonald

This is cool! Looks like there’s some cruft from a merge conflict but the additions look solid.

meyer avatar May 26 '21 23:05 meyer

Hi, Long time no code... Didn't realise there was a merge conflict thought it was merged already 😄

I hope i have resolved the conflicts, i can't remember much about it.

Thanks ✌️

haydendonald avatar Oct 24 '22 05:10 haydendonald

Anything stopping this getting merged? Really need CCU for a project.

rohanwright avatar Feb 13 '23 02:02 rohanwright

My concern here is that while trying to parse the camera control data in another library, it seems to frequently have small changes in parameters. And as the official SDK doesn't strongly define what is possible with this, I find it very hard to reason about what is 'correct'.

I'm curious whether the unit tests still pass for this. Im concerned about what happens if the structure changes slightly in the future, and whether that could cause connection issues. If the tests pass, I suppose I don't have a strong objection to it once I've given it a proper read and some merge issues are fixed

Julusian avatar Feb 15 '23 13:02 Julusian

+1

gavalierm avatar Mar 28 '23 07:03 gavalierm

I've reworked how the command is implemented to make it generic https://github.com/nrkno/sofie-atem-connection/pull/144. The deserialization/serialization logic that this PR has in the command should be refactored into some util and used in the applyToState function instead.

I would also like to see some justification for ids used at various points, as some of them differ from what the arduino sdi shield use, so I am unsure which is correct.

Julusian avatar Apr 13 '23 09:04 Julusian

I've reworked how the command is implemented to make it generic https://github.com/nrkno/sofie-atem-connection/pull/144.

The deserialization/serialization logic that this PR has in the command should be refactored into some util and used in the applyToState function instead.

I would also like to see some justification for ids used at various points, as some of them differ from what the arduino sdi shield use, so I am unsure which is correct.

Awesome! Thanks for that. Justification would be, not sure, I can't remember much. I do know that they either came from shaarhoj (they removed the docs now by the looks though) or i found them via reverse engineering it. There is also the documentation from BlackMagic for the bluetooth protocol which i think has a similar structure as the ATEM.

Out of curiosity does the atem-sdi shield implement camera control?

haydendonald avatar Apr 14 '23 02:04 haydendonald

https://documents.blackmagicdesign.com/UserManuals/ShieldForArduinoManual.pdf?_v=1581580811000 Page 19 and onwards lists the official parameters. You are right, https://documents.blackmagicdesign.com/UserManuals/BlackmagicPocketCinemaCameraManual.pdf?_v=1661410810000 does also document this on page 158.

I did start on rewriting the parsing but when I noticed the differences I stopped unsure on how to proceed. Perhaps it would work to write a clean implementation based solely on that documentation and then for you or someone else with one of the cameras to give it a test and see what does and doesnt work?

Julusian avatar Apr 14 '23 08:04 Julusian

I have cameras to test it out. But i need some steps to reproduce the exact testcases.

gavalierm avatar Apr 14 '23 09:04 gavalierm

With #144 this can be done entirely in user code, perhaps written as a separate library. If someone writes this library, we will happily link to it from here to help other users find it. We are open to accepting a PR which adds better support for this camera control, but without a need for it ourselves or a way to perform any testing, we are hesitant. Our recommendation for now is to do this as a separate library, or directly in your codebase and once there is an implementation which is proven to work reliably we can reconsider including it here. With #144, hooking into this library is pretty simple:

const connection = new Atem()

// Receiving state updates
connection.on('receivedCommands', (commands) => {
    for (const command of commands) {
        if (command instanceof CameraControlUpdateCommand) {
            // TODO - update your state object here
        }
    }
})

// Sending a command
const cameraCommand = new CameraControlCommand(1, 2, 3, {}) // TODO populate this with real parameters
await connection.sendCommand(cameraCommand)

I think that in the past (in another library) I have seen that the data received can vary either across camera model, or across firmware versions, which reduces my confidence that an implementation will work reliably and consistently.
That said, I (personally not as part of nrkno) might be interested in implementing some of this or at least a subset of the state for Companion to make use of.

Julusian avatar Oct 19 '23 10:10 Julusian