Make it possible to "learn" camera state, to fill in values in camera actions

[jswalden]

Desired new functionality

PTZOptics cameras encode in presets the camera exposure settings. For my own use of my cameras, I'm looking into not using presets to avoid that. I'd dial in the exposure settings once before an event, then I'd move the camera between absolute locations without adjusting exposure. (The alternative would be reprogramming every preset for event-specific exposure settings every time, a mostly impractical approach.)

The sensible way to support this is using "learn" capabilities, combined with an action to move to (learned) absolute positions. (There's no such action yet, but that's trivial to add.)

Implementation

Implementing "learn" support requires actually parsing the response to a camera-position status inquiry command. Because commands/responses are sent/received over TCP, that requires continually parsing responses to all commands sent to the camera.

I've written an extensive patch stack that gradually, incrementally, converts what is currently just "send these raw bytes out the socket" to a structured representation of commands, their parameters, their expected responses, and even the expected parameters in those responses. That structured representation can be used to parse command responses (and to detect errors).

Patching preliminaries

The first four patches in this stack are just #30. From that PR, I specifically need:

1. format-on-commit support to maintain consistent formatting through this stack,
2. making the bugs URL in manifest.json correct so that error logging that directs the user to file a bug won't just direct him to /dev/null, and
3. making the code use ECMAScript modules to get a degree of typing support of imports.

So really, whoever looks at this should review #30 first.

(Regarding formatting and code cleanup, I'd have happily converted the entire module to TypeScript if I weren't concerned about a reviewer seeing it as a dealbreaker by impeding future contributions. Although at this point, I think I'd be willing to become module maintainer if that were the price to do it. I made do as best as possible with extensive, unenforced yet still highly helpful in vscode, jsdoc type comments. But really...this complexity needs enforced types.)

Patch stack details

The patch sequence is looooong, yes! But every incremental step through commits should work without breaking anything.

Here's a high-level description of what I implemented, that parallels the actual commits:

1. Define a Command class to encapsulate a command, its bytes, and parameters to it (including the options that encode them).
   • VISCA details are added to a new visca.js file.
   • Initially define only a ModuleDefinedCommand subclass, for commands created by the module (and any parameters in them should have been filled by the module and so be sanitized) that can be presumed not to contain syntax errors.
   • Parameters in the raw bytes of a command are filled using structured option representations in a new options.js source file. That representation specifies the id used for the option, the nibbles of the command that store it, and functions that convert parameter (the value encoded in the bytes) to choice (the value of options[id] and back.
2. Convert existing commands from just sending a raw byte sequence, to sending a Command.
   • This is broken into a dozen-plus very small individual patches so that it's easy to review them.
3. Convert the "Custom command" action to create and send a UserDefinedCommand.
4. Clean up how the pan/tilt speed storage is implemented, so it's not so intertwined with the action UI to change it and is much simpler to understand.
5. Convert the pan/tilt actions to use ModuleDefinedCommand like everything else.
6. Remove old send-raw-bytes functionality, now that everything deals in Commands.
7. Move the TCP socket property into a class that encapsulates it and all manipulations of it. (Manipulations that get increasingly complex after this point, that really shouldn't be in the minimal coding that defines an instance.)
8. Send the command, then asynchronously check the incoming response for errors.
   • Existing actions assume commands are fire-and-forget. Some camera commands have a response that consists of an immediate ACK, followed at considerable delay by Completion. So it's not acceptable to delay command sending just because we're waiting for a response.
9. Fix two existing actions that PTZOptics documentation says were miscoded.
   • There is no "Bright mode (manual)" exposure mode, and if you send the command to select it, at least my G3 camera responds with a syntax error.
   • The Preset Drive Speed action, which supposedly sets the speed of recalling a specific numbered preset, does not exist in that form in any docs I've been able to find. Rather, the command sets the recall speed for all presets, and it does not include any bytes dedicated to encoding that preset number. (Yes: this command always receives a Syntax Error response from the camera.)
10. Treat syntax errors in module-implemented commands as an actual error that fails the connection, so that they'll be found and reported faster in the future.
11. Extend Command to support defining the expected response from the camera.
    • For most commands, this is an immediate ACK and Completion.
    • For a couple others -- recalling presets, resetting pan/tilt to home position -- it's an immediate ACK, then a Completion once motion has stopped (or the command is superseded by another command sent to the camera).
    • For inquiries -- there are none in use at this point in the patch stack -- each one has a custom response format.
    • Some response messages are a fixed byte sequence. Some contain nibbles of varying value, so a response message is defined as a sequence of value bytes, a sequence of mask bytes, and the locations of half-byte parameters within them.
12. Implement "learn" support for Exposure Mode and Focus Mode, as proof of concept.
13. Extend the "Custom command" action to support input parameters, that the user can even fill in from variables if desired.
14. Bump the package.json and manifest.json major versions.
    • Other than removing the broken Preset Drive Speed option and "Bright mode (manual)" options, none of these patches should change observable functionality. Those (or at least the Bright mode removal -- simply ignoring a previously supplied option should be forwards-compatible, right?) would merit a minor version bump.
    • BUT, the degree of change overall IMO merits a major-version bump.

Next steps

Judging by the commit history, this module is relatively stagnant. Which is fine if it all basically works. But it might mean there's very little bandwidth for anyone to review such a large sequence of patches for issues, to suggest improvements, etc.

If that's the case, I dunno what happens. I've done lots of hands-on testing with Packet Sender, but nothing more. Notably, I don't know what's out there by way of TCP mocking of data reading/receiving/replying if I wanted to do any automated testing of this (although I did ask on Slack, to :cricket: ). At some point I could test my work in a production setting. But that's no substitute for careful reviewing, not for changes of this magnitude.

The "Custom command" action and response parameters

One final note: everything is ready to even support parameters in the command response, for the "Custom command" action. I even wrote a patch to sink parameter values in responses into variables. (It's not quite working yet, but I'm pretty sure it's 90% done.) This would be helpful in letting users use custom VISCA inquiries, not just whatever inquiries we hardcode into the module somehow.

But there's some complexity surrounding "where do you write those values", "when do those values become observable", and "how do you avoid a situation where multiple custom command actions might overwrite the same variables" (either variables specific to the module, or custom variables prescribed by the user). And setCustomVariableValue is deprecated.

I've dumped the patch into a branch so at least the work is saved somewhere. But I don't intend to do anything with it until there's a safe way to handle returning values from an action, that doesn't depend on some form of mutable global state.

[jswalden]

Hey @haakonnessjoen -- are you still maintaining this module? Just want to be sure this isn't just completely twisting in the wind with nobody ever intending to look at it at all. (Obviously it's a huge mass of change to drop all at once, even if almost every separate revision is bite-sized and incrementally landable, and basically all are as bite-sized as I could manage -- and that probably has something to do with it, too. 😅 )

If you're not around to review these changes, I dunno what the next step is. But at least then I can ask around about what my options are to move this forward.

[Gartom]

@jswalden , that's a rather massive update you have prepared. 🙂

I made some changes in this module long time ago and got it working well enough for the use cases we have in the church were we started to livestream during the pandemic.

We rely heavily on presets in our setup and typically do initial exposure and white balance settings for a service, before fine tuning the presets. I have not seen the issue that exposure settings are stored together with presets. What settings are you referring to?

I may be able to test this update later this spring, but as we have a "production setup" that we use every week, I need to find a good way to test this separately "on site".

[jswalden]

@jswalden , that's a rather massive update you have prepared. 🙂

Conceded. :-) I did everything I could to break it up into almost entirely bite-sized pieces, to ameliorate this. I've been on the other side of massive patches before! It's not fun, and the whole process bogs down far more than if the patch author makes extra effort to split it up.

We rely heavily on presets in our setup and typically do initial exposure and white balance settings for a service, before fine tuning the presets. I have not seen the issue that exposure settings are stored together with presets. What settings are you referring to?

According to the Move SE/Move 4K/Link 4K manual, page 30 (31 of 78), presets include image settings. Are you using specifically a PTZOptics camera? This module will probably largely work with other cameras by other manufacturers, but there are no guarantees of the VISCA command set being compatibly supported with them.

If you have already attempted setting presets and noticed the image settings changing when moving the camera or switching between presets, please read through the instructions below.

1. Lighting: Before adjusting the camera’s settings and saving presets, it is extremely important that you are satisfied with the lighting in the area you plan to operate the camera. (Tip: The easiest lighting to work with, is often referred to as “flat lighting”, meaning the lighting is as evenly dispered as possible thoughout the scene.)
2. Web UI: Once you have determined that the lighting setup is complete, type the camera’s IP address into your web browser to open up the camera’s web UI. If you are not familiar with how to do this, please see the Web UI section on page 29.
3. Default: We recommend setting all of the camera’s image settings, exposure settings, color settings, and focus settings to default before setting up presets. The default settings are shown on page 30 and page 31. (Note: When you save a preset, not only are you saving the position the camera is in, but you are also saving all the image settings it had at that exact time. When panning, tilting, and zooming the camera, all image settings will stay set to their last applied/saved values.)

Or perhaps you have an older PTZOptics camera? Doing a bit of quick searching now, it looks like this may be an active change from the past, where presets didn't include most image settings. Or at least that's what this discussion post seems to indicate.

If you're using an older camera, you wouldn't need this capability for the specific use case I lay out. But you would benefit from these patches enabling "Learn" capabilities for all sorts of commands that don't exist right now, to configure specific image/exposure/etc. settings without having to just guess what the right parameter values for them would be.

[jswalden]

@Gartom As for testing...shouldn't you just be able to slot in a local modules folder that contains a copy of the repository? The repo copy will be loaded preferentially to the built-in module version, and you shouldn't have to make any alterations to your setup.

I asked on Slack (beyond the freely-available history at this point, because it was before I posted these patches!) and nobody offered any suggestions, but I'd love to have some sort of automated testing mechanism that could be used to test this -- if someone could suggest some sort of TCP socket-mocking mechanism or something, that could be used to run it. For this, it should be something like expected TCP bytes written to socket, coupled with bytes the mock-TCP-socket would return in response to those expected bytes, or so.

I tested the early patches against my cameras, but past a certain point I mostly just checked the bytes sent/received using Packet Sender -- because from the module point of view the TCP traffic is the expected behaviors/characteristics of this module, not of a camera ultimately controlled by it.

[Gartom]

Or perhaps you have an older PTZOptics camera? Doing a bit of quick searching now, it looks like this may be an active change from the past, where presets didn't include most image settings. Or at least that's what this discussion post seems to indicate.

Yes, we have two PT20X-SDI-GY-G2 cameras, and it was a long time since I updated the FW. I haven't really updated neither the FW or the Companion setup since we got our setup stable enough for production. We have a rather competent setup with a lot of functionality that is "just working". The version of Companion we use is 2.2.1, and it has been "good enough" for us. We see some quirks that show up really seldom and we have workarounds for them.

As for testing...shouldn't you just be able to slot in a local modules folder that contains a copy of the repository? The repo copy will be loaded preferentially to the built-in module version, and you shouldn't have to make any alterations to your setup.

As mentioned above, it would probably not be that easy. :-) I am not sure that I want to update the FW in the cameras if we get the behavior with all image parameters stored. We typically only want the position and focus stored in a preset.

The reason I checked in here now is that I started to prepare an update to support speed for focus and zoom long time ago (see https://github.com/Gartom/companion-module-ptzoptics-visca/tree/Speed-settings). I considered it not important enough to implement and it was difficult to get previous implementations merged so I decided not to make the effort at that time, but thought again about the possibility to include it the other day.

I need to check some data on our setup to see what we use and look into if I e.g. want to update FW in the cameras or not.

[Gartom]

I skimmed through all the commits and it is really a massive rewrite of the module. The changes makes the module more complex and structured at the same time, but I like it.

I did not spend enough time to understand the "learning" part of the update, though, so I don't really understand the impact of it. @jswalden can you elaborate a bit about this?

[jswalden]

The changes makes the module more complex and structured at the same time, but I like it.

@Gartom And yet, also simpler. :-) The definitions of the byte patterns of commands/responses are all concise few-line beasts now, rather than defining the interspersing of options converted to byte patterns manually at every site. The sending of commands, the processing of response bytes, and their parsing into response parameters is all defined well away from the individual commands themselves. You can look at the entirety of the bytes sent and received in commands.js, separate from the goo of action definition.

In the latest API spec for PT20X-SDI-xx-G2, the "Bright mode (manual)" option is still available (although I don't know if it returns an error).

That's...semi-unfortunate. I guess we can't have as much rigor in what is accepted, in order to detect interaction bugs (that might wedge the connection) at earliest convenience.

(Every revision up to the point of making syntax errors fatal should be still in good working order, tho, as far as reviewing goes. Like I said, I very intentionally made this incrementally landable so concerns about later parts don't have to hold up earlier ones!)

Or we could do what many modules do and require selecting the device model you're working with. Then that model is used to restrict the choices exposed and the actions that are possible. But that probably isn't an ideal option if (likely) no one has access to all the necessary camera models and firmware updates (and just as important, time/resources to check compatibility across them).

If my memory is accurate, I can revert the extra rigor and leave invocations of such stuff to just log an error and at least pretend to keep moving on. Call it "user error", wash our hands of things.

I did not spend enough time to understand the "learning" part of the update, though, so I don't really understand the impact of it. @jswalden can you elaborate a bit about this?

The vast bulk is getting a systematic way of encoding commands and their expected responses. Once you have every command encoded with its expected response so that you're constantly decoding every current response from the camera, you can send new command/responses corresponding to VISCA inquiries to discover camera state.

The "learning" part in these patches is merely the cherry on top. (The actual actions with "learn" support that I'll want to add, I'm pushing into separate issues/PRs. This patch set is super-big already!) Rather than filling in the options within a command by hand (perhaps by just guessing numbers), you can learn the option values from the camera itself (having finely tuned them using a nice, dedicated control surface like a PT-JOY-G4). Then learn the exact values from the camera using a VISCA inquiry and processing its response, then return them as the learned option values.

There are existing options to set the exposure mode and to set the focus mode. Presently you just select from a dropdown. But with "learn" support, you can set the modes on a PT-JOY-G4 using dedicated buttons, then learn them from the camera. This is debatably easier than picking a dropdown option, executing the action, then verifying you like what you picked.

This is far from groundbreaking for those two modes. Selecting a mode on a control pad is similar trouble to picking from a dropdown.

But for, say, moving to a specific pan/tilt setting (using an action I haven't implemented yet)? It's far more useful. Imagine you want to move to, say, a position 17° left of home, 10° upward from level. There's a VISCA command to move to an absolute position:

Pan Tilt:Pan Tilt Drive:AbsolutePosition 81 01 06 02 vv ww 0y 0y 0y 0y 0z 0z 0z 0z FF vv: Pan speed 0x01 (low speed) ~ 0x18 (high speed), ww: Tilt speed 0x01 (low speed) ~ 0x14 (high speed), yyyy: Pan Position zzzz: Tilt Position

What yyyy/zzzz do 17° left/10° upward correspond to? Who knows. You'd be forced to enter in numbers, run the action, adjust them to a better guess, repeat til you found your final setting.

...or instead you could use a joystick to find the exact setting you want, then use a learn callback that sends this inquiry to get the exact number, then return them as learned options:

Pan Tilt Inquiry:Pan & Tilt Position Command: 81 09 06 12 FF Return: 90 50 0w 0w 0w 0w 0z 0z 0z 0z FF (wwww: Pan Position, zzzz: Tilt Position)

For focus/exposure modes, learning is kind of a novelty. For pan/tilt positions, learning is far easier than guess-and-test.

[Gartom]

Ok, thanks @jswalden , then I think I understand the "learn" function better. Is it basically a possibility to read back the settings from the camera?

Our workflow is that we have 32 defined "base" presets for each PTZ camera that we use and fine tune before each service. In practice, there are two stream deck pages for each PTZ with 16 presets and some other camera control buttons were you easily navigate the PTZ to get the framing you want and store that in a preset. When starting from "scratch" a Sunday morning before a service, the settings are typically OK, but there will always be fine tuning. One thing that often will differ is the lighting as we have a lot of natural light in the church, which means that we adjust white balance and exposure based on that. If we would need to update all presets for the "light of the day" every time, that would be painful.

[Gartom]

I checked our setup today and it seems that we have FW 6.3.04 in our PTZ cameras, and the latest FW version for PT20X-SDI/NDI-xx is 6.3.22 according to https://ptzoptics.com/firmware-changelog/

For FW version 6.3.20 there is a note that says "Added the ability to save unique OSD settings per preset". It is not clear for me if this is an optional ability or if it will always be saved to a preset. Do you know that, @jswalden?

[jswalden]

Ok, thanks @jswalden , then I think I understand the "learn" function better. Is it basically a possibility to read back the settings from the camera?

Yes. More generally it's for learning state from any kind of device. The wiki discusses the "learn" callback outside the context of any specific device.

When starting from "scratch" a Sunday morning before a service, the settings are typically OK, but there will always be fine tuning. One thing that often will differ is the lighting as we have a lot of natural light in the church, which means that we adjust white balance and exposure based on that. If we would need to update all presets for the "light of the day" every time, that would be painful.

You could also define a not-visible button that sets WB/exposure, before each service configure the options it sets by learning from the camera, then have every button include an action to push that button. (This does require you only use Companion buttons to call presets. But then, some subset of vMix/ProPresenter/etc. eschew using camera presets probably partly for this reason, so there's precedent for this.)

In my case...honestly the WB/exposure we have is passable enough to not totally embarrass me. (Although using the same settings for mornings and for the occasional evening service is super-dodgy.) :-) But I do want to fix it, and letting Companion learn those settings is a useful step toward addressing the problem.

[jswalden]

For FW version 6.3.20 there is a note that says "Added the ability to save unique OSD settings per preset". It is not clear for me if this is an optional ability or if it will always be saved to a preset. Do you know that, @jswalden?

The comments on the PTZOptics forum post I mentioned earlier suggest this is an "always", as I read it. But that's the extent of my actual knowledge here.

[jswalden]

Okay, removed the strict syntax error checking. I left a comment in noting why the checking can't be stricter, at least for now. Perhaps at some point models will be well tested enough to consider reimplementing stricter support again.

That said, the Preset Drive Speed option is still broken as currently coded, so some sort of followup issue will be needed to change it...

[jswalden]

@Gartom So this module has little enough review bandwidth, that they just gave me maintainer access so I can make changes myself. Unfortunately this means anything I write and land, that I can't find someone to review anyway, won't have other eyes on it to suggest better approaches or notice problems.

I know you said you did a skim over the changes -- any chance I could get you to do a look-over of this at any deeper level, or asking questions about anything that's not immediately clear or seems too "clever" or anything? Basically anything you can offer would help here, because right now I've got nothing lined up. If not, I'll probably start landing this (or maybe partition it into individual issue/PR pairs for landing) within a week or so, so I can move on to other changes I'd like to make.

[ploveman]

I won't pretend I follow all the changes @jswalden that you're proposing. All for more maintainable code. I was working on a simple PR to take preset from a variable. My only question/concern, as primarily a user of the module, is whether this could introduce any performance impact trying to do the learning - may be totally off base/unfounded? Should there be a setting to "turn on learning" to reduce (off by default)? not sure any risk of getting/processing response vs the simple socket send before. Also, is there any incompatibility with older cameras? I don't know if any of the response codes change across time too?

[jswalden]

@ploveman The performance impact of not merely firing off bytes into a socket and ignoring responses, but processing them and handling them, is negligible. (Even when all of this is being done in JavaScript.) Particularly when you consider that you're going to be manipulating cameras mostly on human time scales, sending one or a few commands per button press typically.

It's worth clarifying that the "learn" aspects of this only happen if you press a "Learn" button in an action equipped with such support -- which is useful when programming Companion, not when actually using a fully constructed configuration of it. During normal Companion use, you'd never press a "Learn" button.

Changes in camera API implementation across time are at least possible. We seem to have identified a couple of them here already: bright mode, preset drive speeds for two. At worst, the custom command action allows an affected user to work around that. But fundamentally, there's no avoiding that concern if PTZOptics can change things underneath us: in this set of fixes, or in any others. One solution would be to add a connection option to select the particular camera model (and maybe firmware?) in use, tailoring command expectations accordingly. But this seems like a bridge to cross when we get to it. I rather doubt much of this module was written with access to multiple camera models and multiple firmware versions, and that's the only way you really can negate this concern -- so these changes don't really change the risk calculus.

[ploveman]

@jswalden I wish I could be more help to you. js is def one of the languages I haven't done much with (mostly learning b/c of companion). Thanks for the clarifications.

[jswalden]

Okay, I landed all the changes here.

I was going to spin up a v3.0.0-rc1 release for this, and got a ways into writing up the release notes/etc. for it...and realized my changes might "break" users' custom commands.

The existing custom command action takes as option just the byte sequence sent. But after these changes, custom commands must have their expected response specified -- which means I should probably write (at least minimally) upgrade scripting that adds the standard response (ACK followed by Completion) as the expected response to existing Custom command actions that might be in the user's settings.

In principle someone could send a custom command that expects something other than that, so there's no perfect way to auto-upgrade settings. But simply expecting ACK/Completion probably works for most custom commands.

People can and should feel free to test out these changes, even if there's no release spun up yet -- with caveat that they'll have to manually fix their Custom command actions. But I'm not going to create any sort of prerelease or such until I get the upgrade path working correctly.

[jswalden]

I added "Custom command" action upgrades to add all the new options in https://github.com/bitfocus/companion-module-ptzoptics-visca/commit/3660515dcd701e32f3c71bc4a95a45cc7ea5e2c4. Will manually create/tag an rc1 release shortly.