Question of phosphor-sel-logger

[Bills, Jason M]

On 3/25/2021 7:25 AM, Matt Spinler wrote:
> 
> 
> On 3/25/2021 3:22 AM, Duke Du (杜祥嘉) wrote:
>>> -----Original Message-----
>>> From: Matt Spinler <mspinler at linux.ibm.com>
>>> Sent: Wednesday, March 24, 2021 9:20 PM
>>> To: Duke Du (杜祥嘉) <Duke.Du at quantatw.com>; openbmc at lists.ozlabs.org
>>> Cc: vernon.mauery at linux.intel.com; jason.m.bills at linux.intel.com; 
>>> Fran Hsu
>>> (徐誌謙) <Fran.Hsu at quantatw.com>; George Hung (洪忠敬)
>>> <George.Hung at quantatw.com>
>>> Subject: Re: Question of phosphor-sel-logger
>>>
>>>
>>>
>>> On 3/24/2021 6:28 AM, Duke Du (杜祥嘉) wrote:
>>>> Hi all,
>>>>
>>>>        I used package phosphor-hwmon and phospor-sel-logger to
>>> monitor sensor and create log when sensor reading
>>>>      cross the threshold. I found after the commit
>>> 25b26e162bd109b51aa09b16f26f9aa3d9d940fa of phosphor-sel-logger
>>>>      would catch the signal "ThresholdAsserted" to create sensor 
>>>> threhold
>>> log in the journal, but the phosphor-hwmon
>>>>      would not send the signal "ThresholdAsserted" when sensor reading
>>> is abnormal so that phosphor-sel-logger
>>>>      would not create the sensor threhold log, am I right ?
>>>>
>>>>      If I'm right, can you give me some suggestion to fix this side 
>>>> effect, or
>>> what setting I have lost in the
>>>>      phosphor-hwmon or phosphor-sel-logger ?
>>> Hi,
>>> That signal isn't defined in phosphor-dbus-interfaces, so phosphor-hwmon
>>> cannot use it.  When I tried to add it, it was rejected as-is with a
>>> recommendation to break it up into separate signals for each alarm 
>>> property
>>> on each interface.  At that point I gave up and had the code I was 
>>> working
>>> on at the time just look at propertiesChanged signals instead.
>>>
>>> If you would like to take that up it would entail:
>>> * Update
>>> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit. 
>>>
>>> openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%2B
>>> %2F39899&data=04%7C01%7CDuke.Du%40quantatw.com%7Cc5bf4d3d1
>>> 6f046cc6efa08d8eec78fd7%7C179b032707fc4973ac738de7313561b2%7C1%7
>>> C0%7C637521888783853893%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
>>> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp
>>> ;sdata=XoY4nKa3Go%2F9jt2coyzOcnXNrcMaw6XUtqnmK57k0ds%3D&res
>>> erved=0
>>> as requested
>>> * Update phosphor-hwmon to emit the new signals
>>> * Update phosphor-sel-logger to also listen for these new signals in 
>>> addition
>>> to the  current one, or change the dbus-sensors code to only emit the 
>>> new
>>> signals.
>>>
>>>
>> Hi Matt,
>>
>>     Thanks for your reply, I want to add a event monitor to listen 
>> "signal PropertyChanged" for
>>     "phosphor-phosphor-hwmon" only, like watchdog event monitor
>>     
>> (https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-sel-logger/+/37774), 
>>
>>     I think this is a simple way to fix this side effect, what do you 
>> think about my thought ?
>>     Please feel free to give me some suggestion, thanks very much !
> 
> I don't really know the best way to go here other than what I already 
> suggested.  sel-logger used to look at PropertiesChanged, but was 
> changed to use ThresholdAsserted I think so that it could capture the 
> sensor value within the signal.  But applications that need 
> phosphor-dbus-interfaces bindings for their operations, like 
> phosphor-hwmon, can't use ThresholdAsserted because it failed the PDI 
> review.
> 
> Jason or Vernon, what do you suggest?
With just PropertiesChanged we were getting events but when we read the 
current sensor value it had changed, so we were getting cases where an 
event showed a threshold was crossed but the value in that event was 
below the threshold.

As Matt noted, we attempted to fix this issue by creating the 
ThresholdAsserted signal so the value that triggered the threshold event 
could be sent with it and logged correctly.

I think the best options are
1. Find a new solution to the original problem where we can report 
accurate threshold crossed value with just PropertiesChanged.
2. Pursue Matt's proposed change to phosphor-dbus-interfaces to get the 
signal officially defined to include the sensor value in the signal.

Thanks,
-Jason

> 
> 
>>
>> Thanks
>> Duke
>>
>>>>      phosphor-sel-logger commit
>>> 25b26e162bd109b51aa09b16f26f9aa3d9d940fa link :
>>>> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
>>>>
>>> ub.com%2Fopenbmc%2Fphosphor-sel-logger%2Fcommit%2F25b26e162bd10
>>> 9b51aa0
>>> 9b16f26f9aa3d9d940fa&data=04%7C01%7CDuke.Du%40quantatw.com%
>>> 7Cc5bf4
>>> d3d16f046cc6efa08d8eec78fd7%7C179b032707fc4973ac738de7313561b2%7C
>>> 1%7C0
>>> %7C637521888783853893%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>>> wMDAiLCJQ
>>> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=rHxKqIk
>>> Vg7
>>>> yQXmqvjXal7I6eVBzw3ifl26gsZF8o4xo%3D&reserved=0
>>>>
>>>>      Thanks very much !
>>>>      Duke
>