Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support for flying key 'Run to' (to keyframe and to infinite) #102

Merged
merged 9 commits into from
Feb 28, 2021

Conversation

endreszabo
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature added: Flying keys 'Run to' feature implemented (at least it's called this way in original GUI).

  • What is the current behavior? (You can also link to an open issue here)

There's no support for 'Run to'.

  • What is the new behavior (if this is a feature change)?

Flying key Run to Keyframe and Run to Infinite (in specific directions) are now supported. Keyframe size and location can't be edited right now.

  • Other information:

Be easy on me, I'm not a coder. I made this change against the master tree because the README.md did not instruct me to use another one. Also I'm absolutely not sure if using of this.flag in the send buffer is appropriate in this case. I'd add test and documentation if I'd find a close match to this code. I used tcpdump to capture traffic while using GUI. Also I checked the source of other ATEM control software for actual command parameters.

@endreszabo
Copy link
Contributor Author

Hmm, surprisingly, there were test cases for this RFlK ATEM command. Discovering them too late, it is not surprising that the argument names don't match my function names.

@Julusian
Copy link
Member

Thanks for the contribution

Yes, there are unused test cases for a lot of the commands as most of this library is based on https://github.com/LibAtem/LibAtem, and the testcases are generated from that. It has some tests comparing it to the official sdk, so by proxy that means the testcases should be pretty accurate, but could be a bit outdated

Unfortunately this does mean that you didn't need to dive into it with tcpdump to figure out the commands, I should write some contribution guidelines for future refence

I havent fully read through the code yet, as it has been a while and I need to remind myself about this codebase, but could you define enums in src/enums/index.ts for the keyframe and direction? You can base them on the ones at https://github.com/LibAtem/LibAtem/blob/27e1f7e3b6236eb1929f42b6b3cabd0575ee8652/LibAtem/Common/Id.cs#L110

@endreszabo
Copy link
Contributor Author

Ok, somewhy the first byte in command must be 0x02, or at least the second bit must be set to make the Run to Infinite work.

Also, I have no idea why all the tests fail. :) In the packet debug output I get the right bytes set and the commands also work on my Mini Pro.

@endreszabo
Copy link
Contributor Author

Thank you for your feedback. I copied over those two enum definitions. But I guess they should be referenced in the final .ts command file to be checked as valid argument values. Tests started to show up values different than all 0x00s. :)

@Julusian
Copy link
Member

@endreszabo I have taken a proper look at this and made a few tweaks to your change. You can follow along with the commits as I kept it in stages to make it clear what I was changing and why.

The tests were failing partly because you had assigned the keyframeId and direction properties on the class, as well as inside the properties object. The tests expect them to be in the properties object.
Additionally, as you pointed out, the naming between the test data and the class dont match up. This is fine, but needed the translation map to be updated.
Then finally, the test data was bad. You are right in that the first byte needs to be a 2 when doing a run to infinite, so I fixed up that data too (not expected for anyone outside the maintainers to do that)

Finally, I used the enums you added, and changed the api method you added to make it clearer on how to use it correctly (because the direction is only valid when the keyframe is 'infinite')

I am now happy with the changes, so shall merge them soon

@Julusian Julusian changed the title Added support for flying key 'Run to' (to keyframe and to infinite) feat: support for flying key 'Run to' (to keyframe and to infinite) Feb 28, 2021
@Julusian Julusian merged commit 7ccc292 into nrkno:master Feb 28, 2021
@Julusian
Copy link
Member

Julusian commented Mar 3, 2021

Sorry for the delay, this is published in v2.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants