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

Indi pentax #936

Merged
merged 9 commits into from
Jul 13, 2024
Merged

Indi pentax #936

merged 9 commits into from
Jul 13, 2024

Conversation

Graf2242
Copy link
Contributor

@Graf2242 Graf2242 commented Jul 2, 2024

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

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

@knro
Copy link
Collaborator

knro commented Jul 3, 2024

Thank you! @karlrees Can you please review this?

@Graf2242
Copy link
Contributor Author

Graf2242 commented Jul 3, 2024

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
Copy link
Collaborator

knro commented Jul 3, 2024

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

@Graf2242
Copy link
Contributor Author

Graf2242 commented Jul 3, 2024

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

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

@karlrees
Copy link
Contributor

karlrees commented Jul 4, 2024

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
Copy link
Contributor Author

Graf2242 commented Jul 4, 2024

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
Copy link
Contributor Author

Graf2242 commented Jul 4, 2024

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)

Copy link
Contributor

@karlrees karlrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commits do not cause any regression for me, so I am okay with them. They do not appear to fix the problem on any of the Raspberry Pi 5 installations I have tested, so there is probably more to the issue, but if they help some people, I have no problem with them.

@karlrees
Copy link
Contributor

karlrees commented Jul 7, 2024

@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
Copy link
Contributor Author

Graf2242 commented Jul 8, 2024

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
Copy link
Collaborator

knro commented Jul 10, 2024

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

@Graf2242
Copy link
Contributor Author

Graf2242 commented Jul 11, 2024

So, I did merged our fixes together:

  • throw away my "Bad descriptor" fix as karlrees's works for me too and it's definitely looks better (thank you!)
  • merge other fixes together
  • edit debian changelogs (again: did it first time. Please check if I did it right)

@Graf2242
Copy link
Contributor Author

[ 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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@knro knro merged commit 2086287 into indilib:master Jul 13, 2024
2 of 4 checks passed
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.

3 participants