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

Fix exposure mode handling bugs. #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sh0
Copy link
Contributor

@sh0 sh0 commented Aug 10, 2020

There are some weird things going on with pslr_exposure_mode_t and pslr_gui_exposure_mode_t.

My understanding is that there was a commit 582e03f7926314ee6fbec4f1b053e222d96cb6e0 (Wed May 29 18:04:27 2013 +0000) which added separate GUI exposure mode enum pslr_gui_exposure_mode_t, but the conversion between enums was forgotten in user_mode_combo_changed_cb and/or pslr_set_exposure_mode. Then recently this issue was discovered and commit f9e23edcf88bfc5ca356bbfce91ca085aef56e54 (Sun Mar 22 14:03:39 2020 +0100) tried to fix it by adding enum conversion to pslr_set_exposure_mode. However the conversion is done in the wrong direction.

To try to summarize, currently:

  • pktriggercord.c calls pslr_set_exposure_mode with pslr_gui_exposure_mode_t argument.
  • pktriggercord-cli.c calls pslr_set_exposure_mode with pslr_exposure_mode_t argument.
  • pslr.c function pslr_set_exposure_mode incorrectly converts pslr_exposure_mode_t to pslr_gui_exposure_mode_t. In CLI case this is incorrect as the enum should be left to pslr_exposure_mode_t and in GUI case the mode is already in pslr_gui_exposure_mode_t type so it does the conversion twice.

I think the whole mode switching code was buggy before need_exposure_mode_conversion flag was added for K-30 camera. This can also be seen from the issue #52 that something is wrong. So I think that the K-30 handling was probably not needed in the first place.

My experimental fix is as follows:

  • pslr.c keeps track of camera mode only using pslr_exposure_mode_t type. This is preferred over pslr_gui_exposure_mode_t as it has more entries and is more accurate.
  • CLI pktriggercord-cli.c code mostly operates with pslr_exposure_mode_t type except few PSLR_GUI_EXPOSURE_MODE_B compares which were changed.
  • GUI pktriggercord.c now converts between pslr_gui_exposure_mode_t and pslr_exposure_mode_t as needed.
  • pslr.c has new functions pslr_convert_exposure_mode_to_gui and pslr_convert_exposure_mode_from_gui instead of single exposure_mode_conversion function.
  • need_exposure_mode_conversion structure entry and pslr_get_model_need_exposure_conversion function are removed.

I don't have Pentax cameras so someone else would need to do tesing.

There are some weird things going on with `pslr_exposure_mode_t` and `pslr_gui_exposure_mode_t`.

My understanding is that there was a commit `582e03f7926314ee6fbec4f1b053e222d96cb6e0` (Wed May 29 18:04:27 2013 +0000) which added separate GUI exposure mode enum `pslr_gui_exposure_mode_t`, but the conversion between enums was forgotten in `user_mode_combo_changed_cb` and/or `pslr_set_exposure_mode`. Then recently this issue was discovered and commit `f9e23edcf88bfc5ca356bbfce91ca085aef56e54` (Sun Mar 22 14:03:39 2020 +0100) tried to fix it by adding enum conversion to `pslr_set_exposure_mode`. However the conversion is done in the wrong direction.

To try to summarize, currently:
* `pktriggercord.c` calls `pslr_set_exposure_mode` with `pslr_gui_exposure_mode_t` argument.
* `pktriggercord-cli.c` calls `pslr_set_exposure_mode` with `pslr_exposure_mode_t` argument.
* `pslr.c` function `pslr_set_exposure_mode` incorrectly converts `pslr_exposure_mode_t` to `pslr_gui_exposure_mode_t`. In CLI case this is incorrect as the enum should be left to `pslr_exposure_mode_t` and in GUI case the mode is already in `pslr_gui_exposure_mode_t` type so it does the conversion twice.

I think the whole mode switching code was buggy before `need_exposure_mode_conversion` flag was added for K-30 camera. This can also be seen from the issue asalamon74#52 that something is wrong. So I think that the K-30 handling was probably not needed in the first place.

My experimental fix is as follows:
* `pslr.c` keeps track of camera mode only using `pslr_exposure_mode_t` type. This is preferred over `pslr_gui_exposure_mode_t` as it has more entries and is more accurate.
* CLI `pktriggercord-cli.c` code mostly operates with `pslr_exposure_mode_t` type except few `PSLR_GUI_EXPOSURE_MODE_B` compares which were changed.
* GUI `pktriggercord.c` now converts between `pslr_gui_exposure_mode_t` and `pslr_exposure_mode_t` as needed.
* `pslr.c` has new functions `pslr_convert_exposure_mode_to_gui` and `pslr_convert_exposure_mode_from_gui` instead of single `exposure_mode_conversion` function.
* `need_exposure_mode_conversion` structure entry and `pslr_get_model_need_exposure_conversion` function are removed.
@asalamon74
Copy link
Owner

Thanks, I'll need more time to check this. This part of the code is a mess, definitely, it would be great to clean this up.

@asalamon74
Copy link
Owner

I checked a few things and the history of this fields:

The values of pslr_exposure_mode_t are rather similar to the PictureMode exif field ( https://exiftool.org/TagNames/Pentax.html ) the values of pslr_gui_exposure_mode_t more resembles the values printed on the camera.

So why was this conversion added by me when I added the support for K-x? I don't really remember, but probably I realised that when I set the camera to M mode I read the value 8 instead of the expected 6. Setting exposure values was not possible for K-x, so I couldn't check that part.

Why do we need this at all? I tested the exposure mode setting using K-70. It only works if I set the camera to user mode. Let's say I want to change to Sv mode. The value is 15 in pslr_exposure_mode_t and 2 in pslr_gui_exposure_mode_t. I have to send the value 2 to the camera (I turned off all the conversions for the test). The camera will change to Sv mode, and reading back will get me the value of 15. So it seems to me, the camera has different values for reading or writing this field (but why?).

So it means that pslr_set_exposure_mode should either get the gui value or convert the non-gui value before sending the value to the camera. It is clearly wrong that in case of the GUI we have a double conversion. It is also true that it's better to store the non-gui version values in pslr.c since it has more values.

@sh0
Copy link
Contributor Author

sh0 commented Nov 5, 2020

Thanks for taking a look. I'm also not quite sure what the best option is here.

I don't known how to handle all the user mode value conversions, but it feels to me that having only pslr_exposure_mode_t in pslr.c would at least simplify the API. Last time I looked, it seemed like libgphoto2 was also using the exposure mode settings incorrectly because it was mixing GUI and non-GUI enum values. So some clarity from the user-facing aspects would be nice.

For K-30 camera the issue #52 was somewhat promising that my patch allowed the exposure mode to be correctly recognized. I don't really remember all the details though.

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.

2 participants