Motor Zaber Motion

[colbysparks]

Review for zaber motion's implementation of asyn motor controller and axis. This module makes use of ZaberMotionCppSupport package which sits in the ./zaberMotionSupport folder. This package isn't included in the diff as it contains many shared libraries and header files.

This repo also includes scripts for updating the zml support package, and for configuring and building the zaber motor library, example ioc and unit tests.

Also, this github repository is a mirror of a zaber's public zaber-motion-epics gitlab repo.

[kmpeters]

Are there really three controllers all sharing /dev/USB0? Controllers from other vendors usually have a single USB connection per controller, so it is much more common to see USB0, USB1 and USB2 for a configuration like this. Also, for systems that have multiple USB devices connection, it is much more convenient to use the soft-links in /dev/serial/by-id/ (on RHEL systems), rather than the /dev/ttyUSB# devices, because the by-id links don't depend on the order the devices were connected to the system.

[colbysparks]

Zaber devices can be chained together on a single serial connection, so this is possible. Each device has a unique device chain index, which is passed as the last parameter of the create function.

[colbysparks]

Also, for systems that have multiple USB devices connection, it is much more convenient to use the soft-links in /dev/serial/by-id/ (on RHEL systems), rather than the /dev/ttyUSB# devices, because the by-id links don't depend on the order the devices were connected to the system.

I realized that I didn't respond to this portion of your comment. I'll update this to use soft-link and also throw in an example tcp configuration in case it might be useful.

[colbysparks]

I added an example of how to set up a 2-axis controller with a tcp connection, but decided against using soft-links for serial because /dev/ttyUSBX seems salient enough (for me at least).

[kmpeters]

The ZML_HOST_ARCH, ZML_SUPPORT_LIB, and ZML_SUPPORT_INCLUDE should be moved to the CONFIG_SITE file, rather than reside in the RELEASE file. My understanding is that the only thing that should reside in RELEASE files is module paths (or the inclusion of other files that contain module paths). It is likely that the inclusion of these definitions in the RELEASE file has resulted in them also being included in the IOC's envPaths file, which doesn't make a lot of sense.

[kmpeters]

Does the zaberMotionSupport directory need its own configure subdirectory? I would have expected the build to work if the contents in the zaberMotionSupport/RELEASE file instead resided in the configure/RELEASE and configure/CONFIG_SITE files at the top of the module.

[colbysparks]

So this should be moved to the top level config site? I was honestly a bit confused about how to define and include ZML_SUPPORT_INCLUDE and ZML_SUPPORT_LIB so I can access them from the app and ioc Makefile.

I agree that it doesn't actually make very much sense to define them in zaberMotionSupport/configure though

[kmpeters]

It is probably a good idea to add _WIN32 or _Linux suffixes to the LIBRARY_IOC variable, like Mark Rivers recently did for the motorAutomation1 support, to avoid build problems when cross compiling for unsupported platfors like VxWorks or RTEMS in addition to building for host architectures like Linux:

https://github.com/epics-motor/motorAutomation1/pull/35/files#diff-8c5311d1a1d8f4d777d776f0396bb31d56cd632eb0f9afb20e8a2d0a0ba1fa42L12-R13

[colbysparks]

Is there a suffix for macOS as well? I imagine it's not a very common use case, but I have been developing on mac

[kmpeters]

It looks like _Darwin is the suffix for macOS:

https://github.com/areaDetector/ADCore/blob/0ed979a1c8e1f74a16d5c3d80d847417c582f42d/ADApp/pluginTests/Makefile#L27

[kmpeters]

This is much better documentation than I ususally write. Users will appreciate it.

[kmpeters]

The approach used by the xxx (example synApps IOC) module is to provide separate st.cmd files for each support arch:

https://github.com/epics-modules/xxx/tree/master/iocBoot/iocxxx

It is a judgement call whether having lots of unused files or having to edit a file before it can be used is more annoying to users.

[colbysparks]

If this is the precedent, then I think it makes sense to do it this way.

[kmpeters]

I'm able to build the module now. I'm building it as a standalone module, not as a submodule of motor. I created a RELEASE.local file that looks like defines the path to MOTOR:

MOTOR=/home/username/epics/local/zaberReview/synApps_6_3/support/motor
-include $(MOTOR)/modules/RELEASE.$(EPICS_HOST_ARCH).local
# path to motorAcsMotion is needed to build the IOC inside motorZaberMotion, but outside motor
#!MOTOR_ZABER_MOTION=

Creating a RELEASE.local file or editing a top-level RELEASE file is normal. I expect to do it for every EPICS module.

I also needed to correct the path to MOTOR_ZABER_MOTION in motorZaberMotion/zaberMotionSupport/configure/RELEASE, which is not something I normally expect to do.

My expectation is that the the libraries that are installed in the zaberMotionSupport/lib directory are instead installed to the motorZaberMotion/lib directory, instead of the libraries residing in two locations:

❯ ls -l lib/linux-x86_64 zaberMotionSupport/lib/linux_x64
lib/linux-x86_64:
total 10284
-r--r--r-- 1 username group 6996230 Nov 15 15:41 libZaberMotion.a
-r-xr-xr-x 1 username group 3480912 Nov 15 15:41 libZaberMotion.so

zaberMotionSupport/lib/linux_x64:
total 41912
-rw-rw-r-- 1 username group 12728208 Nov 15 09:22 libzaber-motion-core.so.7.2.0
lrwxrwxrwx 1 username group       22 Nov 15 09:22 libzaber-motion.so -> libzaber-motion.so.7.2
lrwxrwxrwx 1 username group       24 Nov 15 09:22 libzaber-motion.so.7.2 -> libzaber-motion.so.7.2.0
-rwxrwxr-x 1 username group 30003368 Nov 15 09:22 libzaber-motion.so.7.2.0

I think areaDetector drivers are much better examples of how to do this, since many of them include vendor libraries:

https://github.com/areaDetector/ADSpinnaker

[colbysparks]

I also needed to correct the path to MOTOR_ZABER_MOTION in motorZaberMotion/zaberMotionSupport/configure/RELEASE, which is not something I normally expect to do

Does it make sense to define MOTOR_ZABER_MOTION in configure/RELEASE, and give the user the option to override that in their own custom RELEASE.local file? It's definitely not an external support module, but if users are used to modifying RELEASE files then this might be the right place for it.

[colbysparks]

Ok I just tested building this as standalone and as a motor submodule and it seems to be working. Support libs are being installed in the correct place and standalone configuration is as simple as creating a RELEASE.local file.

[kmpeters]

I was able to further simplify the build configuration using a relative path to the include files. I pushed my changes to the review-branch-build-improvements branch:

https://github.com/colbysparks/motorZaberMotion/compare/review-branch...review-branch-build-improvements?expand=1

I tested the build as a standalone module by copying the EXAMPLE_RELEASE.local and defining only the path to MOTOR and as a submodule of motor by adding the following line to motor/modules/Makefile:

diff --git a/modules/Makefile b/modules/Makefile
index 13b5cdc5..7de8cf0b 100644
--- a/modules/Makefile
+++ b/modules/Makefile
@@ -41,6 +41,7 @@ endif
 SUBMODULES += motorSmarAct
 SUBMODULES += motorSmartMotor
 SUBMODULES += motorThorLabs
+SUBMODULES += motorZaberMotion
 
 # Allow sites to add extra submodules
 -include Makefile.local

[colbysparks]

I was able to further simplify the build configuration using a relative path to the include files. I pushed my changes to the review-branch-build-improvements branch:
https://github.com/colbysparks/motorZaberMotion/compare/review-branch...review-branch-build-improvements?expand=1

Apparently there were a lot of cascading effects from my not having specified the LIB_INSTALLS path correctly. Now that this has been updated, the support libs always get installed to the correct location, so there is no longer an need to specify -rpath nor add any extra -L paths.

Thanks for figuring this out! When I saw other repos using ../ to install support libs I thought they might have been wrong, but it turns out that this path needs to be relative to the Makefile in ./O.linux-<arch>, not relative to this file path..

I just tested your changes on review-branch-build-improvements on my linux machine and everything works, but I'm getting a runtime linking error running the example ioc on macOS. I'll commit these changes once I figure it out.

[kmpeters]

I'm sorry I don't have an OS X machine I can use for testing.

[colbysparks]

All good. I just discovered that the build system breaks symlinks when it copies the libs from the support package folder. I'll have to make some changes so that it doesn't just add three copies of the same library to the target lib install folder.

That said, even if libzaber-motion.dylib is a copy of the versioned lib it points to, runtime linking should still work on macOS so I doubt that's the issue I'm seeing.

[colbysparks]

I haven't managed to find a good solution to the symlink copying issue in the EPICS build system, nor on the gulpfile scripting side of things. I might just have to make changes to how the zml support package is built, but that's a conversation I need to have with my team. I am fine leaving this for now but will make a note to fix some time in the short term.

As for the macOS problem, it's very likely that I could have missed something, but it seems like there may be an oversight in the EPICS build system where rpaths are not included for shared libs which need them to be specified. When linking the example ioc on linux (and LINKER_USE_RPATH=YES), the build system system will add rpaths for all the lib folders, including the folder which contains the support libs:

-Wl,-rpath,/home/ubuntu/EPICS/support/motor/lib/linux-aarch64

But for darwin builds no rpaths are passed to the linker, even with LINKER_USE_RPATH=YES, so it looks like a macOS user may have to manually add the rpath or use a command line tool for specifying an rpath in an existing executable.

[kmpeters]

The "-rpath" option for Linux comes from this config file in EPICS base:

https://github.com/epics-base/epics-base/blob/b7cc33c3c9c1a6cc14290b9a558e97bd89171e80/configure/os/CONFIG.Common.linuxCommon#L26-L27

There isn't any reference to "rpath" in any of the darwin config files:

❯ pwd
/home/username/epics/local/zaberReview/base-7.0.8.1/configure/os
❯ grep rpath * | grep -i darwin
❯ 

I don't know if it is an oversight or not. The EPICS base developers have a mailing list where they answer questions like this:

https://epics.anl.gov/core-talk/

[colbysparks]

The "-rpath" option for Linux comes from this config file in EPICS base:
https://github.com/epics-base/epics-base/blob/b7cc33c3c9c1a6cc14290b9a558e97bd89171e80/configure/os/CONFIG.Common.linuxCommon#L26-L27

I ended up finding this yesterday shortly after making that comment. Adding the following to either CONFIG or CONFIG_SITE does fix the build issue with darwin:

PRODDIR_RPATH_LDFLAGS_YES += $(PROD_DEPLIB_DIRS:%=-Wl,-rpath,%)
PRODDIR_LDFLAGS += $(PRODDIR_RPATH_LDFLAGS_$(LINKER_USE_RPATH))

[colbysparks]

I just emailed the core-talk mailing list, so I will wait to hear back from them on whether or not the above is an appropriate solution for darwin builds