analogdevicesinc / libiio

A cross platform library for interfacing with local and remote Linux IIO devices
http://analogdevicesinc.github.io/libiio/
GNU Lesser General Public License v2.1
492 stars 318 forks source link

iiod systemd unit won't start with only hwmon devices #1194

Closed normanr closed 5 hours ago

normanr commented 3 months ago

I think that the ConditionPathExists in the systemd service file (permalink) is over restrictive now that hwmod support is enabled by default (#977)

I think something like

ConditionPathExists=|/sys/bus/iio
ConditionPathExists=|/sys/class/hwmon

should allow the service to start if iio or hwmon devices are present. (The pipe symbol (|) allows the service to start if any conditions are satisfied, instead of the default where all conditions need to be satisfied - ref)

I'm not actually sure if /sys/class/hwmon will be missing on a system without hwmon devices (I don't have any test machines without hwmon devices). If it's always present, then the condition should rather be written using a glob, something like the following should be used instead:

ConditionPathExists=|/sys/bus/iio
ConditionPathExistsGlob=|/sys/class/hwmon/*

(of course it could be made so that the extra line is only added if HWMON support is enabled, but I don't know cmake well enough to suggest the syntax for that).

rgetz commented 2 months ago

Can confirm on Pi 5 / default Raspberry Pi OS - /sys/bus/iio does not exist, /sys/class/hwmon exists, and iiod does not start.

It's a minor tweak to libiio/iiod/init/iiod.service.cmakein similar to what's suggested, plus actually turning it on, (otherwise it's installed but not enabled)...

rgetz commented 2 months ago

@normanr : Can you try this out? This is a patch for the libiio-v0 branch, but you should be able to apply to main as well (with some minor tweaks).

diff --git a/iiod/CMakeLists.txt b/iiod/CMakeLists.txt
index b1c75e81..b76c0483 100644
--- a/iiod/CMakeLists.txt
+++ b/iiod/CMakeLists.txt
@@ -90,8 +90,22 @@ if(NOT SKIP_INSTALL_ALL)
 endif()

 if (WITH_SYSTEMD)
+       if(${WITH_HWMON})
+               set(CMAKE_SERVICE_PATH "ConditionPathExists=|/sys/bus/iio\nConditionPathExists=|/sys/class/hwmon")
+       else()
+               set(CMAKE_SERVICE_PATH "ConditionPathExists=/sys/bus/iio")
+       endif()
        configure_file(${CMAKE_CURRENT_SOURCE_DIR}/init/iiod.service.cmakein ${PROJECT_BINARY_DIR}/init/iiod.service)
        install(FILES ${PROJECT_BINARY_DIR}/init/iiod.service DESTINATION ${SYSTEMD_UNIT_INSTALL_DIR})
+       find_program(SYSTEMCTL
+               NAMES systemctl
+               DOC "Control the systemd system and service manager (systemctl)")
+       if(SYSTEMCTL)
+               message("starting iiod.service with ${SYSTEMCTL}")
+               install(CODE "execute_process(COMMAND ${SYSTEMCTL} enable iiod.service)")
+               install(CODE "execute_process(COMMAND ${SYSTEMCTL} restart iiod.service)")
+               install(CODE "execute_process(COMMAND ${SYSTEMCTL} status iiod.service)")
+       endif()
 endif()

 if (WITH_SYSVINIT)
diff --git a/iiod/init/iiod.service.cmakein b/iiod/init/iiod.service.cmakein
index 0f762474..0330d574 100644
--- a/iiod/init/iiod.service.cmakein
+++ b/iiod/init/iiod.service.cmakein
@@ -8,7 +8,7 @@
 Description=IIO Daemon
 Requires=systemd-udev-settle.service
 After=network.target systemd-udev-settle.service
-ConditionPathExists=/sys/bus/iio
+@CMAKE_SERVICE_PATH@

 [Service]
 EnvironmentFile=-/etc/default/iiod
julienmalik commented 2 months ago

Hi @rgetz

The first part of the patch looks fine at solving @normanr issue, but the second part regarding starting the service will fail when cross-compiling (or installing without root permissions). IMHO installation and execution are two different concepts.

rgetz commented 2 months ago

all the instructions are "sudo make install" - so I don't think that is a problem.

As for cross compile - adding an AND NOT CMAKE_CROSSCOMPILING makes sense to me. Installing and enabling would be the equivalent to what initd does.

rgetz commented 2 months ago

I think this is fixed in main (almost), and on the v0 branch - so this can be closed.