tachsensor: Similar fixes from HwmonTempSensor
Ported the substance of the changes from here to TachSensor:
https://gerrit.openbmc.org/c/openbmc/dbus-sensors/+/56019
The goal is to provide similar fixes to I/O and buffering, catching up
to similar changes made recently in HwmonTempSensor.
Tested: Fan RPM now shows up correctly on my machine again.
Signed-off-by: Josh Lehan <krellan@google.com>
Change-Id: I49c539e3713e9d02d4584f305e4ced6d4b2ba572
diff --git a/include/TachSensor.hpp b/include/TachSensor.hpp
index c2eaf36..a9da29f 100644
--- a/include/TachSensor.hpp
+++ b/include/TachSensor.hpp
@@ -1,6 +1,6 @@
#pragma once
#include <Thresholds.hpp>
-#include <boost/asio/streambuf.hpp>
+#include <boost/asio/random_access_file.hpp>
#include <boost/container/flat_map.hpp>
#include <boost/container/flat_set.hpp>
#include <gpiod.hpp>
@@ -58,7 +58,9 @@
boost::container::flat_map<std::string, bool> statuses;
};
-class TachSensor : public Sensor
+class TachSensor :
+ public Sensor,
+ public std::enable_shared_from_this<TachSensor>
{
public:
TachSensor(const std::string& path, const std::string& objectType,
@@ -73,21 +75,25 @@
const PowerState& powerState,
const std::optional<std::string>& led);
~TachSensor() override;
+ void setupRead();
private:
+ // Ordering is important here; readBuf is first so that it's not destroyed
+ // while async operations from other member fields might still be using it.
+ std::array<char, 128> readBuf{};
sdbusplus::asio::object_server& objServer;
std::optional<RedundancySensor>* redundancy;
std::unique_ptr<PresenceSensor> presence;
std::shared_ptr<sdbusplus::asio::dbus_interface> itemIface;
std::shared_ptr<sdbusplus::asio::dbus_interface> itemAssoc;
- boost::asio::streambuf readBuf;
- boost::asio::posix::stream_descriptor inputDev;
+ boost::asio::random_access_file inputDev;
boost::asio::deadline_timer waitTimer;
std::string path;
std::optional<std::string> led;
bool ledState = false;
- void setupRead(void);
- void handleResponse(const boost::system::error_code& err);
+
+ void handleResponse(const boost::system::error_code& err, size_t bytesRead);
+ void restartRead(size_t pollTime);
void checkThresholds(void) override;
};
diff --git a/src/FanMain.cpp b/src/FanMain.cpp
index 9367aac..13902c7 100644
--- a/src/FanMain.cpp
+++ b/src/FanMain.cpp
@@ -154,7 +154,7 @@
return true;
}
void createRedundancySensor(
- const boost::container::flat_map<std::string, std::unique_ptr<TachSensor>>&
+ const boost::container::flat_map<std::string, std::shared_ptr<TachSensor>>&
sensors,
const std::shared_ptr<sdbusplus::asio::connection>& conn,
sdbusplus::asio::object_server& objectServer)
@@ -206,7 +206,7 @@
void createSensors(
boost::asio::io_service& io, sdbusplus::asio::object_server& objectServer,
- boost::container::flat_map<std::string, std::unique_ptr<TachSensor>>&
+ boost::container::flat_map<std::string, std::shared_ptr<TachSensor>>&
tachSensors,
boost::container::flat_map<std::string, std::unique_ptr<PwmSensor>>&
pwmSensors,
@@ -489,11 +489,15 @@
}
findLimits(limits, baseConfiguration);
- tachSensors[sensorName] = std::make_unique<TachSensor>(
+
+ auto& tachSensor = tachSensors[sensorName];
+ tachSensor = nullptr;
+ tachSensor = std::make_shared<TachSensor>(
path.string(), baseType, objectServer, dbusConnection,
std::move(presenceSensor), redundancy, io, sensorName,
std::move(sensorThresholds), *interfacePath, limits, powerState,
led);
+ tachSensor->setupRead();
if (!pwmPath.empty() && fs::exists(pwmPath) &&
(pwmSensors.count(pwmPath) == 0U))
@@ -517,7 +521,7 @@
auto systemBus = std::make_shared<sdbusplus::asio::connection>(io);
systemBus->request_name("xyz.openbmc_project.FanSensor");
sdbusplus::asio::object_server objectServer(systemBus);
- boost::container::flat_map<std::string, std::unique_ptr<TachSensor>>
+ boost::container::flat_map<std::string, std::shared_ptr<TachSensor>>
tachSensors;
boost::container::flat_map<std::string, std::unique_ptr<PwmSensor>>
pwmSensors;
diff --git a/src/TachSensor.cpp b/src/TachSensor.cpp
index 9afe094..549838b 100644
--- a/src/TachSensor.cpp
+++ b/src/TachSensor.cpp
@@ -25,6 +25,7 @@
#include <sdbusplus/asio/connection.hpp>
#include <sdbusplus/asio/object_server.hpp>
+#include <charconv>
#include <fstream>
#include <iostream>
#include <istream>
@@ -53,17 +54,10 @@
objectType, false, false, limits.second, limits.first, conn,
powerState),
objServer(objectServer), redundancy(redundancy),
- presence(std::move(presenceSensor)), inputDev(io), waitTimer(io),
- path(path), led(ledIn)
+ presence(std::move(presenceSensor)),
+ inputDev(io, path, boost::asio::random_access_file::read_only),
+ waitTimer(io), path(path), led(ledIn)
{
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- int fd = open(path.c_str(), O_RDONLY);
- if (fd < 0)
- {
- std::cerr << "Tach sensor failed to open file\n";
- }
- inputDev.assign(fd);
-
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/fan_tach/" + name,
"xyz.openbmc_project.Sensor.Value");
@@ -98,7 +92,6 @@
itemAssoc->initialize();
}
setInitialProperties(sensor_paths::unitRPMs);
- setupRead();
}
TachSensor::~TachSensor()
@@ -116,19 +109,45 @@
objServer.remove_interface(itemAssoc);
}
-void TachSensor::setupRead(void)
+void TachSensor::setupRead()
{
- boost::asio::async_read_until(inputDev, readBuf, '\n',
- [&](const boost::system::error_code& ec,
- std::size_t /*bytes_transfered*/) {
- handleResponse(ec);
+ std::weak_ptr<TachSensor> weakRef = weak_from_this();
+ inputDev.async_read_some_at(
+ 0, boost::asio::buffer(readBuf),
+ [weakRef](const boost::system::error_code& ec, std::size_t bytesRead) {
+ std::shared_ptr<TachSensor> self = weakRef.lock();
+ if (self)
+ {
+ self->handleResponse(ec, bytesRead);
+ }
+ });
+}
+
+void TachSensor::restartRead(size_t pollTime)
+{
+ std::weak_ptr<TachSensor> weakRef = weak_from_this();
+ waitTimer.expires_from_now(boost::posix_time::milliseconds(pollTime));
+ waitTimer.async_wait([weakRef](const boost::system::error_code& ec) {
+ if (ec == boost::asio::error::operation_aborted)
+ {
+ return; // we're being canceled
+ }
+ std::shared_ptr<TachSensor> self = weakRef.lock();
+ if (!self)
+ {
+ return;
+ }
+ self->setupRead();
});
}
-void TachSensor::handleResponse(const boost::system::error_code& err)
+void TachSensor::handleResponse(const boost::system::error_code& err,
+ size_t bytesRead)
{
- if (err == boost::system::errc::bad_file_descriptor)
+ if ((err == boost::system::errc::bad_file_descriptor) ||
+ (err == boost::asio::error::misc_errors::not_found))
{
+ std::cerr << "TachSensor " << name << " removed " << path << "\n";
return; // we're being destroyed
}
bool missing = false;
@@ -143,24 +162,24 @@
}
itemIface->set_property("Present", !missing);
}
- std::istream responseStream(&readBuf);
+
if (!missing)
{
if (!err)
{
- std::string response;
- try
- {
- std::getline(responseStream, response);
- rawValue = std::stod(response);
- responseStream.clear();
- updateValue(rawValue);
- }
- catch (const std::invalid_argument&)
+ const char* bufEnd = readBuf.data() + bytesRead;
+ int nvalue = 0;
+ std::from_chars_result ret =
+ std::from_chars(readBuf.data(), bufEnd, nvalue);
+ if (ret.ec != std::errc())
{
incrementError();
pollTime = sensorFailedPollTimeMs;
}
+ else
+ {
+ updateValue(nvalue);
+ }
}
else
{
@@ -168,24 +187,8 @@
pollTime = sensorFailedPollTimeMs;
}
}
- responseStream.clear();
- inputDev.close();
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- int fd = open(path.c_str(), O_RDONLY);
- if (fd < 0)
- {
- return; // we're no longer valid
- }
- inputDev.assign(fd);
- waitTimer.expires_from_now(boost::posix_time::milliseconds(pollTime));
- waitTimer.async_wait([&](const boost::system::error_code& ec) {
- if (ec == boost::asio::error::operation_aborted)
- {
- return; // we're being canceled
- }
- setupRead();
- });
+ restartRead(pollTime);
}
void TachSensor::checkThresholds(void)