Standardize read errors
Each sensor handled read errors their own way,
making it inconsistant. This helps to align them all
in the base class, so that error handling happens the
same for each sensor. It also aligns the power state
change handling.
Tested: Tested DC Cycling and Enabling/Disabling ME to
make sure functional change correctly
Change-Id: I1a191d27629602e1ca3871d933af07b15bf9f331
Signed-off-by: James Feist <james.feist@linux.intel.com>
diff --git a/src/ADCSensor.cpp b/src/ADCSensor.cpp
index bfe17ac..20e8041 100644
--- a/src/ADCSensor.cpp
+++ b/src/ADCSensor.cpp
@@ -54,11 +54,12 @@
std::optional<BridgeGpio>&& bridgeGpio) :
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration,
- "xyz.openbmc_project.Configuration.ADC", maxReading, minReading),
+ "xyz.openbmc_project.Configuration.ADC", maxReading, minReading,
+ readState),
std::enable_shared_from_this<ADCSensor>(), objServer(objectServer),
inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path),
- errCount(0), scaleFactor(scaleFactor), bridgeGpio(std::move(bridgeGpio)),
- readState(std::move(readState)), thresholdTimer(io, this)
+ scaleFactor(scaleFactor), bridgeGpio(std::move(bridgeGpio)),
+ thresholdTimer(io, this)
{
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/voltage/" + name,
@@ -78,9 +79,6 @@
association = objectServer.add_interface(
"/xyz/openbmc_project/sensors/voltage/" + name, association::interface);
setInitialProperties(conn);
-
- // setup match
- setupPowerMatch(conn);
}
ADCSensor::~ADCSensor()
@@ -174,32 +172,15 @@
nvalue = std::round(nvalue * roundFactor) / roundFactor;
updateValue(nvalue);
- errCount = 0;
}
catch (std::invalid_argument&)
{
- errCount++;
+ incrementError();
}
}
- else if (readState == PowerState::on && !isPowerOn())
- {
- errCount = 0;
- updateValue(std::numeric_limits<double>::quiet_NaN());
- }
else
{
- errCount++;
- }
- // only print once
- if (errCount == warnAfterErrorCount)
- {
- std::cerr << "Failure to read sensor " << name << " at " << path
- << " ec:" << err << "\n";
- }
-
- if (errCount >= warnAfterErrorCount)
- {
- updateValue(0);
+ incrementError();
}
responseStream.clear();
@@ -231,7 +212,7 @@
void ADCSensor::checkThresholds(void)
{
- if (readState == PowerState::on && !isPowerOn())
+ if (!readingStateGood())
{
return;
}
diff --git a/src/CPUSensor.cpp b/src/CPUSensor.cpp
index afc4706..bf2b6aa 100644
--- a/src/CPUSensor.cpp
+++ b/src/CPUSensor.cpp
@@ -43,11 +43,11 @@
bool show, double dtsOffset) :
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration, objectType, maxReading,
- minReading),
+ minReading, PowerState::on),
objServer(objectServer), inputDev(io, open(path.c_str(), O_RDONLY)),
waitTimer(io), path(path),
privTcontrol(std::numeric_limits<double>::quiet_NaN()),
- dtsOffset(dtsOffset), show(show), errCount(0)
+ dtsOffset(dtsOffset), show(show)
{
nameTcontrol = labelTcontrol;
nameTcontrol += " CPU" + std::to_string(cpuId);
@@ -87,6 +87,8 @@
setInitialProperties(conn);
}
}
+
+ // call setup always as not all sensors call setInitialProperties
setupPowerMatch(conn);
setupRead();
}
@@ -223,52 +225,16 @@
}
}
}
- errCount = 0;
}
catch (const std::invalid_argument&)
{
- errCount++;
+ incrementError();
}
}
else
{
pollTime = sensorFailedPollTimeMs;
- errCount++;
- }
-
- if (errCount >= warnAfterErrorCount)
- {
- // only an error if power is on
- if (isPowerOn())
- {
- // only print once
- if (errCount == warnAfterErrorCount)
- {
- std::cerr << "Failure to read sensor " << name << " at " << path
- << "\n";
- }
- if (show)
- {
- updateValue(0);
- }
- else
- {
- value = 0;
- }
- errCount++;
- }
- else
- {
- errCount = 0; // check power again in 10 cycles
- if (show)
- {
- updateValue(std::numeric_limits<double>::quiet_NaN());
- }
- else
- {
- value = std::numeric_limits<double>::quiet_NaN();
- }
- }
+ incrementError();
}
responseStream.clear();
diff --git a/src/ExitAirTempSensor.cpp b/src/ExitAirTempSensor.cpp
index 70c7bae..7197c4a 100644
--- a/src/ExitAirTempSensor.cpp
+++ b/src/ExitAirTempSensor.cpp
@@ -166,7 +166,7 @@
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(thresholdData), sensorConfiguration,
"xyz.openbmc_project.Configuration.ExitAirTemp", cfmMaxReading,
- cfmMinReading),
+ cfmMinReading, PowerState::on),
std::enable_shared_from_this<CFMSensor>(), parent(parent),
dbusConnection(conn), objServer(objectServer)
{
@@ -494,7 +494,7 @@
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(thresholdData), sensorConfiguration,
"xyz.openbmc_project.Configuration.ExitAirTemp", exitAirMaxReading,
- exitAirMinReading),
+ exitAirMinReading, PowerState::on),
std::enable_shared_from_this<ExitAirTempSensor>(), dbusConnection(conn),
objServer(objectServer)
{
@@ -518,7 +518,6 @@
"/xyz/openbmc_project/sensors/temperature/" + name,
association::interface);
setInitialProperties(conn);
- setupPowerMatch(conn);
}
ExitAirTempSensor::~ExitAirTempSensor()
diff --git a/src/HwmonTempSensor.cpp b/src/HwmonTempSensor.cpp
index 71ff4ed..38bc96b 100644
--- a/src/HwmonTempSensor.cpp
+++ b/src/HwmonTempSensor.cpp
@@ -47,10 +47,9 @@
const std::string& sensorConfiguration, const PowerState powerState) :
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration, objectType, maxReading,
- minReading),
+ minReading, powerState),
std::enable_shared_from_this<HwmonTempSensor>(), objServer(objectServer),
- inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path),
- errCount(0), readState(powerState)
+ inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path)
{
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/temperature/" + name,
@@ -72,7 +71,6 @@
"/xyz/openbmc_project/sensors/temperature/" + name,
association::interface);
setInitialProperties(conn);
- setupPowerMatch(conn);
}
HwmonTempSensor::~HwmonTempSensor()
@@ -119,34 +117,17 @@
double nvalue = std::stod(response);
nvalue /= sensorScaleFactor;
updateValue(nvalue);
- errCount = 0;
}
catch (const std::invalid_argument&)
{
- errCount++;
+ incrementError();
}
}
- else if (readState == PowerState::on && !isPowerOn())
- {
- errCount = 0;
- updateValue(std::numeric_limits<double>::quiet_NaN());
- }
else
{
- errCount++;
+ incrementError();
}
- // only print once
- if (errCount == warnAfterErrorCount)
- {
- std::cerr << "Failure to read sensor " << name << " at " << path
- << " ec:" << err << "\n";
- }
-
- if (errCount >= warnAfterErrorCount)
- {
- updateValue(0);
- }
responseStream.clear();
inputDev.close();
int fd = open(path.c_str(), O_RDONLY);
@@ -172,9 +153,5 @@
void HwmonTempSensor::checkThresholds(void)
{
- if (readState == PowerState::on && !isPowerOn())
- {
- return;
- }
thresholds::checkThresholds(this);
}
diff --git a/src/IpmbSensor.cpp b/src/IpmbSensor.cpp
index f36e836..d8b0d4f 100644
--- a/src/IpmbSensor.cpp
+++ b/src/IpmbSensor.cpp
@@ -69,9 +69,9 @@
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(thresholdData), sensorConfiguration,
"xyz.openbmc_project.Configuration.ExitAirTemp", ipmbMaxReading,
- ipmbMinReading),
- deviceAddress(deviceAddress), readState(PowerState::on),
- objectServer(objectServer), dbusConnection(conn), waitTimer(io)
+ ipmbMinReading, PowerState::on),
+ deviceAddress(deviceAddress), objectServer(objectServer),
+ dbusConnection(conn), waitTimer(io)
{
std::string dbusPath = sensorPathPrefix + sensorTypeName + "/" + name;
@@ -89,7 +89,6 @@
dbusPath, "xyz.openbmc_project.Sensor.Threshold.Critical");
}
association = objectServer.add_interface(dbusPath, association::interface);
- setupPowerMatch(conn);
}
IpmbSensor::~IpmbSensor()
@@ -226,68 +225,55 @@
void IpmbSensor::checkThresholds(void)
{
- if (readState == PowerState::on && !isPowerOn())
- {
- return;
- }
- else if (readState == PowerState::biosPost && !hasBiosPost())
- {
- return;
- }
thresholds::checkThresholds(this);
}
-void IpmbSensor::processError(void)
-{
- static constexpr size_t errorFilter = 2;
-
- if (!errorCount)
- {
- std::cerr << "Invalid data from device: " << name << "\n";
- }
- else if (errorCount > errorFilter)
- {
- updateValue(0);
- }
- if (errorCount < std::numeric_limits<uint8_t>::max())
- {
- errorCount++;
- }
-}
-
-double IpmbSensor::processReading(const std::vector<uint8_t>& data)
+bool IpmbSensor::processReading(const std::vector<uint8_t>& data, double& resp)
{
switch (readingFormat)
{
case (ReadingFormat::byte0):
- return data[0];
+ resp = data[0];
+ return true;
case (ReadingFormat::byte3):
if (data.size() < 4)
{
- std::cerr << "Invalid data length returned for " << name
- << "\n";
- return 0;
+ if (!errCount)
+ {
+ std::cerr << "Invalid data length returned for " << name
+ << "\n";
+ }
+ return false;
}
- return data[3];
+ resp = data[3];
+ return true;
case (ReadingFormat::elevenBit):
if (data.size() < 5)
{
- std::cerr << "Invalid data length returned for " << name
- << "\n";
- return 0;
+ if (!errCount)
+ {
+ std::cerr << "Invalid data length returned for " << name
+ << "\n";
+ }
+ return false;
}
- return ((data[4] << 8) | data[3]);
+ resp = ((data[4] << 8) | data[3]);
+ return true;
case (ReadingFormat::elevenBitShift):
if (data.size() < 5)
{
- std::cerr << "Invalid data length returned for " << name
- << "\n";
- return 0;
+ if (!errCount)
+ {
+ std::cerr << "Invalid data length returned for " << name
+ << "\n";
+ }
+ return false;
}
- return ((data[4] << 8) | data[3]) >> 3;
+ resp = ((data[4] << 8) | data[3]) >> 3;
+ return true;
default:
throw std::runtime_error("Invalid reading type");
}
@@ -305,7 +291,6 @@
}
if (!isPowerOn() && readState != PowerState::always)
{
- updateValue(0);
read();
return;
}
@@ -315,14 +300,7 @@
const int& status = std::get<0>(response);
if (ec || status)
{
- processError();
- updateValue(0);
- read();
- return;
- }
- if (!isPowerOn() && readState != PowerState::always)
- {
- updateValue(0);
+ incrementError();
read();
return;
}
@@ -338,17 +316,24 @@
}
if (data.empty())
{
- processError();
+ incrementError();
read();
return;
}
- double value = processReading(data);
+
+ double value = 0;
+
+ if (!processReading(data, value))
+ {
+ incrementError();
+ read();
+ return;
+ }
/* Adjust value as per scale and offset */
value = (value * scaleVal) + offsetVal;
updateValue(value);
read();
- errorCount = 0; // success
},
"xyz.openbmc_project.Ipmi.Channel.Ipmb",
"/xyz/openbmc_project/Ipmi/Channel/Ipmb", "org.openbmc.Ipmb",
diff --git a/src/MCUTempSensor.cpp b/src/MCUTempSensor.cpp
index a413214..69a3226 100644
--- a/src/MCUTempSensor.cpp
+++ b/src/MCUTempSensor.cpp
@@ -186,7 +186,7 @@
else
{
std::cerr << "Invalid read getMCURegsInfoWord\n";
- updateValue(-1);
+ incrementError();
}
read();
});
diff --git a/src/NVMeSensor.cpp b/src/NVMeSensor.cpp
index 8cdf744..d683174 100644
--- a/src/NVMeSensor.cpp
+++ b/src/NVMeSensor.cpp
@@ -229,23 +229,11 @@
constexpr const size_t errorThreshold = 5;
if (errorCode)
{
- sensor->errorCount = 0;
+ // timer cancelled successfully
return;
}
- if (!isPowerOn())
- {
- sensor->errorCount = 0;
- sensor->updateValue(std::numeric_limits<double>::quiet_NaN());
- }
- else if (sensor->errorCount < errorThreshold)
- {
- std::cerr << "MCTP timeout device " << sensor->name << "\n";
- sensor->errorCount++;
- }
- else
- {
- sensor->updateValue(0);
- }
+
+ sensor->incrementError();
// cycle it back
nvmeDevice->sensors.pop_front();
@@ -441,8 +429,9 @@
const int busNumber) :
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration,
- "xyz.openbmc_project.Configuration.NVMe", maxReading, minReading),
- objServer(objectServer), errorCount(0), bus(busNumber)
+ "xyz.openbmc_project.Configuration.NVMe", maxReading, minReading,
+ PowerState::on),
+ objServer(objectServer), bus(busNumber)
{
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/temperature/" + name,
@@ -465,8 +454,6 @@
association::interface);
setInitialProperties(conn);
- // setup match
- setupPowerMatch(conn);
}
NVMeSensor::~NVMeSensor()
@@ -480,9 +467,5 @@
void NVMeSensor::checkThresholds(void)
{
- if (!isPowerOn())
- {
- return;
- }
thresholds::checkThresholds(this);
}
diff --git a/src/PSUSensor.cpp b/src/PSUSensor.cpp
index 4d6bc1d..577f3e4 100644
--- a/src/PSUSensor.cpp
+++ b/src/PSUSensor.cpp
@@ -47,7 +47,7 @@
Sensor(boost::replace_all_copy(sensorName, " ", "_"),
std::move(_thresholds), sensorConfiguration, objectType, max, min),
std::enable_shared_from_this<PSUSensor>(), objServer(objectServer),
- inputDev(io), waitTimer(io), path(path), errCount(0), sensorFactor(factor)
+ inputDev(io), waitTimer(io), path(path), sensorFactor(factor)
{
if constexpr (DEBUG)
{
@@ -147,28 +147,17 @@
nvalue /= sensorFactor;
updateValue(nvalue);
- errCount = 0;
}
catch (const std::invalid_argument&)
{
std::cerr << "Could not parse " << response << "\n";
- errCount++;
+ incrementError();
}
}
else
{
std::cerr << "System error " << err << "\n";
- errCount++;
- }
-
- if (errCount >= warnAfterErrorCount)
- {
- if (errCount == warnAfterErrorCount)
- {
- std::cerr << "Failure to read sensor " << name << "\n";
- }
- updateValue(0);
- errCount++;
+ incrementError();
}
lseek(fd, 0, SEEK_SET);
diff --git a/src/TachSensor.cpp b/src/TachSensor.cpp
index c45fcb0..953f26c 100644
--- a/src/TachSensor.cpp
+++ b/src/TachSensor.cpp
@@ -51,11 +51,11 @@
const std::string& sensorConfiguration,
const std::pair<size_t, size_t>& limits) :
Sensor(boost::replace_all_copy(fanName, " ", "_"), std::move(_thresholds),
- sensorConfiguration, objectType, limits.second, limits.first),
+ sensorConfiguration, objectType, limits.second, limits.first,
+ PowerState::on),
objServer(objectServer), redundancy(redundancy),
presence(std::move(presenceSensor)),
- inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path),
- errCount(0)
+ inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path)
{
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/fan_tach/" + name,
@@ -96,7 +96,6 @@
itemAssoc->initialize();
}
setInitialProperties(conn);
- setupPowerMatch(conn);
setupRead();
}
@@ -133,7 +132,7 @@
{
if (!presence->getValue())
{
- updateValue(std::numeric_limits<double>::quiet_NaN());
+ markAvailable(false);
missing = true;
pollTime = sensorFailedPollTimeMs;
}
@@ -151,35 +150,17 @@
double nvalue = std::stod(response);
responseStream.clear();
updateValue(nvalue);
- errCount = 0;
}
catch (const std::invalid_argument&)
{
- errCount++;
+ incrementError();
+ pollTime = sensorFailedPollTimeMs;
}
}
else
{
- if (!isPowerOn())
- {
- errCount = 0;
- updateValue(std::numeric_limits<double>::quiet_NaN());
- }
- else
- {
- pollTime = sensorFailedPollTimeMs;
- errCount++;
- }
- }
- if (errCount >= warnAfterErrorCount)
- {
- // only print once
- if (errCount == warnAfterErrorCount)
- {
- std::cerr << "Failure to read sensor " << name << " at " << path
- << " ec:" << err << "\n";
- }
- updateValue(0);
+ incrementError();
+ pollTime = sensorFailedPollTimeMs;
}
}
responseStream.clear();
@@ -202,11 +183,6 @@
void TachSensor::checkThresholds(void)
{
- if (!isPowerOn())
- {
- return;
- }
-
bool status = thresholds::checkThresholds(this);
if (redundancy && *redundancy)