libpktriggercord

[asalamon74]

A pktriggercord libary could make it easier to include pktriggercord into other programs. This is mentioned here: https://github.com/indilib/indi-3rdparty/pull/66 and the first version of thelibrary was uploaded by @karlrees in #48

[asalamon74]

I'll create a new branch for this development: https://github.com/asalamon74/pktriggercord/tree/libpktriggercord we can freely commit there, at the very end I just squash the commits to the master branch.

So far I added the commit (I had to rebase it) uploaded in #48 and also added commits ( https://github.com/karlrees/pktriggercord/pull/1 ) by@blemasle .

[asalamon74]

About the packaging question: Probably the real nice solution would be to create a libpktriggercord package and a separate pktriggercord package, the latter should require the first. So if someone want's to use it only as a library, no need the install both packages.

On the other hand it would make more difficult to install the downloaded rpms, so for now I think we can just use one single package. Later we can revisit this decision.

[asalamon74]

@karlrees @blemasle Added you as collaborators, please only modify the libpktriggercord branch.

[asalamon74]

Right now windows cross-compilation does not work, this is also reported by travis: https://travis-ci.org/github/asalamon74/pktriggercord/builds/679689138

[karlrees]

I agree that that it would probably be best in the long run to separate the packages. It'll probably take a while to get there, though.

Not sure how much time I'll have in the immediate future to contribute, but I'm happy to provide input. I may have some time later next month to actually code.

[blemasle]

Great news ! I never got compilation to fully work on any of my machines, all of which are running debians or debians under wsl. I always get segfaults on the directives generated from

winobjs:$(SRCOBJNAMES:=.c) 
    $(foreach srcfile, $(SRCOBJNAMES:=.c), $(WINGCC) -DVERSION='"$(VERSION)"' -DPKTDATADIR=\".\" $(WIN_CFLAGS) -c $(srcfile);)

i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pslr.c; i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pslr_enum.c; i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pslr_scsi.c; i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pslr_lens.c; i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pslr_model.c; i686-w64-mingw32/mingw32/bin/i686-w64-mingw32-gcc -DVERSION='"0.85.01"' -DPKTDATADIR=\".\" -O3 -g -Wall -Isrc/external/js0n -Ii686-w64-mingw32/include/gtk-2.0/ -Ii686-w64-mingw32/lib/gtk-2.0/include/ -Ii686-w64-mingw32/include/atk-1.0/ -Ii686-w64-mingw32/include/cairo/ -Ii686-w64-mingw32/include/gdk-pixbuf-2.0/ -Ii686-w64-mingw32/include/pango-1.0/ -c pktriggercord-servermode.c; Segmentation fault Segmentation fault Segmentation fault Segmentation fault Segmentation fault Segmentation fault

Just tried under Ubuntu 20.04 LTS, same issue. I never face this when running make win-lib directly, only when running make win or make winobjs. I totally forgot about this and got the habbit to use make win-lib for my personnal usage.

I'm currently trying to setup an dev environment similar to the one used on Travis CI (Ubuntu Xenial 16.04) so I can get the whole thing to compile and work on the windows cross compilation.

[blemasle]

Finally got the whole thing to compile and run under a Xenial VM. Using any distro more recent than that just crash with a segfault. For the moment, I just fixed the windows compilation and nothing else.

We should start to move code around so that both the cli and the gui use code defined within the lib, even if the "lib" is a simple .c file at first. At the moment some functions extracted from the cli (notably save_buffer) prevent this because of naming conflicts.

I'm happy to start working on it, but it could mean undoing what @karlrees did for the new pentax INDI driver. Not permanently, but just the time to figure what goes in the lib and what needs to be changed in the cli/gui to reuse that code.

[karlrees]

I'm fine with that. I figure once we get the library standardized, I'd update the INDI driver to whatever API we decide upon anyway.

I was thinking we should put this all in a separate namespace, but then I remembered it's C instead of C++. So we should probably append all of the exposed functions with a unique prefix to avoid conflicts with other libraries. Maybe pk_?

[blemasle]

The current code base already use pslr_ all over the place to define enums and typedefs that we will also need to expose at some point. I think we should reuse that prefix if we can.

It will also make the code easier to port to the "new" library for those that currently use a forked version of this repo (eg Gphotos), because renaming will be limited.

There is also a recurring ipslr_ prefix of which I'm not sure to understand the intent. Maybe @asalamon74 can help us about that :)

BTW : fixed android build :)

[asalamon74]

ipslr_ prefix is for the low level internal funcions, mostly dealing with the SCSI protocol. pslr is for the higher level function. For instance pslr_set_raw_format can be used to set the RAW format, and it will call ipslr_handle_command_x18 which will the x18 SCSI command with the appropriate parameters.

PkRemote (the project I forked to create PkTriggerCord) had a libpslr library (https://sourceforge.net/p/pkremote/code/HEAD/tree/Makefile) so I think the original idea was to put pslr_ functions into the library. On the other hand clients should not deal with ipslr_ function.

In a nice world these should not be in the same file, but it was already mixed.

So probably the easiest is to use plsr_ prefix.

Travis check is green, yeah. :)

[asalamon74]

Unfortunately we have two save_buffer functions, one for the cli and one for the UI. They are very similar but not exactly the same. I always wanted to merge them but I had no time for that.

[blemasle]

Great ! Thanks for all the info.

FYI, I'm currently working on making a static libpktriggercord static lib that the cli reuses. This involves some changes and reorganization in the Makefile and as it's probably breaking the build I won't push it for the moment.

[blemasle]

If things break, I'll try to fix it along the way. Made progress today 😄

libpktriggercord.a

All .c/.h "core" files now build to a static library

libpktriggercord.so / libpktriggercord.dll

The shared library is built from libpktriggercord.a static library. Some macros have been defined to restrain what gets exposed. __declspec(dllexport) on windows seems to only have an impact once at least one member is declared with such attribute. So at the moment every member is exposed through the dll. I've however tested that it work as intended, hiding all members except the ones marked with the PK_API helper.

pktriggercord-cli / pktriggercord

Both applications are linked to the static library.

Makefile

I've reworked the Makefile to remove redondant code & configuration. Appart from the downlod of gtk2 binaries, I've entirely removed the win32 compilation that was using an outdated mingw32 (the so called localwin). This allows to build all targets for Windows on a more recent distros than Ubuntu Xenial. I do not have segmentation faults anymore on debian 10, ubuntu 20.04 or raspbian and I can use each distros packages to build the applications.

Cross compilation

Targets have been reoganized / redefined so that Linux & Win32 targets are identical, minus a few configuration differences (and the download of the gtk2 binaries of course). Use make ARCH=Win32 to build targets for Windows instead of Linux by default. Downside is, you cannot build for both platforms at the same time anymore. This wasn't really an option anyway as intermediate binaries are specific for each platform.

[asalamon74]

Originally I created the cross-compilation (make win) to use my local MINGW32, later (mostly for travis) make localwin was created, which basically downloaded migw32 and used it for compilation. It was of course redundant, using ARCH is a much neater idea.

Building both platforms at the same time is not important, I never trusted the build system that much anyway.

I modified the Makefile a bit, now targets like rpm, deb, .. are not available for Win32.

I also removed the all target from the Win32 and modified the all target for Linux not to include the Windows compilation.

One thing is a bit still strange in the windows cross-compilation: We still have the win_dlls library. There are the dlls from mingw32 required for the windows build to run. In the long run this directory should be eliminated.

[blemasle]

I modified the Makefile a bit, now targets like rpm, deb, .. are not available for Win32.

I hesitated to do so, but didn't because I wanted to limit conditionnal code. Ideally we should use a configure (or even better, CMake as @karlrees has already done all the hard work ?) to make cross platform and dependencies easier 😃 But one thing at a time.

One thing is a bit still strange in the windows cross-compilation: We still have the win_dlls library

Maybe they could be downloaded at build time in the same way the gtk dlls are ?

[asalamon74]

Yes, there are quite a few tools (configure, cmake, ...) for this purpose, but a simple (?) Makefile is working right now, and it's really portable.

Probably downloading the dlls are better, I'll check it.

[blemasle]

I just pushed a bunch of renaming we've talked about. This renaming mostly concern symbols that will most likely be exported; For reference, here there are.

Member	Renamed to
camera_close	pslr_camera_close
camera_connect	pslr_camera_connect
collect_settings_info	pslr_get_settings_info
collect_status_info	pslr_get_status_info
copyright	pslr_copyright
debug_onoff	pslr_set_debugmode
file_formats	pslr_user_file_formats
find_model_by_id	pslr_find_model_by_id
find_setting_by_name	pslr_find_setting_by_name
format_rational	pslr_format_rational
get_file_format_t	pslr_get_user_file_format_t
get_hw_jpeg_quality	pslr_get_hw_jpeg_quality
get_lens_name	pslr_get_lens_name
get_pslr_ae_metering	pslr_get_ae_metering
get_pslr_ae_metering_str	pslr_get_ae_metering_str
get_pslr_af_mode	pslr_get_af_mode
get_pslr_af_mode_str	pslr_get_af_mode_str
get_pslr_af_point_sel	pslr_get_af_point_sel
get_pslr_af_point_sel_str	pslr_get_af_point_sel_str
get_pslr_af11_point_str	pslr_get_af11_point_str
get_pslr_color_space	pslr_get_color_space
get_pslr_color_space_str	pslr_get_color_space_str
get_pslr_custom_ev_steps_str	pslr_get_custom_ev_steps_str
get_pslr_custom_sensitivity_steps_str	pslr_get_custom_sensitivity_steps_str
get_pslr_drive_mode	pslr_get_drive_mode
get_pslr_drive_mode_str	pslr_get_drive_mode_str
get_pslr_flash_mode	pslr_get_flash_mode
get_pslr_flash_mode_str	pslr_get_flash_mode_str
get_pslr_image_format_str	pslr_get_image_format_str
get_pslr_jpeg_image_tone	pslr_get_jpeg_image_tone
get_pslr_jpeg_image_tone_str	pslr_get_jpeg_image_tone_str
get_pslr_raw_format_str	pslr_get_raw_format_str
get_pslr_scene_mode_str	pslr_get_scene_mode_str
get_pslr_white_balance_mode	pslr_get_white_balance_mode
get_pslr_white_balance_mode_str	pslr_get_white_balance_mode_str
get_user_file_format	pslr_get_user_file_format
pslr_camera_name	pslr_get_camera_name
pslr_read_datetime	pslr_get_datetime
pslr_read_dspinfo	pslr_get_dspinfo
pslr_read_setting	pslr_get_setting
pslr_read_settings	pslr_get_settings
pslr_select_af_point	pslr_set_selected_af_point
pslr_set_ec	plsr_set_expose_compensation
pslr_write_setting	pslr_set_setting
pslr_write_setting_by_name	pslr_set_setting_by_name
shexdump	pslr_hexdump

I'll try to keep this list up to date is other renaming are performed, it could be useful once the library get published.

[karlrees]

Awesome. Wish I had some time to test things. As for the cmake discussion, I was only getting cmake to compile my version of the library on linux, I never really got it doing anything else.

[blemasle]

I'm currently trying to find the best and most maintainable way to restrict exported symbols, but I'm afraid this would mean a complete recompilation for the .dll. There will no longer be an option to create the .dll from the static lib, because we need different attributes placed on symbols in each case.

The issue would only be for the .dll though, as .so works diffrently and will probably not need recompilation.

[blemasle]

Only relevant symbols are now exported when building the lib. ⚠️ BUT : Doing so requires recompilation of .c files. To keep things easy, compiling the shared lib requires .ol files instead of .o. This ensures proper files are used for both static and shared lib.

I however noticed that the gui isn't running anymore since I guess b27ca6370 because of missing dlls :

• libpng16-16.dll
• intl.dll
• libgthread-2.0-0.dll

libpng14-14.dll is downloaded, not libpng16-16.dll so that's one less error to track down. No sure what's going on about the two others though.

[blemasle]

I haven't got much time to work on this on the past few weeks, but things are clearing out right now.

My next step : Moving things around so that the cli/gui can be built against the shared lib. I'm aware it is not what we are looking for in the end. But if it can be done, then libpktriggercord is most certainly approaching the point where it is ready. Trying do to so has already pinpointed some hiccups in the exported symbols :)

[blemasle]

Win32 dlls dependency hell is solved once and for all (hopefully) by using the gtk bundle available here : http://ftp.gnome.org/pub/gnome/binaries/win32/gtk+/2.24/gtk+-bundle_2.24.10-20120208_win32.zip

There is no additional download needed. This ensure that output are compiled against the same source as they are dynamically linked at runtime (the bundle contains src, lib and binary files). With these changes, win_dlls can be removed entirely.

Some symbols were also missing from the original work. This is now fixed.

[asalamon74]

Sorry I had no time for this project in the last moth.

Somehow github commit page does not show the travis checks, but it looks good for this branch: https://travis-ci.org/github/asalamon74/pktriggercord/builds/703299876

Downloading one bundle zip is a much cleaner solution. Great idea.

[blemasle]

No worries, I myself am only dedicated to this when I have some spare time :) I found the bundle by chance navigating the gnome release tree.

Anyway, it is much simpler indeed, and now you can also be sure that what has been used to build the app is the same code as the packaged dlls 👍

I think we should start to consider a merge to master, before breaking things further. For one, it could provides a smoother transition from forked code to library usage. Further refactoring (bulb for instance) has nothing to do with the library itself and should be done separately. I also think we should wait for a new release with the few recent fixes before merging. Doing so will allow any fork to include the latest fixes easily before a more tedious work. But there is no rush :)

I'm just not sure where to put .def and .lib files. Should they really go with the win32 distribution package ?

[blemasle]

Hi @asalamon74 ,

As I didn't hear for you after the last commits, I left the project aside. Today I merged the last fixes from master to the branch. I'll open a pull request with it. Even if it's not get merged right away, it will make things easier to track.

Cheers.

[asalamon74]

@blemasle Sorry for the missing response. This is a big change and I was always afraid to commit it and had no time to really testing it. This change addresses multiple things so I think I'll try to commit in multiple parts. I'll start with the windows compilation changes which are quite useful even without the library part. Thanks for all the work.

[blemasle]

@asalamon74 No worries. If you want, I can open a PR with only the changes related to the windows compilation (well, the Makefile makeover actually). I can maintain https://github.com/asalamon74/pktriggercord/pull/68 up to date to date with master, but it will be a hard work. There is already some conflicts despite the fact that I merged master into the branch right before opening the PR.

Appart from a massive renaming / moving, there isn't much change to the code itself.

[asalamon74]

@blemasle I already committed the windows cross-compilation: deae2e60e58441e6762c25fb62779ccba856edfb

[blemasle]

Great! Do you still plan to reintegrate the branch at some point in the future?

I don't mind if you don't, really. I think it would be bad news for other pieces of codes out there that have to maintain a fork of this repository for the exact purpose this branch was set up initially. But I in total honesty don't want to spend time maintaining that branch up to date with master if it won't be merged.

Every change committed to master needs special care because the code has been moved around so that it could be included in the library.

Regards

[asalamon74]

One more commit, the method renames: bdba056f5f4663f5a5492049c5c960d9cab4531d

I'll keep adding more and more from this branch. I know that I'm terribly slow, but it's getting closer:

$ git diff HEAD^..libpktriggercord | wc -l
3150
$ git diff HEAD..libpktriggercord | wc -l
2333

[blemasle]

I know that I'm terribly slow, but it's getting closer

No worries. I'm glad to see that this work will eventually be reintegrated at some point 👍

If you ever need help to glue old and new commits together, do not hesitate to reach me :) I remember having a hard time moving functions around so that references were optimal for instances (no extra function in the lib etc).

[asalamon74]

And one more commit, the logging changes: 5f32c9e30e0080ef67b40a866c1a009f218d96c5

This is again a useful change on its own.

$ git diff HEAD..libpktriggercord | wc -l
1576

[asalamon74]

One more commit for pslr_utils: https://github.com/asalamon74/pktriggercord/commit/008737c5bb0d3b6f7af9e26085e4b9bbfc7161d1

$ git diff HEAD..libpktriggercord | wc -l
1350