Make lights work on FreeBSD

[larsbrinkhoff]

Hello,

I'd like or a way to enable lights if libusb is found, but discard the option if the library is not found. This is primarily to make it easier to build for continuous configuration.

Maybe --enable-lights=optional?

[larsbrinkhoff]

According to @tvrusso, libusb should be a standard part of FreeBSD. So maybe just link with libusb rather than libusb-1.0. The build error is this:

checking for libusb_get_device_descriptor in -lusb-1.0... no
configure: error: Lights require libusb-1.0, which I could not find.
gmake[1]: *** bld-ks-its: No such file or directory.  Stop.

[tvrusso]

Suggest changing:

    AC_CHECK_LIB([usb-1.0], [libusb_get_device_descriptor],
                 [],
                 [AC_MSG_ERROR([Lights require libusb-1.0, which I could not find.])]

to

    AC_SEARCH_LIBS([libusb_get_device_descriptor],[libusb-1.0 libusb],
                 [],
                 [AC_MSG_ERROR([Lights require libusb, which I could not find.])]

That way, autoconf will search for the appropriate function in either of the libraries, and use the one it finds.

[tvrusso]

That comment was premature, since there are a few other places where it is assumed that libusb headers live somewhere special, like:

#include <libusb-1.0/libusb.h>

My suggestion won't help that. Other autoconf silliness would be necessary to check for the existence of libusb.h without the prefix, and if that fails, with the prefix, then add an appropriate "-I" flag to CFLAGS, and then remove the "libusb-1.0/" from the include.

On freebsd, libusb.h is just in /usr/include for the base system.

[Rhialto]

You can, using preprocessor magic, create a macro that expands to a full header name, and then #include MACRO. The < and > should I think be included in the value of MACRO. This is somewhat obscure and not often used, but possibly easier than a bunch of conditional blocks with enumerate all possible combinations of directory and name, etc.

With my hat of package creator on, I can't say I like "optional" packages very much. I want that the binaries that I create are fully specified by whatever configure/build options I use, and the random presence or absence of library XYZ should not influence the result.

[tvrusso]

Couple more suggestions. My apologies if you are already autoconf mavens and I'm telling you things you already know.

Rather than "--enable-lights=optional", just let "--enable-lights" and "--disable-lights" work normally, but instead of erroring out when libusb isn't found when lights are requested, just emit a warning and proceed as if "--disable-lights" had been given. In my suggestion below I made it a fairly gentle warning, but if you really, really wanted to call attention to it you could make the AC_MSG_WARN really glaring.

Some half-baked autoconf ideas:

Currently, you have:

case "${enable_lights}" in
    yes)
        AC_MSG_NOTICE([Compiling with Panda lights support])
        AC_DEFINE(KLH10_DEV_LITES, 1, [Set to 1 to enable Panda lights])
        need_libusb=yes
        ;;
    no | "")
        AC_DEFINE(KLH10_DEV_LITES, 0, [Set to 0 to disable Panda lights])
        ;;
    *)  AC_MSG_ERROR([bad value ${enable_lights} for --enable-lights]) ;;
esac

and then later:

case "${need_libusb}" in
    yes)
        # Check if libusb-1.0 is present, otherwise fail.
        # CPULIBS is for libraries only needed by the CPU, not by other programs.
        SAVE_LIBS="$LIBS"
        LIBS=""
        AC_CHECK_LIB([usb-1.0], [libusb_get_device_descriptor],
                     [],
                     [AC_MSG_ERROR([Lights require libusb-1.0, which I could not find.])]
                    )
        CPULIBS="$LIBS"
        LIBS="$SAVE_LIBS"
        ;;
esac

If instead, you did something like this:

case "${enable_lights}" in
    yes)
        AC_MSG_NOTICE([User requested Panda lights support])
         need_libusb=yes
        ;;
    no | "")
        AC_DEFINE(KLH10_DEV_LITES, 0, [Set to 0 to disable Panda lights])
        ;;
    *)  AC_MSG_ERROR([bad value ${enable_lights} for --enable-lights]) ;;
esac

and then later:

case "${need_libusb}" in
    yes)
        # Check if libusb is present, otherwise fail.
        # CPULIBS is for libraries only needed by the CPU, not by other programs.
        SAVE_LIBS="$LIBS"
        LIBS=""
        FOUNDALLUSB="no"
        AC_SEARCH_LIBS([libusb_get_device_descriptor], [libusb-1.0 libusb],
                       [AC_CHECK_HEADERS([libusb.h],
                                         [FOUNDALLUSB="yes"],
                                         [AC_CHECK_HEADERS([libusb-1.0/libusb.h],
                                                           [FOUNDALLUSB="yes"],
                                                           [AC_MSG_WARN([Lights require libusb.h, which I could not find.])
                                                           ])
                                         ])

                       ],
                       [AC_MSG_WARN([Lights require libusb, which I could not find.])]
                      )
        if test $FOUNDALLUSB = "yes"
        then
            CPULIBS="$LIBS"
            AC_DEFINE(KLH10_DEV_LITES, 1, [Set to 1 to enable Panda lights])
        else
            AC_DEFINE(KLH10_DEV_LITES, 0, [Set to 0 to disable Panda lights])
            AC_MSG_WARN([Lights requested by user, but essential USB libraries/headers not found.  Lights are being disabled.])
        fi
        LIBS="$SAVE_LIBS"
        ;;
esac

The result of this coming out of configure is that KLH10_DEV_LITES should only be defined to one if both library and headers are found, and the defined constants HAVE_LIBUSB_H or HAVE_LIBUSB-1.0_LIBUSB_H will be defined, depending on where the header was found (this latter is because I'm using AC_CHECK_HEADERS instead of AC_CHECK_HEADER).

You can then change src/dvlites.c's include of libusb.h to:

#ifdef HAVE_LIBUSB-1.0_LIBUSB_H
#include <libusb-1.0/libusb.h>
#endif 
#ifdef HAVE_LIBUSB_H
#include <libusb.h>
#endif 

and it should compile as long as one or the other is found at configure time, and the whole thing should get skipped if the library or headers are not located. I have not actually tested this modification and it might require a little refining, but it is similar to how I've made optional features conditional on presence of required libraries and headers in other codes.

[tvrusso]

In my previous comment, the macro defined by AC_CHECK_HEADERS for libusb-1.0/libusb.h is NOT "HAVE_LIBUSB-1.0_LIBUSB_H", the minus sign would be a compiler error. It would actually be "HAVE_LIBUSB_1.0_LIBUSB_H".

[tvrusso]

Final comment, then I'm getting out of the way. If, as @Rhialto suggests, the "fall back to disabling when dependent libraries are missing" thing is not desirable, my suggestion could trivially be made to error out instead by changing the final AC_MSG_WARN to an AC_MSG_ERROR, and should still allow both "libusb-1.0" and "libusb" to work correctly (linux vs. freebsd) when possible.

[Rhialto]

I don't mind if something is made explicitly optional I guess, so --enable-lights=optional would be ok. But --enable-lights=yes or similar should preferably error out if it requires something that cannot be found.

[atsampson]

It'd probably be easier to #include <libusb.h> on all platforms, and pick up the appropriate CFLAGS and LDFLAGS using pkg-config (i.e. PKG_CHECK_MODULES in autoconf). Every version of Linux libusb 1.x provides libusb-1.0.pc, and FreeBSD's builtin libusb has provided it since 2013; a quick test on FreeBSD 12.1 (thanks Tim) shows that it returns -lusb for LDFLAGS and nothing for CFLAGS.

[tvrusso]

Yes, using PKG_CHECK_MODULES to find libs and headers seems the better approach to set CFLAGS and LIBS.

[larsbrinkhoff]

Thank you all for your suggestions. My first priority would be to have --enable-lights work as well on FreeBSD as on Linux, so I'll try to do that.

[larsbrinkhoff]

@tvrusso, I made an update which seems to work on FreeBSD.

[tvrusso]

I cd'd to tools/klh10 and checked out your lars/lights branch there, and built "gmake EMULATOR=klh10". The klh10 sim got built with the -lusb and config.h got "#define KLH10_DEV_LITES 1". Looks good to me.

[larsbrinkhoff]

Thanks!