[OpenBMC] [phosphor-sel-logger] Property type conversion and the value of getScaledIPMIValue
Emily Shaffer
emilyshaffer at google.com
Wed Mar 27 05:31:24 AEDT 2019
Please push your code to Gerrit. You don't need to ask permission and
we can perform the code review there. :)
On Mon, Mar 25, 2019 at 11:43 PM George Hung (洪忠敬)
<George.Hung at quantatw.com> wrote:
>
> Hi all,
>
> Sorry for the previous mail, I should not use the attached file for the mailing list. I re-describe what we would like to do:
>
> Currently phosphor-sel-logger support handles only double style sensors, and we would like to add both of double and in64 support for phosphor-sel-logger.
>
> But I'm not sure my change is OK and post my modification as follows first: (making sel-logger work with int64 and double.)
>
> diff --git a/include/threshold_event_monitor.hpp b/include/threshold_event_monitor.hpp
> index 9352805..00dba32 100644
> --- a/include/threshold_event_monitor.hpp
> +++ b/include/threshold_event_monitor.hpp
> @@ -115,7 +115,7 @@ inline static sdbusplus::bus::match::match startThresholdEventMonitor(
> "org.freedesktop.DBus.Properties", "GetAll");
> getSensorValue.append("xyz.openbmc_project.Sensor.Value");
> boost::container::flat_map<std::string,
> - sdbusplus::message::variant<double>>
> + sdbusplus::message::variant<double, int64_t>>
> sensorValue;
> try
> {
> @@ -135,6 +135,14 @@ inline static sdbusplus::bus::match::match startThresholdEventMonitor(
> ipmi::VariantToDoubleVisitor(), sensorValue["MinValue"]);
> double sensorVal = sdbusplus::message::variant_ns::visit(
> ipmi::VariantToDoubleVisitor(), sensorValue["Value"]);
> + double scale = 0;
> + if(sdbusplus::message::variant_ns::get_if<int64_t>(
> + &sensorValue["Value"]))
> + {
> + scale = sdbusplus::message::variant_ns::visit(
> + ipmi::VariantToDoubleVisitor(), sensorValue["Scale"]);
> + sensorVal *= std::pow(10, scale);
> + }
> try
> {
> eventData[1] = ipmi::getScaledIPMIValue(sensorVal, max, min);
> @@ -158,7 +166,7 @@ inline static sdbusplus::bus::match::match startThresholdEventMonitor(
> conn->new_method_call(msg.get_sender(), msg.get_path(),
> "org.freedesktop.DBus.Properties", "Get");
> getThreshold.append(thresholdInterface, event);
> - sdbusplus::message::variant<double> thresholdValue;
> + sdbusplus::message::variant<double, int64_t> thresholdValue;
> try
> {
> sdbusplus::message::message getThresholdResp =
> @@ -173,6 +181,10 @@ inline static sdbusplus::bus::match::match startThresholdEventMonitor(
> }
> double thresholdVal = sdbusplus::message::variant_ns::visit(
> ipmi::VariantToDoubleVisitor(), thresholdValue);
> + if(sdbusplus::message::variant_ns::get_if<int64_t>(&thresholdValue))
> + {
> + thresholdVal *= std::pow(10, scale);
> + }
> try
> {
> eventData[2] = ipmi::getScaledIPMIValue(thresholdVal, max, min);
> --
>
> If it's OK, I will push the patch to OpenBMC Gerrit.
>
> Thanks.
>
> Best Regard
> George Hung
> Research Division - Software Engineer
> Cloud Computing Business Unit
> Quanta Computer Inc.
> Ext: 16830
> E-Mail : George.Hung at QuantaTW.com
>
>
> -----Original Message-----
> From: George Hung (洪忠敬)
> Sent: Tuesday, March 26, 2019 2:26 PM
> To: 'Bills, Jason M'; OpenBMC Maillist; emilyshaffer at google.com; benjaminfair at google.com
> Cc: Richard Tung (董彥屏); Buddy Huang (黃天鴻); Will Liang (梁永鉉); Fran Hsu (徐誌謙)
> Subject: RE: [OpenBMC] [phosphor-sel-logger] Property type conversion and the value of getScaledIPMIValue
>
> Hi all,
>
> Considering there are two style of sensors (scaled int64 and double values) for sensor value DBus properties, we would like to add both of them support for phosphor-sel-logger.
>
> The attached file is the patch for making sel-logger work with int64 and double.
>
> If it's OK, I will push the patch to OpenBMC Gerrit.
>
> Hi Jason,
>
> Thanks for your kind advices.
>
> About the first question I mentioned before, "The sensor DBus properties , “/MaxValue/MinValue/Value/” are always 0", it's because type conversion cause always get 0, not their values are invalid. I'm sorry for my confused description.
>
>
> Thanks.
>
> Best Regard
> George Hung
> Research Division - Software Engineer
> Cloud Computing Business Unit
> Quanta Computer Inc.
> Ext: 16830
> E-Mail : George.Hung at QuantaTW.com
>
> -----Original Message-----
> From: Bills, Jason M [mailto:jason.m.bills at linux.intel.com]
> Sent: Saturday, March 23, 2019 6:50 AM
> To: George Hung (洪忠敬)
> Cc: Richard Tung (董彥屏); Buddy Huang (黃天鴻); Will Liang (梁永鉉); Fran Hsu (徐誌謙)
> Subject: Re: [OpenBMC] [phosphor-sel-logger] Property type conversion and the value of getScaledIPMIValue
>
> Hi George,
>
> It is better to post these types of questions to the full mailing list as I am not the right person to answer all of them.
>
> On 3/22/2019 12:20 AM, George Hung (洪忠敬) wrote:
> > Hi Jason,
> >
> > Since our OpenBMC project needs to support IPMI SEL, I try to use
> > phosphor-sel-logger to add new IPMI SEL to journal, but I found some
> > issues need to discuss:
> >
> > 1.The sensor DBus properties , “/MaxValue/MinValue/Value/” are always
> > 0
>
> This means that something is wrong in the sensor configuration. These values should all be valid.
> >
> > 2.The /value/ parameter of /getScaledIPMIValue/ function uses the
> > scaled value, not actual value,
>
> The threshold event monitor is currently designed to monitor only the double-style sensors that store the actual value.
> >
> > it may cause the result of /scaleIPMIValueFromDouble /function is
> > “value out of range”
> >
> > After tracing the code, we may find the root cause, but need your help
> > to check if it’s the correct fix. I listed as follows:
> >
> > 1.The original type declaration is
> > “sdbusplus::message::variant<double>”, but should be
> > “sdbusplus::message::variant<int64_t>”
>
> There are two styles of sensors, one style stores scaled int64 values and another style stores actual double values. The threshold event monitor currently handles only double style sensors.
>
> If you need to support both styles of sensors, you can look here for an example visitor that can handle both int64 and double:
> https://github.com/openbmc/phosphor-pid-control/commit/c065cf1b195f9ccd8fea882ce76416473d4fee7c.
> >
> > 2.Modify to use actual sensor value for “getScaledIPMIValue” function
> >
> > The attached file is the fix patch, it should be more clear.
> >
> > If it’s the correct fix, will you patch to OpenBMC Gerrit, or we do it
> > directly ?
>
> Please feel free to push your patch to OpenBMC Gerrit for review and discussion.
> >
> > BTW, the phosphor-sel-logger supports add IPMI SEL to journal, but
> > seems only threshold-based sensor event logs, no discrete sensors event logs.
>
> That is correct.
> >
> > Is there any plan to add discrete sensors event logs to
> > phosphor-sel-logger ?
>
> Intel plans to only support threshold sensor events in the IPMI SEL.
> The phosphor-sel-logger should be extensible to support additional types of events.
--
Emily Shaffer
More information about the openbmc
mailing list