OpenPHDGuiding / phd2

PHD2 Guiding
https://openphdguiding.org
BSD 3-Clause "New" or "Revised" License
245 stars 111 forks source link

Migrate to INDI v2.0.0+ #1089

Closed knro closed 11 months ago

knro commented 11 months ago

Migrate PHD2 to utilize INDI v2

agalasso commented 11 months ago

this looks great so far @knro ! I have PHD2 build/test environments setup for Mac, Windows and Linux -- please let me know if there is any testing you'd like me to do before we merge this.

knro commented 11 months ago

Thanks @agalasso I wasn't able to test this on Windows so far, so if you can that would be great. I tried a Windows VM but that didn't go as I planned so I ordered a MiniPC for Windows :D until I receive it, it would be great to know if there are any issues that can be resolved. On Linux it works great, but should probably get more testing.

pchev commented 11 months ago

I tested on Linux and it work fine by using my INDI 2.0.3 local install.

But using the bundled INDI fail because the file is a zip but thirdparty.cmake try to use a tar.gz

I have all the VM setup for testing on Windows but no build setup. @agalasso I am interested if you can build a Windows installer and send me for testing.

agalasso commented 11 months ago

build a Windows installer and send me for testing.

@pchev here you go: https://openphdguiding.org/phd2-2.6.11dev6-indi2-a-installer.exe

(I have not tested it at all yet, will try to test later today)

pchev commented 11 months ago

I test on a Windows 11 virtual machine, connect to a INDI server 2.0.3 running on Linux with the simulator drivers. Everything work as expected, camera, telescope, calibration, guiding and control panel.

bwdev01 commented 11 months ago

We've just gotten this comment in a long-running support thread: https://groups.google.com/g/open-phd-guiding/c/1rqgqY2Yx5s

The OP now claims that your PR will "fix" his problem although he has never tried using it. The problem is that the local sidereal time returned by the driver was wrong, as was the local time on the computer. When I questioned his assertion, he referred me to you and may have been parroting something you said. Is he just confused? If not, can you explain how a version change to the Indi framework could have anything to do with this and if it does, why it hasn't affected every Indi mount driver that's been used with PHD2. Thanks. Bruce

knro commented 11 months ago

INDI mount drivers do not report LST. PHD2 relies on INDI compiled with libnova to get LST. However, PHD2 was not compiled with libnova, so the value from libindi for LST was always zero. With this PR, PHD2 is required to use libnova when compiled against INDI and therefore the LST returned is always valid.

agalasso commented 11 months ago

@knro thanks for the PR! sorry for the delay merging it.. I have been traveling

bwdev01 commented 11 months ago

Thanks for the explanation, definitely a poor state of affairs. I’ve added a comment to the commit, I think there’s more clean-up that should be done.

From: Jasem Mutlaq @.> Sent: Saturday, August 26, 2023 10:21 PM To: OpenPHDGuiding/phd2 @.> Cc: bwdev01 @.>; Comment @.> Subject: Re: [OpenPHDGuiding/phd2] Migrate to INDI v2.0.0+ (PR #1089)

INDI mount drivers do not report LST. PHD2 relies on INDI compiled with libnova to get LST. However, PHD2 was not compiled with libnova, so the value from libindi for LST was always zero. With this PR, PHD2 is required to use libnova when compiled against INDI and therefore the LST returned is always valid.

— Reply to this email directly, view it on GitHub https://github.com/OpenPHDGuiding/phd2/pull/1089#issuecomment-1694573561 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDHSV2TCUTDXIN3UKL2B2DXXLKMVANCNFSM6AAAAAA3NDXFZA . You are receiving this because you commented. https://github.com/notifications/beacon/ADDHSV5MLI5U2TS2LT2QCU3XXLKMVA5CNFSM6AAAAAA3NDXFZCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTFAER7S.gif Message ID: @. @.> >

sn3ik commented 10 months ago

Hi all, I've been trying to compile this release for several days, but I get a lot of errors.

First: the thirdparty.cmake file tries to find a tar.gz archive, I changed to the zip file and it extracts fine.

Second: cmake throws errors because file paths are changed to INDI > 2 so you have to change all library paths

`configure_file( "${LIBINDI_DIRECTORY}/libs/indiclient/baseclient.h" "${libindi_root_config}/libs/indiclient/baseclient.h" COPY ONLY) set(indiclient_INC libs/indicore/indiapi.h libs/indicore/indidevapi.h libs/indicere/base64.h libs/indicere/lilxml.h libs/indicore/indicom.h libs/eventloop/eventloop.h libs/indibase/indidriver.h libs/indidevice/indibase.h libs/indidevice/indibasetypes.h libs/indidevice/baseddevice.h libs/indibase/defaultdevice.h libs/indibase/indiccd.h libs/indibase/indidetector.h libs/indibase/indifilterwheel.h libs/indibase/indifocuserinterface.h libs/indibase/indifocuser.h libs/indibase/inditelescope.h libs/indibase/indiguiderinterface.h libs/indibase/indifilterinterface.h libs/indidevice/property/indiproperty.h libs/indidevice/indistandardproperty.h libs/indibase/indidome.h libs/indibase/indigps.h libs/indibase/indilightboxinterface.h libs/indibase/indidustcapinterface.h libs/indibase/indiweather.h libs/indibase/indilogger.h libs/indibase/indicontroller.h libs/indibase/indiusbdevice.h libs/hid/hidapi.h

   libs/indibase/connectionplugins/connectioninterface.h
   libs/indibase/connectionplugins/connectionserial.h
   libs/indibase/connectionplugins/connectiontcp.h
 )

 # include_directories( ${CMAKE_CURRENT_BINARY_DIR})
 ####include_directories( ${LIBINDI_DIRECTORY})
 ####include_directories( ${LIBINDI_DIRECTORY}/libs)
 ####include_directories( ${LIBINDI_DIRECTORY}/libs/indibase)
 ####include_directories( ${ZLIB_INCLUDE_DIR})
 ####include_directories( ${CFITSIO_INCLUDE_DIR})

 # default for libindi client
 option(INDI_FAST_BLOB "Build INDI with Fast BLOB support" ON)

 set(indiclient_C_SRC
     ${LIBINDI_DIRECTORY}/libs/indicore/lilxml.cpp
     ${LIBINDI_DIRECTORY}/libs/indicore/base64.c
     ${LIBINDI_DIRECTORY}/libs/indicore/indicom.c)

 set(indiclient_CXX_SRC
     ${LIBINDI_DIRECTORY}/libs/indidevice/basedevice.cpp
     ${LIBINDI_DIRECTORY}/libs/indiclient/baseclient.cpp
     ${LIBINDI_DIRECTORY}/libs/indidevice/property/indiproperty.cpp
     ${LIBINDI_DIRECTORY}/libs/indidevice/indistandardproperty.cpp)`

with these modifications it is possible to generate the Makefile

Third: launching make still causes other errors pointing to the libraries, I tried changing the path of the files again but I was not able to solve all the problems

Fourth: you need to rename the indiapi.h.in file to indiapi.h

Has anyone encountered these problems?

Thanks and sorry for my bad english

agalasso commented 10 months ago

I'm not sure how we got into these compile issues as we did test builds on windows, mac and linux before we merged this. I am able to repro the compile errors on windows and Linux now, so I'm going to revert the merge of this pr into phd2master until we get these compile issues sorted out.

@knro would you mind putting up a new pr so we can fix the issues before re-mergig to phd2/master, thanks

knro commented 10 months ago

@agalasso The issue is with compiling the bundled INDI zip file which is quite complex under Linux/MacOS due to servers, tools, drivers, and libraries that must be installed. Can't we just rely on system installed INDI by default?

agalasso commented 10 months ago

I agree it makes sense to use the system-installed INDI libs for Linux builds -- let's do that.

For Windows we're pretty much forced to compile a bundled INDI zip file since virtually no Windows users will have INDI libs installed (and similar for Mac)

pchev commented 10 months ago

The problem is it not work on all current system that not include INDI 2.0, so almost all, even Ubuntu 23.04 only include 1.9.9. This force user to install INDI from source and is very inconvenient to build in a PPA,

I try on Debian Buster and cmake with -DUSE_SYSTEM_LIBINDI=1 return: -- Found libindi, version 1.7.5 -- INDI version 1.7.5 found in /usr/include/libindi, but at least version 2.0.0 is required INDI not found. Please install INDI and try again.

knro commented 10 months ago

I agree @pchev , I will see if there is an easy way to build libindi within the CMakeLists.txt but since the cmake build process was significantly changed after 2.0.0 this is going to take some time.