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

Adding Pololu Tic Focuser into INDI drivers #883

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bzizou
Copy link
Contributor

@bzizou bzizou commented Jan 17, 2024

Integration of the driver for the DIY Pololu Tic Focuser from @sebo-b https://github.com/sebo-b/TicFocuser-ng

I tested the full kstars->indi->ticfocuser chain with NiXOS and this branch: https://github.com/bzizou/nixpkgs/tree/ticfocuser

As I only tested the "pololu-tic" connection, this driver only builts when the pololu-tic library is found on the system (https://github.com/pololu/pololu-tic-software) which is the case of the master branch of nixpkgs with the dependency "pololu-tic" (as seen here https://github.com/bzizou/nixpkgs/blob/ticfocuser/pkgs/development/libraries/science/astronomy/indilib/indi-3rdparty.nix)
Once the pololu-tic focuser merged into Indi, I'll do a PR to NiXOS and this system will then be ready to use it with no effort.

I really hope that this great project could be integrated into Indi, as I know that I'm not the only one using this DIY focuser that is very easy to build.

@sebo-b
Copy link

sebo-b commented Jan 17, 2024

LibUSB should work as well as Pololu Tic library - it is based on Pololu Arduino Library. In that case there is no dependency on Pololu Tic Library.

@bzizou
Copy link
Contributor Author

bzizou commented Jan 18, 2024

LibUSB should work as well as Pololu Tic library - it is based on Pololu Arduino Library. In that case there is no dependency on Pololu Tic Library.

OK, have to test this. Then, what do you suggest?

  • Use Pololu Tic library if it is available on the system, failback to libusb else
  • Inconditionnaly use libusb and then simplify this PR

By the way @sebo-b , many thanks for that great work! I'm using Tic-Focuser-Ng since a year and it was very easy to build and cheap!

@knro
Copy link
Collaborator

knro commented Jan 18, 2024

I concur. Having this based on libusb would simplify dependencies greatly.

@bzizou
Copy link
Contributor Author

bzizou commented Jan 18, 2024

So, just checked the build without pololu tic library and it's ok.
Then I just forced-pushed a version that does not enforce the use of this library.

@knro
Copy link
Collaborator

knro commented Jan 19, 2024

@bzizou Please check the build errors above

@bzizou
Copy link
Contributor Author

bzizou commented Jan 19, 2024

@bzizou Please check the build errors above

Yes, I saw that. Warnings about deprecations turned into errors. That should be easy to fix.

@bzizou
Copy link
Contributor Author

bzizou commented Jan 19, 2024

Actually, I'm not a CPP expert. Can someone help me to fix this:

'void INDI::DefaultDevice::defineText(ITextVectorProperty*)' is deprecated: Use defineProperty(INDI::Property &)

from this code line:

       m_Device->defineText(&TicSerialNumberTP);

Comment on lines +64 to +68
ISwitch FocusParkingModeS[2];
ISwitchVectorProperty FocusParkingModeSP;

ISwitch EnergizeFocuserS[2];
ISwitchVectorProperty EnergizeFocuserSP;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use INDI::PropertySwitch

Comment on lines +80 to +84
IText InfoS[InfoTabSize] = {};
ITextVectorProperty InfoSP;

IText* InfoErrorS;
ITextVectorProperty InfoErrorSP;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use INDI::PropertyText

@knro
Copy link
Collaborator

knro commented Jan 19, 2024

You need to use new-style INDI property as I commented above. It's fairly easy to use.

@knro
Copy link
Collaborator

knro commented Jan 23, 2024

@bzizou Do you need any help? INDI 2.0.6 is set to release in a week. Hopefully we can include this driver by then.

@bzizou
Copy link
Contributor Author

bzizou commented Jan 23, 2024

@bzizou Do you need any help? INDI 2.0.6 is set to release in a week. Hopefully we can include this driver by then.

To be honest, yes... The code has not been written by me and I'm not a C++ developer (I've been a little, far away in the past ;-) ), so I don't understand the other errors I have when I do the suggested changes into the header files. For example:

Here's my patch:

diff --git a/indi-ticfocuser/TicFocuser.h b/indi-ticfocuser/TicFocuser.h
index 860cd340..e1206a47 100644
--- a/indi-ticfocuser/TicFocuser.h
+++ b/indi-ticfocuser/TicFocuser.h
@@ -62,10 +62,10 @@ class TicFocuser : public INDI::Focuser
         FocusDirection lastFocusDir; //< used to identify direction reversal (for backlash)
 
         ISwitch FocusParkingModeS[2];
-        ISwitchVectorProperty FocusParkingModeSP;
+        INDI::PropertySwitch FocusParkingModeSP {1};
 
         ISwitch EnergizeFocuserS[2];
-        ISwitchVectorProperty EnergizeFocuserSP;
+        INDI::PropertySwitch EnergizeFocuserSP {1};
 
         enum InfoTab {
             VIN_VOLTAGE,
@@ -78,10 +78,10 @@ class TicFocuser : public INDI::Focuser
         };
 
         IText InfoS[InfoTabSize] = {};
-        ITextVectorProperty InfoSP;
+        INDI::PropertyText InfoSP {1};
 
         IText* InfoErrorS;
-        ITextVectorProperty InfoErrorSP;
+        INDI::PropertyText InfoErrorSP {1};
 };
 
 #endif // TICFOCUSER_H
diff --git a/indi-ticfocuser/connection/BluetoothConnection.h b/indi-ticfocuser/connection/BluetoothConnection.h
index 087b1402..33b30973 100644
--- a/indi-ticfocuser/connection/BluetoothConnection.h
+++ b/indi-ticfocuser/connection/BluetoothConnection.h
@@ -52,7 +52,7 @@ class BluetoothConnection:
         bool callHandshake();
 
         IText BtMacAddressT[1] {};
-        ITextVectorProperty BtMacAddressTP;
+        INDI::PropertyText BtMacAddressTP {1};
 
         std::string requiredBtMacAddress;
 
diff --git a/indi-ticfocuser/connection/UsbConnectionBase.h b/indi-ticfocuser/connection/UsbConnectionBase.h
index 1137a043..b672d99d 100644
--- a/indi-ticfocuser/connection/UsbConnectionBase.h
+++ b/indi-ticfocuser/connection/UsbConnectionBase.h
@@ -47,7 +47,7 @@ class UsbConnectionBase:
     protected:
 
         IText TicSerialNumberT[1] {};
-        ITextVectorProperty TicSerialNumberTP;
+        INDI::PropertyText TicSerialNumberTP {1};
 
         std::string requiredSerialNumber;

And I get such errors:

/build/indi-3rdparty/indi-ticfocuser/TicFocuser.cpp: In member function 'virtual bool TicFocuser::initProperties()':
/build/indi-3rdparty/indi-ticfocuser/TicFocuser.cpp:112:25: error: 'INDI::PropertyView<T>* INDI::PropertyBasic<T>::operator&() [with T = _ISwitch]' is protected within this context
  112 |     IUFillSwitchVector(&FocusParkingModeSP,FocusParkingModeS,2,getDeviceName(),"FOCUS_PARK_MODE","Parking Mode",OPTIONS_TAB,IP_RW,ISR_1OFMANY,60,IPS_IDLE);

@bzizou
Copy link
Contributor Author

bzizou commented Jan 23, 2024

Maybe @sebo-b , the original author, can help?

@sebo-b
Copy link

sebo-b commented Jan 27, 2024

Hi - I will try to help, but I need to prepare the whole setup as I was not working on that code for a few years. Give me a week or so :)

@bzizou
Copy link
Contributor Author

bzizou commented Jan 27, 2024

Hi - I will try to help, but I need to prepare the whole setup as I was not working on that code for a few years. Give me a week or so :)

Great! By "the whole setup", do you mean the hardware? As soon as the code compiles without errors, I Can do the functional tests on the whole setup if you want, just Ask 😉

@sebo-b
Copy link

sebo-b commented Jan 27, 2024

By the whole setup, I meant the build environment. I was hoping that you can test it :)

@sebo-b
Copy link

sebo-b commented Jan 30, 2024

@bzizou I have updated the project to avoid the use of the deprecated functions. Please compile and test it in the setup. It will be best if you compile both Pololu and LibUSB interfaces and test both. (the new code is in my original repo).

@knro I started moving the old property interfaces to the new ones, actually, I have almost finished it, but then I realized that it doesn't make too much sense in the current state of migration. Most of the drivers are still not updated, Connection::Interface and INDI::Focuser (base classes I use) are not migrated, so the old API will be mixed with the new one in multiple places. I will migrate when it becomes required as I assume it is not required now.

@knro
Copy link
Collaborator

knro commented Jan 30, 2024

It is required now for all new drivers. It will take a while to migrate all the other base classes and 3rd party drivers.

@bzizou
Copy link
Contributor Author

bzizou commented Jan 30, 2024

@bzizou I have updated the project to avoid the use of the deprecated functions. Please compile and test it in the setup. It will be best if you compile both Pololu and LibUSB interfaces and test both. (the new code is in my original repo).

OK, I'll test this asap (I'm currently under heavy load at work, and I hope that I'll have some time during the evenings this week)

@sebo-b
Copy link

sebo-b commented Jan 31, 2024

@bzizou thx

@knro I see, so I will finish the refactoring to the new APIs. As I'm not sure if I will be able to fully test it, would you be able to review the changes?

@knro
Copy link
Collaborator

knro commented Jan 31, 2024

Yes sure.

@knro
Copy link
Collaborator

knro commented Jul 3, 2024

@sebo-b Can you please rebase?

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