Systemd journactl detection fix; log format minor change; improved fi…

…rst-use suggestions; refactoring; profiling fix; CMakeLists profiler option fix; ensure start PWM is sufficient; README changes

CMakeLists.txt

@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.3)
-set(CMAKE_CXX_STANDARD 14)  # TODO: C++17 support in CMake 3.8 (also cmake_minimum_required -> 3.8)
+set(CMAKE_CXX_STANDARD 14)  # TODO: C++17 when gcc 7 & clang 4 debain testing release; cmake_minimum_required -> 3.8
 project(fancon)
 set(PACKAGE_AUTHOR "Hayden Briese <[email protected]>")
 
@@ -81,7 +81,7 @@ endif ()
 
 ## Google Perf Tools profiling
 option(PROFILE "Build debug release with support for CPU and heap profiling - REQUIRES Google Perf Tools" OFF)
-if (PROFILE AND ${CMAKE_BUILD_TYPE} STREQUAL "Debug")
+if (PROFILE AND CMAKE_BUILD_TYPE STREQUAL "Debug")
     find_package(Profiler REQUIRED)
     find_package(TCMalloc REQUIRED) # TCMalloc currently does not release memory
     set(LIBS ${LIBS} ${PROFILER_LIBRARY} ${TCMALLOC_LIBRARIES})

README.md

@@ -2,53 +2,54 @@
 
 [![License](http://img.shields.io/badge/license-APACHE2-blue.svg)]()
 
-fancon is a Linux fan control daemon and fan testing tool, allowing custom speed-temperature curves for fans, controllable by either PWM or RPM, or percentage.
+A Linux fan control daemon and fan testing tool, allowing custom speed-temperature curves for fans, controllable by either PWM or RPM, or percentage.
   - High performance
   - Low memory usage
   - Support for system fans, and NVIDIA GPUs
 
-Low overhead and easy configuration are the main goals of fancon, this is achieved by:
-  - Use of C++ and optimized STL functions
+Low overhead and easy, meaningful configuration are the main goals of fancon, this is achieved by:
   - Extensive multi-threading support
-  - Standard text file configuration - at /etc/fancon.conf
-  - Fan characteristic testing - allowing more meaningful speed configuration such as through fan RPM, not just PWM control like similar tools
-  - Support for turing off fans (for example, if not under load) and correct handling of required fan PWM for re-starting
+  - Use of C++ and performance profiling
+  - Standard text file configuration - /etc/fancon.conf
+  - Fan characteristic testing - allowing more meaningful speed configuration such as through fan RPM or percentage, not just PWM like similar tools
+  - Speed percentage support (e.g. 65%) rather than cryptic fan-dependent PWM values (e.g. 142)
+  - Fans may be turned off (for example, when system is not under load) with a guaranteed start when they are required
 
 
 ### Installation
-##### Install fancon snap:
-
-Snap [installation instructions](https://snapcraft.io/docs/core/install)
+##### Build from source:
+Tested with both gcc & clang
 
-###### stable
 ```sh
-$ sudo snap install fancon
+$ sudo apt-get install gcc cmake libgcc-6-dev libc6-dev linux-libc-dev libc++-helpers lm-sensors libsensors4-dev libboost-system-dev libboost-filesystem-dev libboost-log-dev libpthread-stubs0-dev libpstreams-dev libsm-dev
+$ sudo apt-get install libxnvctrl-dev libx11-dev
+$ git clone https://github.com/HBriese/fancon.git && cd fancon
+$ mkdir build; cd build && cmake -DCMAKE_BUILD_TYPE=Release .. && make -j && sudo make install
 ```
 
-###### git master
-```sh
-$ sudo snap install fancon --candidate
-```
+| CMake Option     | Default | Description                                                                                 |
+|------------------|---------|---------------------------------------------------------------------------------------------|
+| NVIDIA_SUPPORT   | ON      | Support for NVIDIA GPUs                                                                     |
+| STATIC_LIBSTDC++ | OFF     | Statically link libstdc++ - useful for binary distribution                                  |
+| OPTIMIZE_DEBUG   | OFF     | Enable compiler optimizations on debug build                                                |
+| PROFILE          | OFF     | Support for Google Perf Tools CPU & heap profilers - for debug builds only, due to TCMalloc |
+| LINT             | OFF     | Run lint checker (Clang-Tidy)                                                               |
 
-##### Build from source:
-gcc may be substituted for clang
+##### Install from snap (currently not recommended):
 
+Snap [installation instructions](https://snapcraft.io/docs/core/install)
+
+###### stable
 ```sh
-$ sudo apt-get install gcc cmake libgcc-6-dev libc6-dev linux-libc-dev libc++-helpers lm-sensors libsensors4-dev libboost-system-dev libboost-filesystem-dev libboost-log-dev libpthread-stubs0-dev libpstreams-dev libsm-dev
-$ sudo apt-get install libxnvctrl-dev libx11-dev
-$ git clone https://github.com/HBriese/fancon.git && cd fancon
-$ mkdir build; cd build && cmake -DCMAKE_BUILD_TYPE=Release .. && make && sudo make install
+$ sudo snap install fancon
 ```
-CMake configure options:
-- '-DNVIDIA_SUPPORT=OFF' (Default ON) disable support for NVIDIA GPUs - libxnvctrl-dev & libx11-dev are no longer required
-- '-DSTATIC_LIBSTDC++=ON' Statically link libstdc++, useful for binary distribution to older systems/distributions
 
 ### Contributions
 
 Want to contribute?
 Pull requests, issues and feature requests are welcome.
 
-Contributing developers please see TODO.md, or [email me](mailto:[email protected]?subject=fancon).
+Contributing developers please see TODO.md, or send an [email](mailto:[email protected]?subject=fancon).
 
 The code is formatted to the [LLVM style](http://clang.llvm.org/docs/ClangFormatStyleOptions.html) using clang-format.
 
@@ -58,7 +59,7 @@ The code is formatted to the [LLVM style](http://clang.llvm.org/docs/ClangFormat
 Copyright 2017 Hayden Briese
 
 Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
+you may not use fancon except in compliance with the License.
 You may obtain a copy of the License at
 
     http://www.apache.org/licenses/LICENSE-2.0

TODO.md

@@ -2,6 +2,8 @@
 
 ### Bugs
 
+- Get xauth file & xdisplay from files in /etc/fancon.d/
+
 ### Features
 
 - Add support for AMD GPU fans

src/Controller.cpp

@@ -49,7 +49,7 @@ Controller::Controller(const string &configPath) {
 
     // Find sensors if it has already been defined
     auto sIt = find_if(sensors.begin(), sensors.end(),
-                       [&sensorUID](const unique_ptr<SensorInterface> &s) { return *s == sensorUID; });
+                       [&](const unique_ptr<SensorInterface> &s) { return *s == sensorUID; });
 
     // Add the sensor if it is missing, and update the sensor iterator
     if (sIt == sensors.end()) {
@@ -103,7 +103,7 @@ ControllerState Controller::run() {
 
   threads.shrink_to_fit();
 
-  LOG(llvl::debug) << "fancond started with " << threads.size() << " threads";
+  LOG(llvl::debug) << "Started with " << threads.size() << " threads";
 
   // Synchronize thread wake-up times
   startThreads();

src/FanInterface.cpp

@@ -157,7 +157,7 @@ void FanInterface::verifyPoints(const UID &fanUID) {
 
   /// \return True if <= max or >= min and non-zero
   auto withinBounds =
-      [&invalidPoints, &ignoring](const fan::Point &p, const auto &val, const auto &min, const auto &max) {
+      [&](const fan::Point &p, const auto &val, const auto &min, const auto &max) {
         if (val > max || ((val < min) & (val != 0))) {
           ignoring << ' ' << p;
           return !(invalidPoints = true);
@@ -313,13 +313,21 @@ pwm_t FanInterface::getPWMStart(FanState &state, const milliseconds &waitTime) {
 //  state = FanState::stopped;
 
 //  assert(state == FanState::stopped);
+  // Find PWM at which fan starts
   pwm_t pwmStart = 0;
   while (readRPM() <= 0) {
     writePWM((pwmStart += 5));
     sleep_for(waitTime);
   }
 
-  if (readPWM() != pwmStart)  // pwm has changed since writing it to device
+  // Increase start PWM by an arbitrary amount to ensure start
+  const pwm_t arbInc = 10;
+  if ((pwmStart + arbInc) <= pwm_max_abs)
+    pwmStart += arbInc;
+  else
+    pwmStart = pwm_max_abs;
+
+  if (readPWM() != pwmStart)  // PWM has changed since writing it to device
     LOG(llvl::debug) << "PWM control is not exclusive - this may cause inaccurate testing results";
 
   state = FanState::unknown;

src/Logging.cpp

@@ -1,38 +1,45 @@
 #include "Logging.hpp"
+#include <fstream>
+#include <sys/stat.h>
+
+using std::string;
 
 using namespace boost::log;
 namespace expr = boost::log::expressions;
 
 namespace {
-bool systemdListening(int fileNumber) {
+/// \return True if the file descriptor's device ID and inode number is being accessed by the systemd journal
+bool systemdAccessing(int fileDescriptor) {
+  // Get systemd journal file access list
   const char *jstreamEnv = getenv("JOURNAL_STREAM");
   if (!jstreamEnv)
     return false;
+  string js(jstreamEnv);
 
-  std::string jsEnv(jstreamEnv), stderrFN = std::to_string(fileNumber);
-  for (std::string::iterator it = jsEnv.begin(), endIt = jsEnv.end(), sep;
-       (sep = std::find(it, endIt, ':')) != endIt; it = std::next(sep)) {
-    if (std::string(it, sep) == stderrFN)
-      return true;
-  }
+  // Format of js: `device_id:inode_number`
+  struct stat s;
+  fstat(fileDescriptor, &s);
+  string fd = std::to_string(s.st_dev) + ':' + std::to_string(s.st_ino);
 
-  return false;
+  // Return true if fd is found in js
+  auto it = std::search(js.begin(), js.end(), fd.begin(), fd.end());
+  return it != js.end();
 }
 
 // TODO: replace with function returning a tuple
 template<typename T>
 void setFormatter(boost::shared_ptr<boost::log::sinks::synchronous_sink<T>> sink, bool systemd) {
   if (!systemd)
     sink->set_formatter(
-        expr::format("%1% [%2%] <%3%> - %4%")
+        expr::format("%1% [%2%] <%3%> %4%")
             % expr::format_date_time<boost::posix_time::ptime>(aux::default_attribute_names::timestamp(), "%m/%d %H:%M")
             % getpid()
             % expr::attr<boost::log::trivial::severity_level>(aux::default_attribute_names::severity())
             % expr::smessage
     );
   else
     sink->set_formatter(
-        expr::format("<%1%> - %2%")
+        expr::format("<%1%> %2%")
             % expr::attr<boost::log::trivial::severity_level>(aux::default_attribute_names::severity())
             % expr::smessage
     );
@@ -43,18 +50,19 @@ BOOST_LOG_GLOBAL_LOGGER_INIT(logger, logger_t) {
   logger_t lg;
   core::get()->add_global_attribute(aux::default_attribute_names::timestamp(), attributes::local_clock());
 
-  bool systemd = systemdListening(STDERR_FILENO);
+  bool systemd = systemdAccessing(STDERR_FILENO);
 
-  // Systemd-journal has it's own format attributes, so use a simpler formatter if outputting there
-  if (systemd || (isatty(STDERR_FILENO) > 0)) {
+  // Add cout & cerr sink if they are being by a TTY or systemd's journal
+  if (systemd || isatty(STDERR_FILENO)) {
     // llvl::info -> cout
     auto coutSink = add_console_log(std::cout, keywords::auto_flush = true,  // Unspecified format is "%Message%"
                                     keywords::filter = (trivial::severity == llvl::info));
     core::get()->add_sink(coutSink);
 
-    // non llvl::info -> cerr
+    // != llvl::info -> cerr
     auto cerrSink = add_console_log(std::cerr, keywords::auto_flush = true,
                                     keywords::filter = (trivial::severity != llvl::info));
+    // Systemd-journal has it's own format attributes, so use a simpler formatter when outputting there
     setFormatter(cerrSink, systemd);
     core::get()->add_sink(cerrSink);
   }

src/NvidiaUtil.cpp

@@ -55,9 +55,9 @@ bool NV::DisplayWrapper::open(string da, string xa) {
   if (!(dp = xlib.OpenDisplay(da.c_str()))) {
     stringstream err;
     if (forcedDa)   // da.empty() == true   // TODO: review check - guessed da may be correct
-      err << "Set \"display=\" in " << Util::conf_path << " to your display (echo $" << denv << ')';
+      err << "Set \"display=\" in " << Util::config_path << " to your display (echo $" << denv << ')';
     if (!getenv(xaenv))
-      err << "Set \"xauthority=\" in " << Util::conf_path << " to your .Xauthority file (echo $" << xaenv << ')';
+      err << "Set \"xauthority=\" in " << Util::config_path << " to your .Xauthority file (echo $" << xaenv << ')';
 
     return false;
   }
@@ -96,49 +96,62 @@ bool NV::supported() {
   } else if ((major < 1) || (major == 1 && minor < 9))  // XNVCTRL must be at least v1.9
     LOG(llvl::warning) << "NV-CONTROL X version is not officially supported (too old!); please use v1.9 or higher";
 
+  // Check coolbits value, actual change required restart - ONLY if run in a terminal, so user knows to restart
+  if (isatty(STDERR_FILENO) && enableFanControlCoolbit()) {
+    LOG(llvl::warning) << "RESTART system (or X11) to use NVIDIA fan control - fan control coolbits value enabled";
+    return false;
+  }
+
   return true;
 }
 
-void NV::enableFanControlCoolbit() {
+/// \return True if the system's coolbit value has been changed
+bool NV::enableFanControlCoolbit() {
   // TODO: find and set Coolbits value without 'nvidia-xconfig' - for when not available (e.g. in snap confinement)
-  string command("sudo nvidia-xconfig -t | grep Coolbits");
-  redi::pstream ips(command);
+  redi::pstream ips("sudo nvidia-xconfig -t | grep Coolbits");
   string l;
   std::getline(ips, l);
 
   // Exit early with message if nvidia-xconfig isn't found
   if (l.empty()) {
-    LOG(llvl::info) << "nvidia-xconfig could not be found, either install it, or set coolbits value manually";
-    return;
+    LOG(llvl::info) << "nvidia-xconfig could not be found, either install it, or set the coolbits value manually";
+    return false;
   }
 
-  int iv = Util::lastNum(l);  // Initial value
-  int cv(iv);   // Current val
+  int initv = Util::lastNum(l),
+      curv = initv;
 
   const int nBits = 5;
   const int fcBit = 2;    // 4
-  int cbV[nBits] = {1, 2, 4, 8, 16};  // pow(2, bit)
-  int cbS[nBits]{0};
+  int cbVal[nBits] = {1, 2, 4, 8, 16};  // bit^2
+  bool cbSet[nBits]{0};
 
+  // Determine set coolbit values
   for (auto i = nBits - 1; i >= 0; --i)
-    if ((cbS[i] = (cv / cbV[i]) >= 1))
-      cv -= cbV[i];
+    if ((cbSet[i] = (curv / cbVal[i]) >= 1))
+      curv -= cbVal[i];
 
-  int nv = iv;
-  if (cv > 0) {
+  // Value malformed if there are remaining 'bits'
+  auto newv = initv;
+  if (curv > 0) {
     LOG(llvl::error) << "Invalid coolbits value, fixing with fan control bit set";
-    nv -= cv;
+    newv -= curv;
   }
 
-  if (!cbS[fcBit])
-    nv += cbV[fcBit];
+  // Increment coolbits value if manual fan control coolbit not set
+  if (!cbSet[fcBit])
+    newv += cbVal[fcBit];
 
-  if (nv != iv) {
-    command = string("sudo nvidia-xconfig --cool-bits=") + to_string(nv) + " > /dev/null";
+  // Write new value if changes to the initial value have been made
+  bool ret;
+  if ((ret = newv != initv)) {
+    auto command = string("sudo nvidia-xconfig --cool-bits=") + to_string(newv) + " > /dev/null";
     if (system(command.c_str()) != 0)
-      LOG(llvl::error) << "Failed to write coolbits value, nvidia fan test may fail!";
+      LOG(llvl::error) << "Failed to write coolbits value, nvidia fan control may fail!";
     LOG(llvl::info) << "Reboot, or restart your display server to enable NVIDIA fan control";
   }
+
+  return ret;
 }
 
 int NV::getNumGPUs() {

src/NvidiaUtil.hpp

@@ -84,7 +84,7 @@ struct DisplayWrapper {
 
 extern const bool support;
 bool supported();
-void enableFanControlCoolbit();    // Doesn't work in snap confined
+bool enableFanControlCoolbit();    // Doesn't work in snap confined
 
 int getNumGPUs();
 vector<int> nvProcessBinaryData(const unsigned char *data, const int len);

src/Util.cpp

@@ -22,7 +22,7 @@ int Util::lastNum(const string &str) {
   auto endIt = (endDigRevIt + 1).base();
 
   bool numFound = false;
-  auto begDigRevIt = std::find_if(endDigRevIt, str.rend(), [&numFound](const char &c) {
+  auto begDigRevIt = std::find_if(endDigRevIt, str.rend(), [&](const char &c) {
     if (std::isdigit(c))
       numFound = true;
     return numFound && !std::isdigit(c);
@@ -42,10 +42,10 @@ bool Util::isNum(const string &str) {
 /// \return
 /// True if lock() has been called by a process that is currently running
 bool Util::locked() {
-  if (!exists(pid_file))
+  if (!exists(pid_path))
     return false;
 
-  auto pid = read < pid_t > (pid_file);
+  auto pid = read < pid_t > (pid_path);
   return exists(string("/proc/") + to_string(pid));
 //      && pid != getpid();
 }
@@ -55,7 +55,7 @@ void Util::lock() {
     LOG(llvl::error) << "A fancon process is already running, please close it to continue";
     exit(EXIT_FAILURE);
   } else
-    write(pid_file, getpid());
+    write(pid_path, getpid());
 }
 
 /// \return True if lock is acquired

src/Util.hpp

@@ -41,8 +41,8 @@ fancon::DeviceType operator|(fancon::DeviceType lhs, fancon::DeviceType rhs);
 fancon::DeviceType operator&(fancon::DeviceType lhs, fancon::DeviceType rhs);
 
 namespace Util {
-constexpr const char *pid_file = "/var/run/fancon.pid";
-constexpr const char *conf_path = "/etc/fancon.conf";
+constexpr const char *pid_path = "/var/run/fancon.pid";
+constexpr const char *config_path = "/etc/fancon.conf";
 constexpr const char *fancon_dir = "/etc/fancon.d/";
 constexpr const char *hwmon_path = "/sys/class/hwmon/hwmon";
 constexpr const char *fancon_hwmon_path = "/etc/fancon.d/hwmon";