indilib / indi-3rdparty

INDI 3rd Party drivers repository
https://www.indilib.org/devices.html
GNU Lesser General Public License v2.1
123 stars 208 forks source link

Indi pentax #936

Closed Graf2242 closed 1 month ago

Graf2242 commented 1 month ago

There are two fixes to indi-pentax driver for k20d:

To be fair, there are two more fixes need for k20d to work:

knro commented 1 month ago

Thank you! @karlrees Can you please review this?

Graf2242 commented 1 month ago

Also had to up cmake_minimum_required to 3.19 otherwise I got this warning. It's almost 4 years old, shouldn't be a problem I hope.

CMake Warning (dev) at CMakeLists.txt:51 (find_program):
  Policy CMP0109 is not set: find_program() requires permission to execute
  but not to read.  Run "cmake --help-policy CMP0109" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.

  The file

   /usr/bin/sudo

  is executable but not readable.  CMake is ignoring it for compatibility.
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:51 (find_program):
  Policy CMP0109 is not set: find_program() requires permission to execute
  but not to read.  Run "cmake --help-policy CMP0109" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.

  The file

    /usr/bin/sudo

  is executable but not readable.  CMake is ignoring it for compatibility.
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning at CMakeLists.txt:53 (message):
  sudo must be available on the system in order to set permissions for the
  driver
knro commented 1 month ago

Can you please bump the version and update the corresponding Debian changelog?

Graf2242 commented 1 month ago

Can you please bump the version and update the corresponding Debian changelog?

Done. Never did it before, tbh. Hope it's right

karlrees commented 1 month ago

I had worked out a supposed fix to the bad file descriptor issue as well, though my solution was different. My solution worked on some platforms (e.g., my Astroberry install), but not Ubuntu 23.10 or 24.04 with the Raspberry Pi 5. Due to lack of time, I ended up dropping the issue for a while. Anyway, I just tested your solution on Ubuntu 24.04 with the Raspberry Pi 5, and the error remains for me. What platform were you working on?

Graf2242 commented 1 month ago

I have only AMD64 Gentoo for now and checked fix on Arch VM also. Will try to install ubuntu 24.04 VM today and check this out (and unfortunately still have no raspberry to test Astroberry)

Graf2242 commented 1 month ago

Well, it's work for me on Ubuntu 24.04, here are couple screenshots image image And one shot before fixes: image

Also looks like for ubuntu setcap fix for indi_pentax is not required, it's enough to update libpktriggercord only (but maybe it's some VM stuff)

karlrees commented 1 month ago

@knro I have a fix for all of my Raspberry Pi installations. It's in my fork (https://github.com/karlrees/indi-3rdparty). It's possible that there are two distinct issues and both Graf2242's fix and my fix are needed. I'm not too savvy on best development practices--should I be submitting my fixes as a separate pull request, or somehow combining them with this pull request?

(Full disclosure, I also made a couple of other changes to the driver to support DNG files better, possibly support K3-III, and sync the pktriggercord source to its latest version).

Graf2242 commented 1 month ago

should I be submitting my fixes as a separate pull request, or somehow combining them with this pull request?

I'm totally fine with both ways. Probably at least we should merge both fixes for Debian single release. Better ask @knro for how to do it better (I can cherry-pick karlrees's fixes in my branch, for example)

knro commented 1 month ago

@Graf2242 If you can cherry pick @karlrees fixes that would great!

Graf2242 commented 1 month ago

So, I did merged our fixes together:

Graf2242 commented 1 month ago
[ 63%] Building C object libpktriggercord/CMakeFiles/pktriggercord.dir/libpktriggercord.c.o
/__w/indi-3rdparty/indi-3rdparty/build/indi-3rdparty-libs/libpktriggercord/libpktriggercord.c:70:28: error: 'longopts' defined but not used [-Werror=unused-const-variable=]
   70 | static struct option const longopts[] = {
      |                            ^~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [libpktriggercord/CMakeFiles/pktriggercord.dir/build.make:76: libpktriggercord/CMakeFiles/pktriggercord.dir/libpktriggercord.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1054: libpktriggercord/CMakeFiles/pktriggercord.dir/all] Error 2

Well, I'll comment out this again tomorrow if no one stops me

karlrees commented 1 month ago

@Graf2242

The version needs to be changed in the CMakeLists.txt files for both indi_pentax (line 10, INDI_PENTAX_VERSION_MINOR) and libpktriggercord (line 4, PK_VERSION) as well. Looks like I changed these before without updating the DEBIAN changelog. To be consistent, we probably should bump the version for indi_pentax to 1.2, and the version for libpktriggercord to 85.03.

Graf2242 commented 1 month ago

To be consistent, we probably should bump the version for indi_pentax to 1.2, and the version for libpktriggercord to 85.03.

Yep, good point. Done