Skip to content

Commit

Permalink
Fix exposure mode handling bugs.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sh0
sh0 committed Aug 10, 2020
1 parent 87ecc1d commit 7c98a8e
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 59 deletions.
6 changes: 3 additions & 3 deletions pktriggercord-cli.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -946,12 +946,12 @@ int main(int argc, char **argv) {
DPRINT("shutter_speed.nom=%d\n", shutter_speed.nom);
DPRINT("shutter_speed.denom=%d\n", shutter_speed.denom);

if (shutter_speed.nom <= 0 || (shutter_speed.nom > 30 && status.exposure_mode != PSLR_GUI_EXPOSURE_MODE_B ) || shutter_speed.denom <= 0 || shutter_speed.denom > pslr_get_model_fastest_shutter_speed(camhandle)) {
if (shutter_speed.nom <= 0 || (shutter_speed.nom > 30 && status.exposure_mode != PSLR_EXPOSURE_MODE_B && status.exposure_mode != PSLR_EXPOSURE_MODE_B_OFFAUTO) || shutter_speed.denom <= 0 || shutter_speed.denom > pslr_get_model_fastest_shutter_speed(camhandle)) {
warning_message("%s: Invalid shutter speed value.\n", argv[0]);
}

pslr_set_shutter(camhandle, shutter_speed);
} else if ( status.exposure_mode == PSLR_GUI_EXPOSURE_MODE_B ) {
} else if ( status.exposure_mode == PSLR_EXPOSURE_MODE_B || status.exposure_mode == PSLR_EXPOSURE_MODE_B_OFFAUTO ) {
warning_message("%s: Shutter speed not specified in Bulb mode. Using 30s.\n", argv[0]);
shutter_speed.nom = 30;
shutter_speed.denom = 1;
Expand Down Expand Up @@ -1117,7 +1117,7 @@ int main(int argc, char **argv) {
printf("Taking picture %d/%d\n", frameNo+1, frames);
fflush(stdout);
}
if ( status.exposure_mode == PSLR_GUI_EXPOSURE_MODE_B ) {
if ( status.exposure_mode == PSLR_EXPOSURE_MODE_B || status.exposure_mode == PSLR_EXPOSURE_MODE_B_OFFAUTO ) {
if (pslr_get_model_old_bulb_mode(camhandle)) {
bulb_old(camhandle, shutter_speed, prev_time);
} else {
Expand Down
14 changes: 9 additions & 5 deletions pktriggercord.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,8 @@ static void init_user_mode_combo(pslr_status *st_new, pslr_status *st_old) {
GtkWidget *pw = GW("user_mode_combo");
if (st_new) {
if (!st_old || st_old->exposure_mode != st_new->exposure_mode) {
gtk_combo_box_set_active(GTK_COMBO_BOX(pw), st_new->exposure_mode);
pslr_gui_exposure_mode_t mode = pslr_convert_exposure_mode_to_gui(st_new->exposure_mode);
gtk_combo_box_set_active(GTK_COMBO_BOX(pw), mode);
}
}
gtk_widget_set_sensitive(pw, st_new != NULL && st_new->user_mode_flag);
Expand Down Expand Up @@ -723,7 +724,7 @@ static void update_aperture_label(void) {

static void update_shutter_speed_widgets(void) {
gchar buf[256];
if (status_new && (status_new->exposure_mode == PSLR_GUI_EXPOSURE_MODE_B)) {
if (status_new && (status_new->exposure_mode == PSLR_EXPOSURE_MODE_B || status_new->exposure_mode == PSLR_EXPOSURE_MODE_B_OFFAUTO)) {
sprintf(buf, "BULB");
gtk_label_set_text(GTK_LABEL(GW("label_shutter")), buf);
/* inhibit shutter exposure slide */
Expand Down Expand Up @@ -1485,7 +1486,7 @@ G_MODULE_EXPORT void shutter_press(GtkAction *action) {
}
DPRINT("Shutter press.\n");
pslr_get_status(camhandle, &status);
if (status.exposure_mode == PSLR_GUI_EXPOSURE_MODE_B) {
if (status.exposure_mode == PSLR_EXPOSURE_MODE_B || status.exposure_mode == PSLR_EXPOSURE_MODE_B_OFFAUTO) {
GtkWidget * pw;
pw = GW("bulb_exp_value");
bulb_exp_str = gtk_entry_get_text(GTK_ENTRY(pw));
Expand Down Expand Up @@ -2304,13 +2305,16 @@ G_MODULE_EXPORT void file_format_combo_changed_cb(GtkAction *action, gpointer us
G_MODULE_EXPORT void user_mode_combo_changed_cb(GtkAction *action, gpointer user_data) {
DPRINT("user_mode_combo_changed_cb\n");
pslr_gui_exposure_mode_t val = gtk_combo_box_get_active(GTK_COMBO_BOX(GW("user_mode_combo")));
pslr_exposure_mode_t mode;
if (!status_new) {
return;
}
assert(val >= 0);
assert(val < PSLR_GUI_EXPOSURE_MODE_MAX);
if (status_new == NULL || val != status_new->exposure_mode ) {
pslr_set_exposure_mode(camhandle, val);

mode = pslr_convert_exposure_mode_from_gui(val);
if (status_new == NULL || mode != status_new->exposure_mode ) {
pslr_set_exposure_mode(camhandle, mode);
}
}

Expand Down
42 changes: 28 additions & 14 deletions pslr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,7 @@ user_file_format get_user_file_format( pslr_status *st ) {
}
}

// most of the cameras require this exposure mode conversion step
pslr_gui_exposure_mode_t exposure_mode_conversion( pslr_exposure_mode_t exp ) {
pslr_gui_exposure_mode_t pslr_convert_exposure_mode_to_gui( pslr_exposure_mode_t exp ) {
switch ( exp ) {

case PSLR_EXPOSURE_MODE_GREEN:
Expand Down Expand Up @@ -360,6 +359,33 @@ pslr_gui_exposure_mode_t exposure_mode_conversion( pslr_exposure_mode_t exp ) {
return 0;
}

pslr_exposure_mode_t pslr_convert_exposure_mode_from_gui( pslr_gui_exposure_mode_t exp ) {
switch ( exp ) {

case PSLR_GUI_EXPOSURE_MODE_GREEN:
return PSLR_EXPOSURE_MODE_GREEN;
case PSLR_GUI_EXPOSURE_MODE_P:
return PSLR_EXPOSURE_MODE_P;
case PSLR_GUI_EXPOSURE_MODE_SV:
return PSLR_EXPOSURE_MODE_SV;
case PSLR_GUI_EXPOSURE_MODE_TV:
return PSLR_EXPOSURE_MODE_TV;
case PSLR_GUI_EXPOSURE_MODE_AV:
return PSLR_EXPOSURE_MODE_AV;
case PSLR_GUI_EXPOSURE_MODE_TAV:
return PSLR_EXPOSURE_MODE_TAV;
case PSLR_GUI_EXPOSURE_MODE_M:
return PSLR_EXPOSURE_MODE_M;
case PSLR_GUI_EXPOSURE_MODE_B:
return PSLR_EXPOSURE_MODE_B;
case PSLR_GUI_EXPOSURE_MODE_X:
return PSLR_EXPOSURE_MODE_X;
case PSLR_GUI_EXPOSURE_MODE_MAX:
return PSLR_EXPOSURE_MODE_MAX;
}
return 0;
}

pslr_handle_t pslr_init( char *model, char *device ) {
FDTYPE fd;
char vendorId[20];
Expand Down Expand Up @@ -988,10 +1014,6 @@ int pslr_set_exposure_mode(pslr_handle_t h, pslr_exposure_mode_t mode) {
return PSLR_PARAM;
}

if ( p->model->need_exposure_mode_conversion ) {
mode = exposure_mode_conversion( mode );
}

return ipslr_handle_command_x18( p, true, X18_EXPOSURE_MODE, 2, 1, mode, 0);
}

Expand Down Expand Up @@ -1176,11 +1198,6 @@ bool pslr_get_model_has_jpeg_hue(pslr_handle_t h) {
return p->model->has_jpeg_hue;
}

bool pslr_get_model_need_exposure_conversion(pslr_handle_t h) {
ipslr_handle_t *p = (ipslr_handle_t *) h;
return p->model->need_exposure_mode_conversion;
}

int pslr_get_model_fastest_shutter_speed(pslr_handle_t h) {
ipslr_handle_t *p = (ipslr_handle_t *) h;
return p->model->fastest_shutter_speed;
Expand Down Expand Up @@ -1333,9 +1350,6 @@ static int ipslr_status_full(ipslr_handle_t *p, pslr_status *status) {
} else {
// everything OK
(*p->model->status_parser_function)(p, status);
if ( p->model->need_exposure_mode_conversion ) {
status->exposure_mode = exposure_mode_conversion( status->exposure_mode );
}
if ( p->model->bufmask_command ) {
uint32_t x, y;
int ret;
Expand Down
5 changes: 3 additions & 2 deletions pslr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ int pslr_get_model_extended_iso_max(pslr_handle_t h);
int *pslr_get_model_jpeg_resolutions(pslr_handle_t h);
bool pslr_get_model_only_limited(pslr_handle_t h);
bool pslr_get_model_has_jpeg_hue(pslr_handle_t h);
bool pslr_get_model_need_exposure_conversion(pslr_handle_t h);
pslr_jpeg_image_tone_t pslr_get_model_max_supported_image_tone(pslr_handle_t h);
bool pslr_get_model_has_settings_parser(pslr_handle_t h);
int pslr_get_model_af_point_num(pslr_handle_t h);
Expand All @@ -224,7 +223,9 @@ int pslr_write_setting_by_name(pslr_handle_t *h, char *name, uint32_t value);
bool pslr_has_setting_by_name(pslr_handle_t *h, char *name);
int pslr_read_settings(pslr_handle_t *h);

pslr_gui_exposure_mode_t exposure_mode_conversion( pslr_exposure_mode_t exp );
pslr_gui_exposure_mode_t pslr_convert_exposure_mode_to_gui( pslr_exposure_mode_t exp );
pslr_exposure_mode_t pslr_convert_exposure_mode_from_gui( pslr_gui_exposure_mode_t exp );

char *format_rational( pslr_rational_t rational, char * fmt );

int pslr_test( pslr_handle_t h, bool cmd9_wrap, int subcommand, int argnum, int arg1, int arg2, int arg3, int arg4);
Expand Down
Loading

0 comments on commit 7c98a8e

Please sign in to comment.