Showing signed sensor value when the command "ipmitool sel elist" is executed

[James Feist]

On 10/28/19 11:40 PM, Tony Lee (李文富) wrote:
>> On 10/27/19 11:41 PM, Tony Lee (李文富) wrote:
>>> Hi Jason,
>>>
>>> We know that when we execute the command "ipmitool sel elist", it will
>> return something like the following,
>>> "18 | 10/16/19 | 18:28:41 UTC | Temperature nvme0 | Upper Non-critical
>> going high | Asserted | Reading 72 > Threshold 70 degrees C".
>>>
>>> I met a problem that when the sensor value in the d-bus is negative, the
>> current reading in "ipmitool sel elist" will be 0.
>>> It seems that because the type of scaledValue is defined "uint32_t", there is
>> always a none negative value even current value is a negative value obtained
>> from the d-bus. In
>>>
>> https://github.com/openbmc/phosphor-sel-logger/blob/master/include/sens
>> orutils.hpp#L159
>>>
>>> Is this is an issue or I need to set it up somewhere?
>>
>> If min is < 0, then this should work:
>>
>> https://github.com/openbmc/phosphor-sel-logger/blob/6afe9560852c6431c4
>> 3c8e79a28e2b7cb498e355/include/sensorutils.hpp#L61
>>
>> https://github.com/openbmc/phosphor-dbus-interfaces/blob/12162be363f11
>> b9dafa92b5379db671712b3523c/xyz/openbmc_project/Sensor/Value.interfac
>> e.yaml#L28
>>
>> It uses MinValue < 0 to determine if the sensor is signed or not.
> 
> Hi James,
> 
> Yes, My MinValue and min are < 0.
> I understand that If min is < 0 then bSigned = true finally it will return static_cast<int8_t>(scaledValue)
> then this should work. But, after
> https://github.com/openbmc/phosphor-sel-logger/blob/6afe9560852c6431c43c8e79a28e2b7cb498e355/include/sensorutils.hpp#L159
> I got scaledValue = 0 and return 0 in the end.
> 
> There are another question that why is "scaledValue" defined as uint32_t?
> Shouldn't it return a byte(uint8_t) after calculation?

The return value is an uint8_t (about 5 lines below where you linked). 
The point of it being a uint32_t is so you can check for overflow. That 
being said looking at the unit tests I don't believe we check for 
negative, and that might need to be a int32_t.

The tests are here: 
https://github.com/openbmc/intel-ipmi-oem/blob/master/tests/test_sensorcommands.cpp

I'll try to take a look in the next day or two and write a new test to 
see if that fixes it, but feel free to add a test yourself if you beat 
me to it.

-James

> 
> Here is my settings and debug logs:
> max = 127.000000
> min = -128.000000
> value = -1.000000
> mValue = 1
> rExp = 0
> bValue = 0
> bExp = 0
> bSigned = true
> scaledValue = 0
> 
> Thanks,
> -Tony
> 
>> Thanks,
>>
>> -James
>>
>>>
>>> Thanks
>>> Best Regards,
>>> Tony
>>>