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

[James Feist]

On 10/29/19 9:18 AM, James Feist wrote:
> 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.

Give this a shot: 
https://gerrit.openbmc-project.xyz/c/openbmc/intel-ipmi-oem/+/26664

It'll need to be ported to phosphor-sel-logger when review passes.

> 
> -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
>>>>