pid/zone: Restore PWM when fans returned to auto
This makes use of the improved write() interface, to allow the
PID-loop-determined PWM to be restored, when the fan is returned to
automatic mode.
Without this fix, a fan set to manual mode, then manually set to a
different speed, would not properly return to the correct speed, when
transitioning back to automatic from manual.
This patch also adds a stub to allow the caller to learn the raw PWM
value written as output, another useful write() interface improvement.
Although not the topic of this change, it is included here, to avoid
later patch conflicts.
Tested: I can now correctly toggle between automatic, and manual, fan
control. Upon resuming automatic control, after a few seconds, the fan
PWM is now properly restored, to what the PID loop wanted it to be at.
Signed-off-by: Josh Lehan <krellan@google.com>
Signed-off-by: Jason Ling <jasonling@google.com>
Change-Id: I46fc65d6b931755d51093ea475c64cf5e3e6bacb
diff --git a/pid/fancontroller.cpp b/pid/fancontroller.cpp
index d3c58ca..d7ac0e6 100644
--- a/pid/fancontroller.cpp
+++ b/pid/fancontroller.cpp
@@ -145,7 +145,9 @@
for (const auto& it : _inputs)
{
auto sensor = _owner->getSensor(it);
- sensor->write(percent);
+ auto redundantWrite = _owner->getRedundantWrite();
+ int64_t rawWritten;
+ sensor->write(percent, redundantWrite, &rawWritten);
}
return;
diff --git a/pid/zone.cpp b/pid/zone.cpp
index 441031a..af40a2b 100644
--- a/pid/zone.cpp
+++ b/pid/zone.cpp
@@ -82,6 +82,12 @@
void DbusPidZone::setManualMode(bool mode)
{
_manualMode = mode;
+
+ // If returning to automatic mode, need to restore PWM from PID loop
+ if (!mode)
+ {
+ _redundantWrite = true;
+ }
}
bool DbusPidZone::getFailSafeMode(void) const
@@ -432,6 +438,12 @@
{
p->process();
}
+
+ if (_redundantWrite)
+ {
+ // This is only needed once
+ _redundantWrite = false;
+ }
}
void DbusPidZone::processThermals(void)
@@ -447,6 +459,11 @@
return _mgr.getSensor(name);
}
+bool DbusPidZone::getRedundantWrite(void) const
+{
+ return _redundantWrite;
+}
+
bool DbusPidZone::manual(bool value)
{
std::cerr << "manual: " << value << std::endl;
diff --git a/pid/zone.hpp b/pid/zone.hpp
index 3bea9c2..37b49ea 100644
--- a/pid/zone.hpp
+++ b/pid/zone.hpp
@@ -53,6 +53,8 @@
/* Could put lock around this since it's accessed from two threads, but
* only one reader/one writer.
*/
+
+ bool getRedundantWrite(void) const override;
void setManualMode(bool mode);
bool getFailSafeMode(void) const override;
@@ -71,6 +73,7 @@
void updateSensors(void) override;
void initializeCache(void) override;
void dumpCache(void);
+
void processFans(void) override;
void processThermals(void) override;
@@ -94,6 +97,7 @@
const int64_t _zoneId;
double _maximumSetPoint = 0;
bool _manualMode = false;
+ bool _redundantWrite = false;
const double _minThermalOutputSetPt;
const double _failSafePercent;
diff --git a/pid/zone_interface.hpp b/pid/zone_interface.hpp
index a024c0e..b8da048 100644
--- a/pid/zone_interface.hpp
+++ b/pid/zone_interface.hpp
@@ -41,6 +41,7 @@
* starts processing values to control fans.
*/
virtual void initializeCache(void) = 0;
+
/** Return cached value for sensor by name. */
virtual double getCachedValue(const std::string& name) = 0;
@@ -76,6 +77,11 @@
*/
virtual bool getManualMode(void) const = 0;
+ /** Returns true if a redundant fan PWM write is needed. Redundant write
+ * is used when returning the fan to automatic mode from manual mode.
+ */
+ virtual bool getRedundantWrite(void) const = 0;
+
/** For each fan pid, do processing. */
virtual void processFans(void) = 0;
/** For each thermal pid, do processing. */
diff --git a/test/pid_fancontroller_unittest.cpp b/test/pid_fancontroller_unittest.cpp
index 866b743..883ca84 100644
--- a/test/pid_fancontroller_unittest.cpp
+++ b/test/pid_fancontroller_unittest.cpp
@@ -171,10 +171,13 @@
SensorMock* sm1 = reinterpret_cast<SensorMock*>(s1.get());
SensorMock* sm2 = reinterpret_cast<SensorMock*>(s2.get());
+ EXPECT_CALL(z, getRedundantWrite())
+ .WillOnce(Return(false))
+ .WillOnce(Return(false));
EXPECT_CALL(z, getSensor(StrEq("fan0"))).WillOnce(Return(s1.get()));
- EXPECT_CALL(*sm1, write(0.75));
+ EXPECT_CALL(*sm1, write(0.75, false, _));
EXPECT_CALL(z, getSensor(StrEq("fan1"))).WillOnce(Return(s2.get()));
- EXPECT_CALL(*sm2, write(0.75));
+ EXPECT_CALL(*sm2, write(0.75, false, _));
// This is a fan PID, so calling outputProc will try to write this value
// to the sensors.
@@ -207,10 +210,13 @@
SensorMock* sm1 = reinterpret_cast<SensorMock*>(s1.get());
SensorMock* sm2 = reinterpret_cast<SensorMock*>(s2.get());
+ EXPECT_CALL(z, getRedundantWrite())
+ .WillOnce(Return(false))
+ .WillOnce(Return(false));
EXPECT_CALL(z, getSensor(StrEq("fan0"))).WillOnce(Return(s1.get()));
- EXPECT_CALL(*sm1, write(0.5));
+ EXPECT_CALL(*sm1, write(0.5, false, _));
EXPECT_CALL(z, getSensor(StrEq("fan1"))).WillOnce(Return(s2.get()));
- EXPECT_CALL(*sm2, write(0.5));
+ EXPECT_CALL(*sm2, write(0.5, false, _));
// This is a fan PID, so calling outputProc will try to write this value
// to the sensors.
@@ -243,13 +249,50 @@
double percent = 80;
double value = percent / 100;
+ EXPECT_CALL(z, getRedundantWrite()).WillOnce(Return(false));
EXPECT_CALL(z, getSensor(StrEq("fan0"))).WillOnce(Return(s1.get()));
- EXPECT_CALL(*sm1, write(value));
+ EXPECT_CALL(*sm1, write(value, false, _));
// This is a fan PID, so calling outputProc will try to write this value
// to the sensors.
p->outputProc(percent);
}
+TEST(FanControllerTest, OutputProc_VerifyRedundantWrites)
+{
+ /* when a zone indicates that redundant writes are enabled
+ * make sure the fan controller honors this by forcing a sensor write
+ */
+ ZoneMock z;
+
+ std::vector<std::string> inputs = {"fan0", "fan1"};
+ ec::pidinfo initial;
+
+ std::unique_ptr<PIDController> p =
+ FanController::createFanPid(&z, "fan1", inputs, initial);
+ EXPECT_FALSE(p == nullptr);
+
+ EXPECT_CALL(z, getFailSafeMode()).WillOnce(Return(false));
+
+ int64_t timeout = 0;
+ std::unique_ptr<Sensor> s1 = std::make_unique<SensorMock>("fan0", timeout);
+ std::unique_ptr<Sensor> s2 = std::make_unique<SensorMock>("fan1", timeout);
+ // Grab pointers for mocking.
+ SensorMock* sm1 = reinterpret_cast<SensorMock*>(s1.get());
+ SensorMock* sm2 = reinterpret_cast<SensorMock*>(s2.get());
+
+ EXPECT_CALL(z, getRedundantWrite())
+ .WillOnce(Return(true))
+ .WillOnce(Return(true));
+ EXPECT_CALL(z, getSensor(StrEq("fan0"))).WillOnce(Return(s1.get()));
+ EXPECT_CALL(*sm1, write(0.5, true, _));
+ EXPECT_CALL(z, getSensor(StrEq("fan1"))).WillOnce(Return(s2.get()));
+ EXPECT_CALL(*sm2, write(0.5, true, _));
+
+ // This is a fan PID, so calling outputProc will try to write this value
+ // to the sensors.
+ p->outputProc(50.0);
+}
+
} // namespace
} // namespace pid_control
diff --git a/test/pid_zone_unittest.cpp b/test/pid_zone_unittest.cpp
index 08f22d1..90d2a2f 100644
--- a/test/pid_zone_unittest.cpp
+++ b/test/pid_zone_unittest.cpp
@@ -125,6 +125,39 @@
EXPECT_TRUE(zone->getManualMode());
}
+TEST_F(PidZoneTest, SetManualMode_RedundantWritesEnabledOnceAfterManualMode)
+{
+ // Tests adding a fan PID controller to the zone, and verifies it's
+ // touched during processing.
+
+ std::unique_ptr<PIDController> tpid =
+ std::make_unique<ControllerMock>("fan1", zone.get());
+ ControllerMock* tmock = reinterpret_cast<ControllerMock*>(tpid.get());
+
+ // Access the internal pid configuration to clear it out (unrelated to the
+ // test).
+ ec::pid_info_t* info = tpid->getPIDInfo();
+ std::memset(info, 0x00, sizeof(ec::pid_info_t));
+
+ zone->addFanPID(std::move(tpid));
+
+ EXPECT_CALL(*tmock, setptProc()).WillOnce(Return(10.0));
+ EXPECT_CALL(*tmock, inputProc()).WillOnce(Return(11.0));
+ EXPECT_CALL(*tmock, outputProc(_));
+
+ // while zone is in auto mode redundant writes should be disabled
+ EXPECT_FALSE(zone->getRedundantWrite());
+
+ // but switching from manual to auto enables a single redundant write
+ zone->setManualMode(true);
+ zone->setManualMode(false);
+ EXPECT_TRUE(zone->getRedundantWrite());
+
+ // after one iteration of a pid loop redundant write should be cleared
+ zone->processFans();
+ EXPECT_FALSE(zone->getRedundantWrite());
+}
+
TEST_F(PidZoneTest, RpmSetPoints_AddMaxClear_BehaveAsExpected)
{
// Tests addSetPoint, clearSetPoints, determineMaxSetPointRequest
diff --git a/test/sensor_mock.hpp b/test/sensor_mock.hpp
index d67c457..1de68b6 100644
--- a/test/sensor_mock.hpp
+++ b/test/sensor_mock.hpp
@@ -18,6 +18,7 @@
MOCK_METHOD0(read, ReadReturn());
MOCK_METHOD1(write, void(double));
+ MOCK_METHOD3(write, void(double, bool, int64_t*));
};
} // namespace pid_control
diff --git a/test/zone_mock.hpp b/test/zone_mock.hpp
index 48b7e9a..bab77e3 100644
--- a/test/zone_mock.hpp
+++ b/test/zone_mock.hpp
@@ -18,7 +18,7 @@
MOCK_METHOD0(updateSensors, void());
MOCK_METHOD0(initializeCache, void());
MOCK_METHOD1(getCachedValue, double(const std::string&));
-
+ MOCK_CONST_METHOD0(getRedundantWrite, bool(void));
MOCK_METHOD1(addSetPoint, void(double));
MOCK_METHOD0(clearSetPoints, void());
MOCK_METHOD1(addRPMCeiling, void(double));