sensors: split building from manager object
To increase testability, split out the building of sensors
from the sensor manager. And this further splits out building
from a configuration file.
Tested: Verified code continued to build and link.
Tested: Ran on quanta-q71l board and it behaved as expected.
Change-Id: Ib63a11e1107b1864c3c370bba2bd9ef2effa42f3
Signed-off-by: Patrick Venture <venture@google.com>
diff --git a/Makefile.am b/Makefile.am
index ea2a963..8740fb4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,6 +46,8 @@
sysfs/util.cpp \
sensors/pluggable.cpp \
sensors/host.cpp \
+ sensors/builder.cpp \
+ sensors/builderconfig.cpp \
sensors/manager.cpp \
pid/ec/pid.cpp \
pid/controller.cpp \
diff --git a/main.cpp b/main.cpp
index ed1b8c7..6dbd7d6 100644
--- a/main.cpp
+++ b/main.cpp
@@ -35,6 +35,8 @@
/* Controllers & Sensors. */
#include "interfaces.hpp"
#include "pid/zone.hpp"
+#include "sensors/builder.hpp"
+#include "sensors/builderconfig.hpp"
#include "sensors/manager.hpp"
/* Threads. */
diff --git a/sensors/builder.cpp b/sensors/builder.cpp
new file mode 100644
index 0000000..2786141
--- /dev/null
+++ b/sensors/builder.cpp
@@ -0,0 +1,153 @@
+/**
+ * Copyright 2017 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <iostream>
+
+/* Configuration. */
+#include "conf.hpp"
+
+#include "dbus/dbuspassive.hpp"
+#include "interfaces.hpp"
+#include "notimpl/readonly.hpp"
+#include "notimpl/writeonly.hpp"
+#include "sensors/builder.hpp"
+#include "sensors/manager.hpp"
+#include "sensors/host.hpp"
+#include "sensors/pluggable.hpp"
+#include "sysfs/sysfsread.hpp"
+#include "sysfs/sysfswrite.hpp"
+#include "util.hpp"
+
+static constexpr bool deferSignals = true;
+
+std::shared_ptr<SensorManager> BuildSensors(
+ const std::map<std::string, struct sensor>& config)
+{
+ auto mgmr = std::make_shared<SensorManager>();
+ auto& HostSensorBus = mgmr->getHostBus();
+ auto& PassiveListeningBus = mgmr->getPassiveBus();
+
+ for (auto& it : config)
+ {
+ std::unique_ptr<ReadInterface> ri;
+ std::unique_ptr<WriteInterface> wi;
+
+ std::string name = it.first;
+ const struct sensor* info = &it.second;
+
+ std::cerr << "Sensor: " << name << " " << info->type << " ";
+ std::cerr << info->readpath << " " << info->writepath << "\n";
+
+ IOInterfaceType rtype = GetReadInterfaceType(info->readpath);
+ IOInterfaceType wtype = GetWriteInterfaceType(info->writepath);
+
+ // fan sensors can be ready any way and written others.
+ // fan sensors are the only sensors this is designed to write.
+ // Nothing here should be write-only, although, in theory a fan could be.
+ // I'm just not sure how that would fit together.
+ // TODO(venture): It should check with the ObjectMapper to check if
+ // that sensor exists on the Dbus.
+ switch (rtype)
+ {
+ case IOInterfaceType::DBUSPASSIVE:
+ ri = std::make_unique<DbusPassive>(
+ PassiveListeningBus,
+ info->type,
+ name);
+ break;
+ case IOInterfaceType::EXTERNAL:
+ // These are a special case for read-only.
+ break;
+ case IOInterfaceType::SYSFS:
+ ri = std::make_unique<SysFsRead>(info->readpath);
+ break;
+ default:
+ ri = std::make_unique<WriteOnly>();
+ break;
+ }
+
+ if (info->type == "fan")
+ {
+ switch (wtype)
+ {
+ case IOInterfaceType::SYSFS:
+ if (info->max > 0)
+ {
+ wi = std::make_unique<SysFsWritePercent>(
+ info->writepath,
+ info->min,
+ info->max);
+ }
+ else
+ {
+ wi = std::make_unique<SysFsWrite>(
+ info->writepath,
+ info->min,
+ info->max);
+ }
+
+ break;
+ default:
+ wi = std::make_unique<ReadOnlyNoExcept>();
+ break;
+ }
+
+ auto sensor = std::make_unique<PluggableSensor>(
+ name,
+ info->timeout,
+ std::move(ri),
+ std::move(wi));
+ mgmr->addSensor(info->type, name, std::move(sensor));
+ }
+ else if (info->type == "temp" || info->type == "margin")
+ {
+ // These sensors are read-only, but only for this application
+ // which only writes to fan sensors.
+ std::cerr << info->type << " readpath: " << info->readpath << "\n";
+
+ if (IOInterfaceType::EXTERNAL == rtype)
+ {
+ std::cerr << "Creating HostSensor: " << name
+ << " path: " << info->readpath << "\n";
+
+ /*
+ * The reason we handle this as a HostSensor is because it's
+ * not quite pluggable; but maybe it could be.
+ */
+ auto sensor = HostSensor::CreateTemp(
+ name,
+ info->timeout,
+ HostSensorBus,
+ info->readpath.c_str(),
+ deferSignals);
+ mgmr->addSensor(info->type, name, std::move(sensor));
+ }
+ else
+ {
+ wi = std::make_unique<ReadOnlyNoExcept>();
+ auto sensor = std::make_unique<PluggableSensor>(
+ name,
+ info->timeout,
+ std::move(ri),
+ std::move(wi));
+ mgmr->addSensor(info->type, name, std::move(sensor));
+ }
+ }
+ }
+
+ return mgmr;
+}
+
diff --git a/sensors/builder.hpp b/sensors/builder.hpp
new file mode 100644
index 0000000..2849c71
--- /dev/null
+++ b/sensors/builder.hpp
@@ -0,0 +1,15 @@
+#pragma once
+
+#include <map>
+#include <memory>
+#include <string>
+
+#include "sensors/manager.hpp"
+#include "sensors/sensor.hpp"
+
+/**
+ * Build the sensors and associate them with a SensorManager.
+ */
+std::shared_ptr<SensorManager> BuildSensors(
+ const std::map<std::string, struct sensor>& config);
+
diff --git a/sensors/builderconfig.cpp b/sensors/builderconfig.cpp
new file mode 100644
index 0000000..b3507ae
--- /dev/null
+++ b/sensors/builderconfig.cpp
@@ -0,0 +1,114 @@
+/**
+ * Copyright 2017 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <iostream>
+#include <libconfig.h++>
+
+/* Configuration. */
+#include "conf.hpp"
+
+#include "sensors/builder.hpp"
+#include "sensors/manager.hpp"
+
+/*
+ * If there's a configuration file, we build from that, and it requires special
+ * parsing. I should just ditch the compile-time version to reduce the
+ * probability of sync bugs.
+ */
+std::shared_ptr<SensorManager> BuildSensorsFromConfig(const std::string& path)
+{
+ using namespace libconfig;
+
+ std::map<std::string, struct sensor> config;
+ Config cfg;
+
+ std::cerr << "entered BuildSensorsFromConfig\n";
+
+ /* The load was modeled after the example source provided. */
+ try
+ {
+ cfg.readFile(path.c_str());
+ }
+ catch (const FileIOException& fioex)
+ {
+ std::cerr << "I/O error while reading file: " << fioex.what() << std::endl;
+ throw;
+ }
+ catch (const ParseException& pex)
+ {
+ std::cerr << "Parse error at " << pex.getFile() << ":" << pex.getLine()
+ << " - " << pex.getError() << std::endl;
+ throw;
+ }
+
+ try
+ {
+ const Setting& root = cfg.getRoot();
+
+ /* Grab the list of sensors and create them all */
+ const Setting& sensors = root["sensors"];
+ int count = sensors.getLength();
+
+ for (int i = 0; i < count; ++i)
+ {
+ const Setting& sensor = sensors[i];
+
+ std::string name;
+ struct sensor thisOne;
+
+ /* Not a super fan of using this library for run-time configuration. */
+ name = sensor.lookup("name").c_str();
+ thisOne.type = sensor.lookup("type").c_str();
+ thisOne.readpath = sensor.lookup("readpath").c_str();
+ thisOne.writepath = sensor.lookup("writepath").c_str();
+
+ /* TODO: Document why this is wonky. The library probably doesn't
+ * like int64_t
+ */
+ int min = sensor.lookup("min");
+ thisOne.min = static_cast<int64_t>(min);
+ int max = sensor.lookup("max");
+ thisOne.max = static_cast<int64_t>(max);
+ int timeout = sensor.lookup("timeout");
+ thisOne.timeout = static_cast<int64_t>(timeout);
+
+ // leaving for verification for now. and yea the above is
+ // necessary.
+ std::cerr << "min: " << min
+ << " max: " << max
+ << " savedmin: " << thisOne.min
+ << " savedmax: " << thisOne.max
+ << " timeout: " << thisOne.timeout
+ << std::endl;
+
+ config[name] = thisOne;
+ }
+ }
+ catch (const SettingTypeException &setex)
+ {
+ std::cerr << "Setting '" << setex.getPath()
+ << "' type exception!" << std::endl;
+ throw;
+ }
+ catch (const SettingNotFoundException& snex)
+ {
+ std::cerr << "Setting not found!" << std::endl;
+ throw;
+ }
+
+ return BuildSensors(config);
+}
+
diff --git a/sensors/builderconfig.hpp b/sensors/builderconfig.hpp
new file mode 100644
index 0000000..078f484
--- /dev/null
+++ b/sensors/builderconfig.hpp
@@ -0,0 +1,12 @@
+#pragma once
+
+#include <memory>
+#include <string>
+
+#include "sensors/manager.hpp"
+
+/**
+ * Given a configuration file, parsable by libconfig++, parse it and then pass
+ * the information onto BuildSensors.
+ */
+std::shared_ptr<SensorManager> BuildSensorsFromConfig(const std::string& path);
diff --git a/sensors/manager.cpp b/sensors/manager.cpp
index 590ca80..fad2c33 100644
--- a/sensors/manager.cpp
+++ b/sensors/manager.cpp
@@ -14,235 +14,23 @@
* limitations under the License.
*/
-#include <cstring>
-#include <iostream>
-#include <libconfig.h++>
-#include <map>
-#include <memory>
-
/* Configuration. */
#include "conf.hpp"
-#include "interfaces.hpp"
-#include "manager.hpp"
-#include "util.hpp"
-
-#include "dbus/dbuspassive.hpp"
-#include "notimpl/readonly.hpp"
-#include "notimpl/writeonly.hpp"
-#include "sysfs/sysfsread.hpp"
#include "sensors/manager.hpp"
-#include "sensors/host.hpp"
-#include "sensors/pluggable.hpp"
-#include "sysfs/sysfswrite.hpp"
-
-static constexpr bool deferSignals = true;
-
-std::shared_ptr<SensorManager> BuildSensors(
- std::map<std::string, struct sensor>& Config)
+void SensorManager::addSensor(
+ std::string type,
+ std::string name,
+ std::unique_ptr<Sensor> sensor)
{
- auto mgmr = std::make_shared<SensorManager>();
- auto& HostSensorBus = mgmr->getHostBus();
- auto& PassiveListeningBus = mgmr->getPassiveBus();
+ _sensorMap[name] = std::move(sensor);
- for (auto& it : Config)
+ auto entry = _sensorTypeList.find(type);
+ if (entry == _sensorTypeList.end())
{
- std::unique_ptr<ReadInterface> ri;
- std::unique_ptr<WriteInterface> wi;
-
- std::string name = it.first;
- struct sensor* info = &it.second;
-
- std::cerr << "Sensor: " << name << " " << info->type << " ";
- std::cerr << info->readpath << " " << info->writepath << "\n";
-
- IOInterfaceType rtype = GetReadInterfaceType(info->readpath);
- IOInterfaceType wtype = GetWriteInterfaceType(info->writepath);
-
- // fan sensors can be ready any way and written others.
- // fan sensors are the only sensors this is designed to write.
- // Nothing here should be write-only, although, in theory a fan could be.
- // I'm just not sure how that would fit together.
- // TODO(venture): It should check with the ObjectMapper to check if
- // that sensor exists on the Dbus.
- switch (rtype)
- {
- case IOInterfaceType::DBUSPASSIVE:
- ri = std::make_unique<DbusPassive>(
- PassiveListeningBus,
- info->type,
- name);
- break;
- case IOInterfaceType::EXTERNAL:
- // These are a special case for read-only.
- break;
- case IOInterfaceType::SYSFS:
- ri = std::make_unique<SysFsRead>(info->readpath);
- break;
- default:
- ri = std::make_unique<WriteOnly>();
- break;
- }
-
- if (info->type == "fan")
- {
- switch (wtype)
- {
- case IOInterfaceType::SYSFS:
- if (info->max > 0)
- {
- wi = std::make_unique<SysFsWritePercent>(
- info->writepath,
- info->min,
- info->max);
- }
- else
- {
- wi = std::make_unique<SysFsWrite>(
- info->writepath,
- info->min,
- info->max);
- }
-
- break;
- default:
- wi = std::make_unique<ReadOnlyNoExcept>();
- break;
- }
-
- auto sensor = std::make_unique<PluggableSensor>(
- name,
- info->timeout,
- std::move(ri),
- std::move(wi));
- mgmr->addSensor(info->type, name, std::move(sensor));
- }
- else if (info->type == "temp" || info->type == "margin")
- {
- // These sensors are read-only, but only for this application
- // which only writes to fan sensors.
- std::cerr << info->type << " readpath: " << info->readpath << "\n";
-
- if (IOInterfaceType::EXTERNAL == rtype)
- {
- std::cerr << "Creating HostSensor: " << name
- << " path: " << info->readpath << "\n";
-
- /*
- * The reason we handle this as a HostSensor is because it's
- * not quite pluggable; but maybe it could be.
- */
- auto sensor = HostSensor::CreateTemp(
- name,
- info->timeout,
- HostSensorBus,
- info->readpath.c_str(),
- deferSignals);
- mgmr->addSensor(info->type, name, std::move(sensor));
- }
- else
- {
- wi = std::make_unique<ReadOnlyNoExcept>();
- auto sensor = std::make_unique<PluggableSensor>(
- name,
- info->timeout,
- std::move(ri),
- std::move(wi));
- mgmr->addSensor(info->type, name, std::move(sensor));
- }
- }
+ _sensorTypeList[type] = {};
}
- return mgmr;
+ _sensorTypeList[type].push_back(name);
}
-
-/*
- * If there's a configuration file, we build from that, and it requires special
- * parsing. I should just ditch the compile-time version to reduce the
- * probability of sync bugs.
- */
-std::shared_ptr<SensorManager> BuildSensorsFromConfig(std::string& path)
-{
- using namespace libconfig;
-
- std::map<std::string, struct sensor> config;
- Config cfg;
-
- std::cerr << "entered BuildSensorsFromConfig\n";
-
- /* The load was modeled after the example source provided. */
- try
- {
- cfg.readFile(path.c_str());
- }
- catch (const FileIOException& fioex)
- {
- std::cerr << "I/O error while reading file: " << fioex.what() << std::endl;
- throw;
- }
- catch (const ParseException& pex)
- {
- std::cerr << "Parse error at " << pex.getFile() << ":" << pex.getLine()
- << " - " << pex.getError() << std::endl;
- throw;
- }
-
- try
- {
- const Setting& root = cfg.getRoot();
-
- /* Grab the list of sensors and create them all */
- const Setting& sensors = root["sensors"];
- int count = sensors.getLength();
-
- for (int i = 0; i < count; ++i)
- {
- const Setting& sensor = sensors[i];
-
- std::string name;
- struct sensor thisOne;
-
- /* Not a super fan of using this library for run-time configuration. */
- name = sensor.lookup("name").c_str();
- thisOne.type = sensor.lookup("type").c_str();
- thisOne.readpath = sensor.lookup("readpath").c_str();
- thisOne.writepath = sensor.lookup("writepath").c_str();
-
- /* TODO: Document why this is wonky. The library probably doesn't
- * like int64_t
- */
- int min = sensor.lookup("min");
- thisOne.min = static_cast<int64_t>(min);
- int max = sensor.lookup("max");
- thisOne.max = static_cast<int64_t>(max);
- int timeout = sensor.lookup("timeout");
- thisOne.timeout = static_cast<int64_t>(timeout);
-
- // leaving for verification for now. and yea the above is
- // necessary.
- std::cerr << "min: " << min
- << " max: " << max
- << " savedmin: " << thisOne.min
- << " savedmax: " << thisOne.max
- << " timeout: " << thisOne.timeout
- << std::endl;
-
- config[name] = thisOne;
- }
- }
- catch (const SettingTypeException &setex)
- {
- std::cerr << "Setting '" << setex.getPath()
- << "' type exception!" << std::endl;
- throw;
- }
- catch (const SettingNotFoundException& snex)
- {
- std::cerr << "Setting not found!" << std::endl;
- throw;
- }
-
- return BuildSensors(config);
-}
-
diff --git a/sensors/manager.hpp b/sensors/manager.hpp
index d466039..a3e7420 100644
--- a/sensors/manager.hpp
+++ b/sensors/manager.hpp
@@ -32,18 +32,7 @@
void addSensor(
std::string type,
std::string name,
- std::unique_ptr<Sensor> sensor)
- {
- _sensorMap[name] = std::move(sensor);
-
- auto entry = _sensorTypeList.find(type);
- if (entry == _sensorTypeList.end())
- {
- _sensorTypeList[type] = {};
- }
-
- _sensorTypeList[type].push_back(name);
- }
+ std::unique_ptr<Sensor> sensor);
// TODO(venture): Should implement read/write by name.
std::unique_ptr<Sensor>& getSensor(std::string name)
@@ -69,7 +58,3 @@
sdbusplus::bus::bus _hostSensorBus;
};
-std::shared_ptr<SensorManager> BuildSensors(
- std::map<std::string, struct sensor>& Config);
-
-std::shared_ptr<SensorManager> BuildSensorsFromConfig(std::string& path);