ExternalSensor: Further refinements
Further refinements, learned after more testing.
Moved the writeHook lambda out of the ExternalSensor constructor,
and into separate initWriteHook() function, to reduce the bloat of
an already extremely large argument list to the constructor,
and solve a nasty gotcha regarding enable_shared_from_this usage:
https://stackoverflow.com/a/63579750/3063879
Adding a few more useful debugging messages, to be printed when
debugging is enabled (set the "debug" constants to true).
Tested: Interrupted external source of data, values changed to NaN
as expected. Resumed external source, values changed from NaN to the
correctly updated values. Timer durations appear correct. Also sent
many configuration change messages, forcing sensor objects to be
reconstructed. No errors noted during processing, even when messages
sent quickly, and also with random delays, and left to run a while.
To check sensor data value:
busctl --no-pager introspect xyz.openbmc_project.ExternalSensor \
/xyz/openbmc_project/sensors/temperature/mysensor \
xyz.openbmc_project.Sensor.Value
To send a configuration change message:
dbus-send --system \
/xyz/openbmc_project/inventory/system/board/myboard/mysensor \
x.x.x.PropertiesChanged \
string:xyz.openbmc_project.Configuration.ExternalSensor
Change-Id: I7bd515fed8ddf391df3fabadab61321a446c1b9f
Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/include/ExternalSensor.hpp b/include/ExternalSensor.hpp
index 02110fe..a8b48e5 100644
--- a/include/ExternalSensor.hpp
+++ b/include/ExternalSensor.hpp
@@ -14,17 +14,21 @@
public std::enable_shared_from_this<ExternalSensor>
{
public:
- ExternalSensor(
- const std::string& objectType,
- sdbusplus::asio::object_server& objectServer,
- std::shared_ptr<sdbusplus::asio::connection>& conn,
- const std::string& sensorName, const std::string& sensorMeasure,
- std::vector<thresholds::Threshold>&& thresholdsIn,
- const std::string& sensorConfiguration, double maxReading,
- double minReading, double timeoutSecs, const PowerState& powerState,
+ ExternalSensor(const std::string& objectType,
+ sdbusplus::asio::object_server& objectServer,
+ std::shared_ptr<sdbusplus::asio::connection>& conn,
+ const std::string& sensorName,
+ const std::string& sensorMeasure,
+ std::vector<thresholds::Threshold>&& thresholdsIn,
+ const std::string& sensorConfiguration, double maxReading,
+ double minReading, double timeoutSecs,
+ const PowerState& powerState);
+ virtual ~ExternalSensor();
+
+ // Call this immediately after calling the constructor
+ void initWriteHook(
std::function<void(std::chrono::steady_clock::time_point now)>&&
writeHookIn);
- virtual ~ExternalSensor();
// Returns true if sensor has external Value that is subject to timeout
bool isAliveAndPerishable(void) const;
diff --git a/src/ExternalSensor.cpp b/src/ExternalSensor.cpp
index 13082e3..3aba626 100644
--- a/src/ExternalSensor.cpp
+++ b/src/ExternalSensor.cpp
@@ -26,9 +26,7 @@
const std::string& sensorName, const std::string& sensorUnits,
std::vector<thresholds::Threshold>&& thresholdsIn,
const std::string& sensorConfiguration, double maxReading,
- double minReading, double timeoutSecs, const PowerState& powerState,
- std::function<void(std::chrono::steady_clock::time_point now)>&&
- writeHookIn) :
+ double minReading, double timeoutSecs, const PowerState& powerState) :
// TODO(): When the Mutable feature is integrated,
// make sure all ExternalSensor instances are mutable,
// because that is the entire point of ExternalSensor,
@@ -41,8 +39,7 @@
writeTimeout(
std::chrono::duration_cast<std::chrono::steady_clock::duration>(
std::chrono::duration<double>(timeoutSecs))),
- writeAlive(false), writePerishable(timeoutSecs > 0.0),
- writeHook(std::move(writeHookIn))
+ writeAlive(false), writePerishable(timeoutSecs > 0.0)
{
// The caller must specify what physical characteristic
// an external sensor is expected to be measuring, such as temperature,
@@ -75,14 +72,6 @@
objectServer.add_interface(objectPath, association::interface);
setInitialProperties(conn);
- externalSetHook = [weakThis = weak_from_this()]() {
- auto lockThis = weakThis.lock();
- if (lockThis)
- {
- lockThis->externalSetTrigger();
- }
- };
-
if constexpr (debug)
{
std::cerr << "ExternalSensor " << name << " constructed: path "
@@ -95,6 +84,31 @@
}
}
+// Separate function from constructor, because of a gotcha: can't use the
+// enable_shared_from_this() API until after the constructor has completed.
+void ExternalSensor::initWriteHook(
+ std::function<void(std::chrono::steady_clock::time_point now)>&&
+ writeHookIn)
+{
+ // Connect ExternalSensorMain with ExternalSensor
+ writeHook = std::move(writeHookIn);
+
+ // Connect ExternalSensor with Sensor
+ auto weakThis = weak_from_this();
+ externalSetHook = std::move([weakThis]() {
+ auto lockThis = weakThis.lock();
+ if (lockThis)
+ {
+ lockThis->externalSetTrigger();
+ return;
+ }
+ if constexpr (debug)
+ {
+ std::cerr << "ExternalSensor receive ignored, sensor gone\n";
+ }
+ });
+}
+
ExternalSensor::~ExternalSensor()
{
// Make sure the write hook does not reference this object anymore
diff --git a/src/ExternalSensorMain.cpp b/src/ExternalSensorMain.cpp
index 0712b00..647eb5a 100644
--- a/src/ExternalSensorMain.cpp
+++ b/src/ExternalSensorMain.cpp
@@ -126,13 +126,14 @@
if (err != boost::system::errc::success)
{
// Cancellation is normal, as timer is dynamically rescheduled
- if (err != boost::system::errc::operation_canceled)
+ if (err != boost::asio::error::operation_aborted)
{
std::cerr << "ExternalSensor timer scheduling problem: "
<< err.message() << "\n";
}
return;
}
+
updateReaper(sensors, timer, std::chrono::steady_clock::now());
});
@@ -155,6 +156,11 @@
sensorsChanged,
boost::asio::steady_timer& reaperTimer)
{
+ if constexpr (debug)
+ {
+ std::cerr << "ExternalSensor considering creating sensors\n";
+ }
+
auto getter = std::make_shared<GetSensorConfiguration>(
dbusConnection,
[&io, &objectServer, &sensors, &dbusConnection, sensorsChanged,
@@ -284,6 +290,11 @@
sensorsChanged->erase(it);
findSensor->second = nullptr;
found = true;
+ if constexpr (debug)
+ {
+ std::cerr << "ExternalSensor " << sensorName
+ << " change found\n";
+ }
break;
}
}
@@ -315,7 +326,8 @@
sensorEntry = std::make_shared<ExternalSensor>(
sensorType, objectServer, dbusConnection, sensorName,
sensorUnits, std::move(sensorThresholds), interfacePath,
- maxValue, minValue, timeoutSecs, readState,
+ maxValue, minValue, timeoutSecs, readState);
+ sensorEntry->initWriteHook(
[&sensors, &reaperTimer](
const std::chrono::steady_clock::time_point& now) {
updateReaper(sensors, reaperTimer, now);
@@ -364,14 +376,22 @@
std::cerr << "callback method error\n";
return;
}
- sensorsChanged->insert(message.get_path());
+
+ auto messagePath = message.get_path();
+ sensorsChanged->insert(messagePath);
+ if constexpr (debug)
+ {
+ std::cerr << "ExternalSensor change event received: "
+ << messagePath << "\n";
+ }
+
// this implicitly cancels the timer
filterTimer.expires_from_now(boost::posix_time::seconds(1));
filterTimer.async_wait([&io, &objectServer, &sensors, &systemBus,
&sensorsChanged, &reaperTimer](
const boost::system::error_code& ec) {
- if (ec)
+ if (ec != boost::system::errc::success)
{
if (ec != boost::asio::error::operation_aborted)
{
@@ -379,6 +399,7 @@
}
return;
}
+
createSensors(io, objectServer, sensors, systemBus,
sensorsChanged, reaperTimer);
});