Krellan
commented
1 year ago Here's my thoughts:
1) It looks like a bug, if buildSensors() is in a tight restart loop. It should not be in a restart loop.
Expected behavior is that it should set all zones with missing sensors to failsafe mode, and continue loading. Zones without missing sensors should function normally. Zones with missing sensors should remain in failsafe mode.
If all of the missing sensors appear at a later time, and the zone is no longer missing any sensors, the zone should come out of failsafe mode, and begin normal operation. I'm hoping the program receives notifications when new sensors are added, so that it doesn't have to be manually restarted in order to search for sensors again. If not, that's a bug.
2) We should have a "missing is acceptable" flag, for each sensor. Default to false.
If we simply filter out all missing sensors, then we run the risk of overheating the system if an important sensor is missing. We should set the system to failsafe mode, instead. Failsafe mode is designed to be used if any sensors are missing, or not reporting valid data.
Some unimportant sensors may be designated as optional. Those, I'm OK with filtering them out in the updateSensors() loop, as you described. This designation would be a new feature, so that it would not impact existing sensors already in the field.
Alternatively, a compile-time constant could be used, to change all sensors from mandatory to optional, defaulting to false. This would most likely be much easier to implement. By defaulting to false, it would not affect existing systems in the field, so it would not compromise their ability to go into failsafe mode when sensors are missing.
harveyquta
zevweiss
huangalang
As we found that we have to define sensor reading path in config.json Then buildSensors() function will search all the sensor reading paths defined in config.json Once they are found , they are added to SensorManager (mgmr.addSensor()) (https://github.com/openbmc/phosphor-pid-control/blob/master/sensors/builder.cpp#L184)
Once pid service started , the added sensors reading are updated if the property changed =>void DbusPidZone::updateSensors(void)
However, buildSensors() process will fail when any of the defined sensor reading paths is not found. It will restart over and over
take following case as example : the sensors on a pluggable board The Sensors path need to be predefined in config.json But they wont exist , if the board is not plugged It will let buildSensors() process repeat over and over => (https://github.com/openbmc/phosphor-pid-control/blob/master/sensors/builder.cpp#L99)
We’d like propose a method called valid sensor detection The main idea is to create validSensors map recording if the sensor reading path is found within a certain time, Only the valid ones will be added to sensor manager (https://github.com/openbmc/phosphor-pid-control/blob/master/sensors/builder.cpp#L184)
And this validSensors map will be passed on to buildZones() process
Because only the valid sensors can be added to thermal_inputs of each zone (we use this map to identify the valid sensors) Through this way , only the valid sensors reading will be updated =>
=========================================
void DbusPidZone::updateSensors(void) { for (const auto& t : _thermalInputs) <== only the valid sensors in _thermalInputs { auto sensor = _mgr.getSensor(t); <=== only the valid sensors in mgr ReadReturn r = sensor->read(); }
}
please share your ideas, we are open to further discussion