rivlb patch - detecting jogdial movement from script - General Discussion and Assistance

reyalp

14118

rivlb patch - detecting jogdial movement from script

« on: 25 / June / 2024, 02:52:27 »

Advertisements

Continued from @rivlb's post https://chdk.setepontos.com/index.php?topic=650.msg150290#msg150290 to avoid cluttering the patch thread with discussion:

Quote from: rivlb on 24 / June / 2024, 15:31:15

While experimenting with my SX150, I noticed that I could not get jog dial clicks in Lua script in a simple way like using is_key() or is_pressed() for example. So I tried to fix this problem. Now it is possible to get jog dial rotation event by using script function is_key() (tested in Lua script on SX150):

is_key("jog_left" ) - true if jog dial performed left click
is_key("jog_right") - true if jog dial performed left click

If you find this useful, it might be helpful to update the table here. Patch in attachment.

This has been a requested feature in the past (for example waterwingz post here and outslider here) but I'm not sure if making it a key click for script is the right way to go, or whether there would be undesirable side effects.

Some general thoughts and observations from poking around the related code:
If we treat jogdial as click, the behavior of existing scripts that use wait_click will be affected somewhat (for example, a script with "press any key to exit" logic would now exit on dial event), but I don't feel like this should be a problem for most scripts.

It's a bit odd to have a "key" for which is_pressed would always be false, but it doesn't seem like it should cause any real problem.

Currently, when a script is running, dial events are still seen by the Canon OS (on most ports, at least). This is in contrast to non-script "alt" mode, where correctly implemented ports block the jogdial (see jogdial_control, some ports probably don't implement it). rivlb's patch doesn't change this, and there are at least some scripts which expect this behavior, but IMO scripts that want to handle dial events would probably want to block them from OS in most cases.

The CHDK C UI code kind of treats the dials like a key click in that it treats the *DIAL_* constants as keyboard constants, but get_jogdial_direction is called at the point of use, if there are no key clicks (see gui_menu.c gui_menu_kbd_process for a typical example), so it isn't currently included in the "clicked" logic.

get_jogdial_direction is implemented per-platform and returns the direction of cumulative movement since the previous call. This means it should really only be called from kbd_task and at most once per kbd_task iteration.

Waterwingz post linked above suggested exposing the dial count to script, which would be annoying to do directly given the counters are only available in each platforms kbd.c. In practice though, it should be very rare for the dial to move more than once per ~10ms kbd_task iteration, so one could make a count based on get_jogdial_direction outside if desired.

A script might check keyboard state less frequently than kbd_task, in which case using is_key would only see one "click" per check, but this is consistent with how real keys are handled.

However we support jogdial in script, it would be good to include the secondary control dial for cameras that have them in a similar way. Currently, cameras that support a secondary dial report it from get_jogdial_direction, if the main dial has not moved. This seems a bit sketchy but I suppose it's a fairly safe assumption they won't both change in the same 10ms interval, or the user won't notice a missing step if they do.

Logged

Don't forget what the H stands for.

rivlb

11

Re: rivlb patch - detecting jogdial movement from script

« Reply #1 on: 26 / June / 2024, 04:43:02 »

Quote

Currently, when a script is running, dial events are still seen by the Canon OS (on most ports, at least).

Yes, you are right. And my patch didn't solve that moment. That's really annoying. Especially in liveview mode, when in Alt mode jog dial moves around CHDK menu and, at the same time, continues changing shooting parameters.

Here is my observations.

I noticed that jog dial generates kind of number of clicks in Canon OS internally. Like, after we rotate 5 clicks right some internal Canon OS counter increases by 5, and when we rotate 5 clicks left it decreases by 5. For example, on my SX150 in preview mode:

go to Alt mode,
open CHDK menu,
do 5 jog dial clicks right,
then do 6 j.d. clicks left,
exit Alt mode,
and as a result I see Canon OS action like I click j.d. left, i.e. move to one photo back.

I think var jogdial_stopped added to hook JogDial_task_my (in platform/<model>/sub/<rev>/boot.c) doesn't solve problem completely. It "freezes" that internal Canon OS jog dial counter only when CHDK Alt mode is active. That prevents jog dial processing by Canon OS, but doesn't clear that counter.

To completely control jog dial from CHDK, we need to fully intercept internal state of j.d. and control it. I am interesting in that and will try to find some solution.

Logged

Riva Lab // matrixFont - Raster fonts creating and editing

reyalp

14118

Re: rivlb patch - detecting jogdial movement from script

« Reply #2 on: 26 / June / 2024, 12:01:18 »

Quote from: rivlb on 26 / June / 2024, 04:43:02

Quote
Currently, when a script is running, dial events are still seen by the Canon OS (on most ports, at least).
Yes, you are right. And my patch didn't solve that moment. That's really annoying. Especially in liveview mode, when in Alt mode jog dial moves around CHDK menu and, at the same time, continues changing shooting parameters.

If it does in alt (without a script running), that's a bug in the port. For ports that aren't broken, we could extend the logic to block when script is running, but as mentioned, it affects existing scripts.

Quote

I think var jogdial_stopped added to hook JogDial_task_my (in platform/<model>/sub/<rev>/boot.c) doesn't solve problem completely. It "freezes" that internal Canon OS jog dial counter only when CHDK Alt mode is active. That prevents jog dial processing by Canon OS, but doesn't clear that counter.

This is because the implementation on sx150 is incomplete, see the commented out jog_position logic and see sx160 for an example with it implemented.

Logged

Don't forget what the H stands for.

rivlb

11

Re: rivlb patch - detecting jogdial movement from script

« Reply #3 on: 27 / June / 2024, 11:39:49 »

Thanks for your hint. I made some changes for the SX150 while looking at the SX160 implementation. Now everything works correctly, tested on the device. Updated patch is attached. But it needs some explanation:

file core/kbd_common.c:
fixed issue with Canon OS jog dial detection in script running mode, now Canon OS doesn't see jog dial activity when a script is running and Alt mode is active
files core/kbd_process.c, modules/script_key_funcs.c:
add support for is_key like described in initial post
files platform/sx150is/kbd.c, platform/sx150is/sub/100a/boot.c:
fix issue with incomplete jog dial implementation on SX150IS

As you can see, this patch is quite complex. So you might want to split it into smaller logical parts.

fix-few-problems-with-jogdial.patch (4.43 kB - downloaded 3 times.)

Logged

Riva Lab // matrixFont - Raster fonts creating and editing

reyalp

14118

Re: rivlb patch - detecting jogdial movement from script

« Reply #4 on: 29 / June / 2024, 01:56:04 »

Quote from: rivlb on 27 / June / 2024, 11:39:49

As you can see, this patch is quite complex. So you might want to split it into smaller logical parts.

The sx150 platform changes are clearly a bug fix, so I split that off and checked in as trunk 6275, stable 6276.

Quote

core/kbd_common.c:
fixed issue with Canon OS jog dial detection in script running mode, now Canon OS doesn't see jog dial activity when a script is running and Alt mode is active

For this, I'm a bit reluctant to just change the existing behavior, because scripts may expect it (my contae script is one example, though I don't think it's used much). Scripts could be updated to "pass through" by calling wheel_left or wheel_right, but it would be nice not to surprise users.

Some possible options:
- Add a function like set_jogdial_block(boolean), default to the existing behavior.
- Change the behavior depending on the @ chdk_version param, so existing scripts are unaffected until they explicitly say they support the new version

FWIW, I think

Code: [Select]

        if (!jogdial_stopped && camera_info.state.gui_mode) {

shouldn't need camera_info.state.gui_mode, that branch is only entered if kbd_process returned non-zero, meaning CHDK is handling keys instead of the Canon firmware.

The !camera_info.state.state_kbd_script_run check that was there before was an intentional exception for script. I don't know the history of it, despite my name being on that line in svn blame, it existed when I consolidated that code into kbd_common from all the different platform files.

Also, not a problem but just to note: state.state_kbd_script_run (checked in kbd_common) and state.gui_mode == GUI_MODE_SCRIPT (checked in kbd_process) are not exactly equivalent. kbd_script_run is non-zero when script is running, regardless of alt state, while the gui_mode becomes NONE if you exit alt while a script is running. In this case, it shouldn't matter since that code is only entered in the alt mode case anyway. The "script running while not in alt" state isn't really well defined, but I confirmed the jogdial is not blocked in this case with your code, which is consistent with the keyboard and seems like the correct behavior.

Logged

Don't forget what the H stands for.

rivlb

11

Re: rivlb patch - detecting jogdial movement from script

« Reply #5 on: 01 / July / 2024, 05:48:04 »

Quote from: reyalp on 29 / June / 2024, 01:56:04

Some possible options:
- Add a function like set_jogdial_block(boolean), default to the existing behavior.
- Change the behavior depending on the @ chdk_version param, so existing scripts are unaffected until they explicitly say they support the new version

Ok, I'd rather choose the second one. IMO, it is clearer solution. Try added patch, use it instead of the previous same part of the patch. But in this case we need to describe this behavior somewhere in the docs. Like, "in CHDK scripts with @ chdk_version 1.7.0 and above jog dial will be fully intercepted by CHDK" or similar thougths.

And how about

Quote

files core/kbd_process.c, modules/script_key_funcs.c:
add support for is_key like described in initial post

?
This is my main goal in all of this.

check-script-version-for-jog-dial.patch (0.75 kB - downloaded 4 times.)

Logged

Riva Lab // matrixFont - Raster fonts creating and editing

rivlb

11

Re: rivlb patch - detecting jogdial movement from script

« Reply #6 on: 24 / July / 2024, 11:51:32 »

So, what are you going to do?

Logged

Riva Lab // matrixFont - Raster fonts creating and editing

reyalp

14118

Re: rivlb patch - detecting jogdial movement from script

« Reply #7 on: 26 / July / 2024, 01:03:16 »

Quote from: rivlb on 24 / July / 2024, 11:51:32

So, what are you going to do?

Sorry, I've been distracted by other things. I was also kind of hoping other devs or users would have opinions, but things are pretty quiet here these days.

Anyway, thanks for bumping the thread, I'll try to test a bit and integrate it when I have some time.

I don't really love having the behavior depend on chdk_version but I guess it's OK. I don't love the other option I offered of controlling it with a function either so I'm not saying you picked the wrong one

I debated a bit whether it makes sense to treat the dial as a "key" at all, but on the balance I think it probably does:
- If it was accessed in a different way, using wait_click loops to drive UIs would get a bit weird
- We already "remote" and "no_key" which aren't really keys

We need to make sure press(), release(), click() and is_pressed() behave sensibly if the dial names are passed. If they are all ignored (which I think they will be, but haven't tested) that is probably good enough.

Logged

Don't forget what the H stands for.

Caefix

947
Sorry, busy deleting test shots...

Re: rivlb patch - detecting jogdial movement from script

« Reply #8 on: 26 / July / 2024, 09:45:03 »

Some new keywords might be an option...

https://chdk.setepontos.com/index.php?topic=14048.msg143541#msg143541

Logged

All lifetime is a loan from eternity.

reyalp

14118

Re: rivlb patch - detecting jogdial movement from script

« Reply #9 on: 03 / September / 2024, 21:25:49 »

I've finally committed code for this in trunk r6280, with some changes.

Overview:
For scripts with chdk_version set to 1.7 or higher, or started using PTP, jog dial and front dial inputs are treated as key clicks by wait_click() and is_key(), and are not seen by the Canon firmware. The key names used are "jogdial_left" "jogdial_right" "frontdial_left" and "frontdial_right".

If chdk_version is less than 1.7 or not present, the behavior is like previous versions of CHDK: Dial movements are not seen by CHDK script, and are seen by the Canon firmware.

Note the "front" dial is defined in CHDK code and may be what Canon calls a front dial (at the top of the grip) or a "control ring" around the lens, and not all cameras which have these have support in CHDK.

Changes from the posted patches
Bugs:
- The version check logic in post #5 was not correct. It didn't compile because versions.h was missing, and the logic in kbd_common.c kbd_update_key_state did not operate correctly, because the block/unblock logic was only entered when the state changed, and scripts are started from alt mode, when blocking is already active.
- script_version comes from the currently loaded script file, so behavior of PTP scripts would vary depending on what script the user had selected
I addressed the above by moving the version logic to script initialization, handling the file and PTP cases separately, and controlling the desired behavior with camera_info.state.script_dial_control. PTP scripts use the new behavior, and cannot switch to the old. I doubt many people use the physical dials with scripts started via PTP, but if someone really needs it, adding a function to control which behavior is used would be trivial.

Other changes:
- The "keys" are named jogdial_left, jogdial_right rather than jog_left and jog_right. This aligns with keyboard.h names, and makes supporting other dials more consistent
- For cameras with a "front" dial defined in CHDK, it is also exposed, with the names "frontdial_left" and "frontdial_right".
- Both blocking the dial events from the Canon firmware, and making them visible as clicks are controlled by the version logic, where in the posted patch, only the blocking was affected. This should make the behavior for chdk_version < 1.7 identical to the previous CHDK behavior.

A test script is included in CHDK/SCRIPTS/TEST/clicktst.lua. Note to test the "old" behavior you must edit thing chdk_version header.
Things I tested:
- With chdk_version >= 1.7, both Lua and ubasic see clicks for the dials, and prevent them from being seen by the Canon firmware.
- Script run using PTP behaves like chdk_version >= 1.7
- If chdk_version is missing or < 1.7, dial clicks are not seen by script and are seen by the Canon firmware
- Calling press and click with the dial "key" names is ignored, and is_pressed always returns false.
- Calling set_exit_key with a dial "key" is accepted, but the dial cannot be used to exit script. This is similar to no_key or remote, or any CHDK defined key which is not present in the camera keymap
- If you leave alt mode with a chdk_version 1.7 script running, dial clicks are seen by both the script and the Canon firmware. This is consistent with what happens for real keys

Tested cameras:
G7 X - jog dial and "control ring" which is defined as front dial in CHDK
SX730 - jog dial only
ELPH130 - no dials

Note M10 uses KBD_CUSTOM_UPDATE_KEY_STATE, so the jogdial logic from kbd_common.c kbd_update_key_state is duplicated there. I think this should be fine, but I haven't tested it. AFAIK, none of the other KBD_CUSTOM_UPDATE_KEY_STATE have dial support (except M100, which isn't in SVN).

Logged

Don't forget what the H stands for.