<div dir="ltr"><div>Sorry about the regression issue, Hank.</div>IIRC I proposed to have separate groups of min/max values in D-Bus and json files since they convey different semantics.<div>At the end the min/max value <font color="#000000"><span style="">was merged in the current way,</span></font> because the assumption that min/max values in json is only used for fan pwm and nothing else -- which obviously is not true now.<br>Should we revisit the approach? Having something like 'fanPwmScale' as a separate config param is a cleaner approach in my opinion.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 9, 2019 at 9:59 AM Patrick Venture <<a href="mailto:venture@google.com">venture@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <<a href="mailto:Hank.Liou@quantatw.com" target="_blank">Hank.Liou@quantatw.com</a>> wrote:<br>
><br>
> Hi Patrick,<br>
><br>
><br>
> Sorry for not clearly stating our problem. We have the following issue:<br>
><br>
><br>
> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as input for temp sensor -><br>
><br>
> stepwise fan table output 100 duty -> full speed fan<br>
<br>
Ok, so you're getting a dbus-based min/max value and you want to<br>
ignore it? In the json configuration, if you set the values to 0,<br>
they are set to the default (0), so there'd be no clean way to know to<br>
ignore dbus in this case, without adding a small check to only<br>
consider min/max from dbus when sensor is not temp/margin. Basically,<br>
only care about min/max on dbus if type is "fan"<br>
<br>
If that's right, James do you have a cycle to look at this one-liner?<br>
<br>
><br>
><br>
> As a result, fan will be at full speed while temp is low. Before the commit fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will use config min/max, which is 0 in our case. This would not trigger scaling function, namely scaleSensorReading, in util.cpp. However, after such commit, min/max would be non-zero (-128/127 from DBus). This will trigger scaling function.<br>
><br>
><br>
> [1] <a href="https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c</a><br>
><br>
><br>
> Hank<br>
><br>
><br>
> ________________________________<br>
> From: Hank Liou (劉晉翰)<br>
> Sent: Monday, September 2, 2019 4:52 PM<br>
> To: Patrick Venture<br>
> Cc: James Feist; <a href="mailto:openbmc@lists.ozlabs.org" target="_blank">openbmc@lists.ozlabs.org</a><br>
> Subject: Re: [phosphor-pid-control] scaling issue<br>
><br>
><br>
> Hi Patrick,<br>
><br>
><br>
> Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.<br>
><br>
><br>
> [1] <a href="https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59</a><br>
><br>
><br>
> Hank<br>
><br>
><br>
> ________________________________<br>
> From: Patrick Venture <<a href="mailto:venture@google.com" target="_blank">venture@google.com</a>><br>
> Sent: Friday, August 30, 2019 1:47 AM<br>
> To: Hank Liou (劉晉翰)<br>
> Cc: James Feist; <a href="mailto:openbmc@lists.ozlabs.org" target="_blank">openbmc@lists.ozlabs.org</a><br>
> Subject: Re: [phosphor-pid-control] scaling issue<br>
><br>
> PTAL - <a href="https://gerrit.openbmc-project.xyz/24827" rel="noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/24827</a> - this is merged, and<br>
> the srcrev bump should propagate into openbmc/openbmc in a day or two.<br>
><br>
> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <<a href="mailto:Hank.Liou@quantatw.com" target="_blank">Hank.Liou@quantatw.com</a>> wrote:<br>
> ><br>
> > Hi Patrick,<br>
> ><br>
> > I think it's OK to parse the config min&max for temp sensors.<br>
> ><br>
> > Any suggestion?<br>
> ><br>
> > Thanks,<br>
> > Hank<br>
> ><br>
> > >-----Original Message-----<br>
> > >From: openbmc [mailto:<a href="mailto:openbmc-" target="_blank">openbmc-</a><br>
> > >bounces+hank.liou=<a href="mailto:quantatw.com@lists.ozlabs.org" target="_blank">quantatw.com@lists.ozlabs.org</a>] On Behalf Of Hank Liou<br>
> > >(劉晉翰)<br>
> > >Sent: Friday, August 23, 2019 4:31 PM<br>
> > >To: Patrick Venture <<a href="mailto:venture@google.com" target="_blank">venture@google.com</a>>; James Feist<br>
> > ><<a href="mailto:james.feist@linux.intel.com" target="_blank">james.feist@linux.intel.com</a>><br>
> > >Cc: <a href="mailto:openbmc@lists.ozlabs.org" target="_blank">openbmc@lists.ozlabs.org</a><br>
> > >Subject: RE: [phosphor-pid-control] scaling issue<br>
> > ><br>
> > >Hi Patrick,<br>
> > ><br>
> > >>-----Original Message-----<br>
> > >>From: Patrick Venture [mailto:<a href="mailto:venture@google.com" target="_blank">venture@google.com</a>]<br>
> > >>Sent: Wednesday, August 21, 2019 10:32 PM<br>
> > >>To: Hank Liou (劉晉翰) <<a href="mailto:Hank.Liou@quantatw.com" target="_blank">Hank.Liou@quantatw.com</a>>; James Feist<br>
> > >><<a href="mailto:james.feist@linux.intel.com" target="_blank">james.feist@linux.intel.com</a>><br>
> > >>Cc: <a href="mailto:openbmc@lists.ozlabs.org" target="_blank">openbmc@lists.ozlabs.org</a><br>
> > >>Subject: Re: [phosphor-pid-control] scaling issue<br>
> > >><br>
> > >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)<br>
> > >><<a href="mailto:Hank.Liou@quantatw.com" target="_blank">Hank.Liou@quantatw.com</a>> wrote:<br>
> > >>><br>
> > >>> Hi All,<br>
> > >>><br>
> > >>><br>
> > >>> After commit [1], I found my temp sensor reading would be re-scaled<br>
> > >>> by<br>
> > >>multiplying 1 over 255, making temperature into unfamiliar unit. Also<br>
> > >>the fan rpm reading would lie in [0,1] interval, letting the fan input<br>
> > >>to be 0 (since the input value of fan is from an integer array [2]). Are these<br>
> > >normal behaviors?<br>
> > >>Or do I miss something?<br>
> > >><br>
> > >>Are you using dbus configuration or json? If json, can you attach your config.<br>
> > >>Since you're saying it was working and now isn't, I'm assuming there's<br>
> > >>something about the config being treated differently with the code<br>
> > >>changes in an unexpected way.<br>
> > ><br>
> > >I found pid control will first read min and max from dbus and then (before<br>
> > >commit [1]) revise them if<br>
> > ><br>
> > >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)<br>
> > ><br>
> > >After value initialization, the min and max would be the ones in json file (Json<br>
> > >file [3]). However, after commit [1] the min and max values of config would<br>
> > >not be fed into the fan control process. The scaling issue is originated from<br>
> > >commit [4] with the aim to treat fan rpm as percentage. It seems that commit<br>
> > >[1] unexpectedly affects temp sensors in the sense that the temp is the<br>
> > >integer reading not percentage. Before commit [1], it would not re-scale the<br>
> > >temp reading since my min and max are 0 [6].<br>
> > ><br>
> > ><br>
> > ><br>
> > >There is another issue with commit [1]. Now the fan rpm would be something<br>
> > >like<br>
> > ><br>
> > >1500 / 20000 = 0.075<br>
> > ><br>
> > >where rpm max 20000 is from dbus. However the fan input function would<br>
> > >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0<br>
> > >not percentage. Is there something wrong?<br>
> > ><br>
> > >[1] <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c<br>
> > >[2] <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle<br>
> > >r.cpp#L86<br>
> > >[3]<br>
> > > {<br>
> > > "name": "fan1",<br>
> > > "type": "fan",<br>
> > > "readPath": "/sys/class/hwmon/hwmon1/fan1_input",<br>
> > > "writePath": "/sys/class/hwmon/hwmon1/pwm1",<br>
> > > "min": 0,<br>
> > > "max": 255<br>
> > > },<br>
> > > {<br>
> > > "name": "temp1",<br>
> > > "type": "temp",<br>
> > > "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",<br>
> > > "writePath": "",<br>
> > > "min": 0,<br>
> > > "max": 0<br>
> > > }<br>
> > >[4] <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >control/commit/75eb769d351434547899186f73ff70ae00d7934a<br>
> > >[5] <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle<br>
> > >r.cpp#L64<br>
> > >[6] <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi<br>
> > >ve.cpp#L158<br>
> > ><br>
> > >><br>
> > >>><br>
> > >>><br>
> > >>> [1]<br>
> > >>> <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >>control/commit/fc2e803f5d92569<br>
> > >>> 44e18c7c878a441606b1f121c<br>
> > >>><br>
> > >>> [2]<br>
> > >>> <a href="https://github.com/openbmc/phosphor-pid-" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-pid-</a><br>
> > >>control/blob/a7ec8350d17b70153<br>
> > >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86<br>
> > >>><br>
> > >>><br>
> > >>> Thanks,<br>
> > >>><br>
> > >>><br>
> > >>> Hank Liou<br>
> > >>><br>
> > >>> Quanta Computer Inc.<br>
> > >>><br>
> > >>><br>
> > ><br>
> > >Sincerely,<br>
> > >Hank<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr">Regards,<div>Kun</div></div></div>