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

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

[Tony Lee (李文富)]

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/sensorutils.hpp#L159

Is this is an issue or I need to set it up somewhere?

Thanks
Best Regards,
Tony

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

[James Feist]

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/sensorutils.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/6afe9560852c6431c43c8e79a28e2b7cb498e355/include/sensorutils.hpp#L61

https://github.com/openbmc/phosphor-dbus-interfaces/blob/12162be363f11b9dafa92b5379db671712b3523c/xyz/openbmc_project/Sensor/Value.interface.yaml#L28

It uses MinValue < 0 to determine if the sensor is signed or not.

Thanks,

-James

> 
> Thanks
> Best Regards,
> Tony
> 

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

[Tony Lee (李文富)]

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

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

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

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

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

[Tony Lee (李文富)]

> 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/s
> >>> ens
> >>> 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/6afe9560852c6431
> >>> c4
> >>> 3c8e79a28e2b7cb498e355/include/sensorutils.hpp#L61
> >>>
> >>>
> https://github.com/openbmc/phosphor-dbus-interfaces/blob/12162be363f
> >>> 11
> >>>
> b9dafa92b5379db671712b3523c/xyz/openbmc_project/Sensor/Value.interfa
> >>> c
> >>> 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/6afe9560852c6431c
> >> 43c8e79a28e2b7cb498e355/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.
OK, I get it.

> > The tests are here:
> > https://github.com/openbmc/intel-ipmi-oem/blob/master/tests/test_senso
> > rcommands.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
Looks good to me, thanks for the help.

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