dbusconfiguration: add support for one-to-one fan/pwm mapping
Problem: in a fan pid only the last pwm from "Outputs" in the fan pid
configuration would be applied to all the fans listed in "Inputs". This
is an error since "Outputs" is a list of strings, not a single string.
Solution: If Class=="fan" && InputInterfaces.size() ==
OutputInterfaces.size() then elaborate the sensors on a one Input to one
Output basis.
Else if Class=="fan" && OutputInterfaces.size() == 1 then:
do the old behavior where all the fans in Inputs have their writepaths
set to the one pwm interface.
Else:
This is an error condition so throw an exception
Additional changes:
Improve readability: for clarity "Inputs" and "Outputs" have been split into
separate datastructures. The same was done for "SensorInterfaces". This
makes it easier to understand what is going on during sensor
elaboration.
Improve error handling: For fans there should be one output or the
number of outputs should match inputs. If this isn't the case then the
configuration isn't valid and the system should fail.
Tested:
fragment of dbus configuration used:
{
"Class": "fan",
"Name": "fan0_pid",
...
"Inputs": [
"fan0_tach", "fan1_tach", "fan2_tach"
],
...
"Outputs": [
"fan0_pwm","fan1_pwm", "fan2_pwm"
],
},
Without Change: see in the sensor config the last element of Outputs is used to drive all the fans.
Additionally fan pwms are added to the sensor configuration which looks to be incorrect.
Only a single sensor node is needed for fans and fan tachs are chosen for this purpose.
{fan0_pwm,
{, , /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0},
},
{fan0_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan0_tach, /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0},
},
{fan1_pwm,
{, , /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0},
},
{fan1_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan1_tach, /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0},
},
{fan2_pwm,
{, , /xyz/openbmc_project/control/fanpwm/fan1_pwm, 0, 255, 0},
},
{fan2_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan2_tach, /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0},
},
With Change: see in the sensor config the correct association for a fan_tach sensor. Also the fan_pwms are trimmed.
sensor config:
{
{fan0_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan0_tach, /xyz/openbmc_project/control/fanpwm/fan0_pwm, 0, 255, 0}
},
{fan1_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan1_tach, /xyz/openbmc_project/control/fanpwm/fan1_pwm, 0, 255, 0}
},
{fan2_tach,
{fan, /xyz/openbmc_project/sensors/fan_tach/fan2_tach, /xyz/openbmc_project/control/fanpwm/fan2_pwm, 0, 255, 0}
},
}
Signed-off-by: Jason Ling <jasonling@google.com>
Change-Id: Id4494028752026a5ba8f0c1d8081276e0bf4be90
diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp
index e1ca91c..c1279a4 100644
--- a/dbus/dbusconfiguration.cpp
+++ b/dbus/dbusconfiguration.cpp
@@ -67,10 +67,22 @@
namespace dbus_configuration
{
-
using DbusVariantType =
std::variant<uint64_t, int64_t, double, std::string,
std::vector<std::string>, std::vector<double>>;
+using SensorInterfaceType = std::pair<std::string, std::string>;
+
+inline std::string getSensorNameFromPath(const std::string& dbusPath)
+{
+ return dbusPath.substr(dbusPath.find_last_of("/") + 1);
+}
+
+inline std::string sensorNameToDbusName(const std::string& sensorName)
+{
+ std::string retString = sensorName;
+ std::replace(retString.begin(), retString.end(), ' ', '_');
+ return retString;
+}
bool findSensors(const std::unordered_map<std::string, std::string>& sensors,
const std::string& search,
@@ -628,89 +640,160 @@
zone.at("FailSafePercent"));
}
auto findBase = configuration.second.find(pidConfigurationInterface);
+ // loop through all the PID configurations and fill out a sensor config
if (findBase != configuration.second.end())
{
const auto& base =
configuration.second.at(pidConfigurationInterface);
+ const std::string pidName = std::get<std::string>(base.at("Name"));
+ const std::string pidClass =
+ std::get<std::string>(base.at("Class"));
const std::vector<std::string>& zones =
std::get<std::vector<std::string>>(base.at("Zones"));
for (const std::string& zone : zones)
{
size_t index = getZoneIndex(zone, foundZones);
conf::PIDConf& conf = zoneConfig[index];
+ std::vector<std::string> inputSensorNames(
+ std::get<std::vector<std::string>>(base.at("Inputs")));
+ std::vector<std::string> outputSensorNames;
- std::vector<std::string> sensorNames =
- std::get<std::vector<std::string>>(base.at("Inputs"));
- auto findOutputs =
- base.find("Outputs"); // currently only fans have outputs
- if (findOutputs != base.end())
+ // assumption: all fan pids must have at least one output
+ if (pidClass == "fan")
{
- std::vector<std::string> outputs =
- std::get<std::vector<std::string>>(findOutputs->second);
- sensorNames.insert(sensorNames.end(), outputs.begin(),
- outputs.end());
+ outputSensorNames = std::get<std::vector<std::string>>(
+ getPIDAttribute(base, "Outputs"));
}
- std::vector<std::string> inputs;
- std::vector<std::pair<std::string, std::string>>
- sensorInterfaces;
- for (const std::string& sensorName : sensorNames)
+ std::vector<SensorInterfaceType> inputSensorInterfaces;
+ std::vector<SensorInterfaceType> outputSensorInterfaces;
+ /* populate an interface list for different sensor direction
+ * types (input,output)
+ */
+ /* take the Inputs from the configuration and generate
+ * a list of dbus descriptors (path, interface).
+ * Mapping can be many-to-one since an element of Inputs can be
+ * a regex
+ */
+ for (const std::string& sensorName : inputSensorNames)
{
- std::string name = sensorName;
- // replace spaces with underscores to be legal on dbus
- std::replace(name.begin(), name.end(), ' ', '_');
- findSensors(sensors, name, sensorInterfaces);
+ findSensors(sensors, sensorNameToDbusName(sensorName),
+ inputSensorInterfaces);
+ }
+ for (const std::string& sensorName : outputSensorNames)
+ {
+ findSensors(sensors, sensorNameToDbusName(sensorName),
+ outputSensorInterfaces);
}
- for (const auto& sensorPathIfacePair : sensorInterfaces)
+ inputSensorNames.clear();
+ for (const SensorInterfaceType& inputSensorInterface :
+ inputSensorInterfaces)
{
-
- if (sensorPathIfacePair.second == sensorInterface)
+ const std::string& dbusInterface =
+ inputSensorInterface.second;
+ const std::string& inputSensorPath =
+ inputSensorInterface.first;
+ std::string inputSensorName =
+ getSensorNameFromPath(inputSensorPath);
+ auto& config = sensorConfig[inputSensorName];
+ inputSensorNames.push_back(inputSensorName);
+ config.type = pidClass;
+ config.readPath = inputSensorInterface.first;
+ // todo: maybe un-hardcode this if we run into slower
+ // timeouts with sensors
+ if (config.type == "temp")
{
- size_t idx =
- sensorPathIfacePair.first.find_last_of("/") + 1;
- std::string shortName =
- sensorPathIfacePair.first.substr(idx);
-
- inputs.push_back(shortName);
- auto& config = sensorConfig[shortName];
- config.type = std::get<std::string>(base.at("Class"));
- config.readPath = sensorPathIfacePair.first;
- // todo: maybe un-hardcode this if we run into slower
- // timeouts with sensors
- if (config.type == "temp")
- {
- config.timeout = 0;
- config.ignoreDbusMinMax = true;
- }
+ config.timeout = 0;
+ config.ignoreDbusMinMax = true;
}
- else if (sensorPathIfacePair.second == pwmInterface)
+ if (dbusInterface != sensorInterface)
{
- // copy so we can modify it
- for (std::string otherSensor : sensorNames)
- {
- std::replace(otherSensor.begin(), otherSensor.end(),
- ' ', '_');
- if (sensorPathIfacePair.first.find(otherSensor) !=
- std::string::npos)
- {
- continue;
- }
-
- auto& config = sensorConfig[otherSensor];
- config.writePath = sensorPathIfacePair.first;
- // todo: un-hardcode this if there are fans with
- // different ranges
- config.max = 255;
- config.min = 0;
- }
+ /* all expected inputs in the configuration are expected
+ * to be sensor interfaces
+ */
+ throw std::runtime_error(
+ "sensor at dbus path [" + inputSensorPath +
+ "] has an interface [" + dbusInterface +
+ "] that does not match the expected interface of " +
+ sensorInterface);
}
}
+ /* fan pids need to pair up tach sensors with their pwm
+ * counterparts
+ */
+ if (pidClass == "fan")
+ {
+ /* If a PID is a fan there should be either
+ * (1) one output(pwm) per input(tach)
+ * OR
+ * (2) one putput(pwm) for all inputs(tach)
+ * everything else indicates a bad configuration.
+ */
+ bool singlePwm = false;
+ if (outputSensorInterfaces.size() == 1)
+ {
+ /* one pwm, set write paths for all fan sensors to it */
+ singlePwm = true;
+ }
+ else if (inputSensorInterfaces.size() ==
+ outputSensorInterfaces.size())
+ {
+ /* one to one mapping, each fan sensor gets its own pwm
+ * control */
+ singlePwm = false;
+ }
+ else
+ {
+ throw std::runtime_error(
+ "fan PID has invalid number of Outputs");
+ }
+ std::string fanSensorName;
+ std::string pwmPath;
+ std::string pwmInterface;
+ if (singlePwm)
+ {
+ /* if just a single output(pwm) is provided then use
+ * that pwm control path for all the fan sensor write
+ * path configs
+ */
+ pwmPath = outputSensorInterfaces.at(0).first;
+ pwmInterface = outputSensorInterfaces.at(0).second;
+ }
+ for (uint32_t idx = 0; idx < inputSensorInterfaces.size();
+ idx++)
+ {
+ if (!singlePwm)
+ {
+ pwmPath = outputSensorInterfaces.at(idx).first;
+ pwmInterface =
+ outputSensorInterfaces.at(idx).second;
+ }
+ if (pwmInterface != pwmInterface)
+ {
+ throw std::runtime_error(
+ "fan pwm control at dbus path [" + pwmPath +
+ "] has an interface [" + pwmInterface +
+ "] that does not match the expected interface "
+ "of " +
+ pwmInterface);
+ }
+ const std::string& fanPath =
+ inputSensorInterfaces.at(idx).first;
+ fanSensorName = getSensorNameFromPath(fanPath);
+ auto& fanConfig = sensorConfig[fanSensorName];
+ fanConfig.writePath = pwmPath;
+ // todo: un-hardcode this if there are fans with
+ // different ranges
+ fanConfig.max = 255;
+ fanConfig.min = 0;
+ }
+ }
// if the sensors aren't available in the current state, don't
// add them to the configuration.
- if (inputs.empty())
+ if (inputSensorNames.empty())
{
continue;
}
@@ -738,14 +821,14 @@
{
struct conf::ControllerInfo& info =
conf[std::get<std::string>(base.at("Name"))];
- info.inputs = std::move(inputs);
+ info.inputs = std::move(inputSensorNames);
populatePidInfo(bus, base, info, nullptr);
}
else
{
// we have to split up the inputs, as in practice t-control
// values will differ, making setpoints differ
- for (const std::string& input : inputs)
+ for (const std::string& input : inputSensorNames)
{
struct conf::ControllerInfo& info = conf[input];
info.inputs.emplace_back(input);
@@ -773,13 +856,10 @@
bool sensorFound = false;
for (const std::string& sensorName : sensorNames)
{
- std::string name = sensorName;
- // replace spaces with underscores to be legal on dbus
- std::replace(name.begin(), name.end(), ' ', '_');
std::vector<std::pair<std::string, std::string>>
sensorPathIfacePairs;
-
- if (!findSensors(sensors, name, sensorPathIfacePairs))
+ if (!findSensors(sensors, sensorNameToDbusName(sensorName),
+ sensorPathIfacePairs))
{
break;
}