Add temp-to-margin conversion in margin PID loops

[Krellan]

This is not an issue, but rather, it's a proposal for enhancement. It takes more than a few sentences to describe, so I'm writing it up here, instead of just mentioning it on the OpenBMC Discord.

The read-margin-temp program is very useful, as a preprocessor for phosphor-pid-control, however, it's part of Quanta's BMC fork. It's not part of OpenBMC.

In the list of input sensors, for a margin PID loop, there are 2 key features that using read-margin-temp provides, that aren't currently matched by phosphor-pid-control alone:

• Ability to freely mix and match temperature sensors with margin sensors. Essentially, all temperature sensors are converted to margin sensors, as a preprocessing step, so that all the temperature sensors become margin sensors. Then, the PID loop can be treated as a margin PID loop.

• Ability to freely mix and match margin sensors with differing margin-zero point (this temperature point is also called Tjmax, also called setpoint). All margin sensors are compared individually, and the combined worst margin is determined. Essentially, this lets you set the setpoint on a per-sensor basis, without having to use the same setpoint for the entire PID loop.

I propose adding a new field to the PID loop configuration for PID loops of type margin.

The name of the field will be TempToMargin (unless I can think of a better name). The content of the field will be a JSON dictionary.

• The name field of this dictionary will be a string, which must correspond to one of the input sensors already named in the Inputs field of this PID loop, otherwise this dictionary entry will be ignored.

• The value field of this dictionary will be a number. This will be the margin-zero point, Tjmax, for this sensor.

The phosphor-pid-control program will then preprocess the sensors accordingly, converting the named temperature sensors into margin sensors, in the usual way (Margin = Tjmax - Temperature). The rest of the PID loop processing will continue normally.

Example:

{
  ...
  "Type": "margin",
  "Inputs": ["temp1", "temp2", "margin1", "margin2"],
  "TempToMargin": {
    "temp1": 85,
    "temp2": 95
  },
  ...
}

How's this sound?

[drakedog2008]

How could it deal with the case that the "temp1" is dynamically populated by EntityManager?

[harveyquta]

According to your example and my understanding, this is only for pid-control static json(config.json), not for entity-manager json setting, right?

[Krellan]

The TempToMargin field will be optional: if not present, it will be treated as if it were an empty JSON dictionary. In other words, it will have no effect.

It should work with entity-manager JSON and static JSON. They both boil down to populating the same data structure internally. I see no reason for a difference in behavior.

Ugh, I got the syntax wrong. Now I see what you mean. Here's a revised example:

{
  ...
  "Type": "Pid",
  "Class": "margin",
  "Inputs": [
    "temp1",
    "temp2",
    "margin1",
    "margin2"
  ],
  "TempToMargin": {
    "temp1": 85,
    "temp2": 95
  },
  ...
}

If it's to be dynamically populated by EntityManager, then this JSON stanza should be placed within the Exposes array of your EntityManager JSON files. This is only the addition of a new optional field TempToMargin to the existing JSON dictionary that holds the PID loop configuration (all the coefficients and so forth). It doesn't change or affect any other syntax.

[harveyquta]

So...it means that pid-control will preprocess to calculate the margin value of temp1(become 85 - temp1) and temp2(become 95 - temp2) and use the same setpoint as margin1 and margin2, for example, 10, to calculate pid?

Another question, does "TempToMargin" value need to support string value? Like "SetPointOffset" in pid-control.

[Krellan]

Yes, you got it. The intent of the example above: temp1 and temp2 are in units of raw temperature, and will be converted to margin, as you described. As margin1 and margin2 are already natively in units of margin, all four inputs will now be in units of margin. The PID can then take the input with the worst margin (the leader), using this margin value as the PID input, in the usual way.

As for string values, I haven't used this usage before. Looking through the code, I see it's a way to dynamically take a specified threshold field, by name, for the sensor, and get the necessary value from the threshold, instead of having to specify the value directly in the field. Interesting, it could be useful for some users. I don't really use thresholds, so haven't had the need for this. It could be added, as an optional syntax extension, if desired. Give the name of a particular threshold, such as CriticalHigh, and that value will be looked up and used as the margin-zero temperature point.

[Krellan]

Slight change of format needed.

I'm working on an initial implementation of this. Unfortunately, entity-manager has some limitations:

• No sub-objects allowed. So, the TempToMargin field can not itself be an object, although this is perfectly valid JSON.

• No arrays of integers allowed. Only arrays of double (floating-point numbers). Using only integers will result in an empty array, due to a bug in entity-manager. Mixing integers and floating-point numbers in the same array will result in it failing to parse.

These are bugs in entity-manager that are beyond the scope of this feature request to fix. So, working around them seems to be the best approach for now.

I define a new format:

"TempToMargin": [
  85.0,
  95.0
]

These are the changes:

• It is now an array of floating-point numbers. Matching them up with the correct sensor names, from the Inputs field (an array of strings), will be done by matching array indexes, not by matching names. So, the ordering now becomes important.

• If you have sensors that are already in margin format, move them to the end of your Inputs array, and do not add any corresponding elements for them in the TempToMargin array. That way, there will be no corresponding index. This is OK, and it simply means that not all of the input sensors in this PID loop are to be converted from degrees of temperature to degrees of safety margin.

• For all numbers in the TempToMargin array, append .0 so that they are parsed as floating-point numbers, even if they would otherwise be just fine as integers.

Hope this is all right with everyone.

[Krellan]

Oh, two more things this brings up:

• In the old style JSON parser, this field will be named tempToMargin, to be consistent with their lowerSnakeCase capitalization they have standardized upon.

• Because of the requirement to be only floating-point numbers, this prevents strings from being used as values, so this rules out being able to dynamically substitute in an already-configured threshold value for that sensor, as SetPointOffset would let you do. Maybe for a future revision to this feature (perhaps parsing it as an array of strings would also be acceptable).

[Krellan]

OK, the implementation is now up for review: https://gerrit.openbmc.org/c/openbmc/phosphor-pid-control/+/60303

[Krellan]

This feature has been merged! This FR can now be closed.