Entity-Manager's Configuration Generation leaves address as string

[Patrick Venture]

On Wed, Aug 28, 2019 at 2:38 PM James Feist <james.feist at linux.intel.com> wrote:
>
> On 8/28/19 2:36 PM, Patrick Venture wrote:
> > On Wed, Aug 28, 2019 at 2:29 PM James Feist <james.feist at linux.intel.com> wrote:
> >>
> >> On 8/28/19 2:16 PM, Patrick Venture wrote:
> >>> On Wed, Aug 28, 2019 at 9:29 AM James Feist <james.feist at linux.intel.com> wrote:
> >>>>
> >>>> On 8/28/19 7:27 AM, Patrick Venture wrote:
> >>>>> I think I've figured out what's happening.
> >>>>>
> >>>>> If a configuration has no fields that are changed by the template code
> >>>>> (or possibly even in that case), nothing happens to the values.  So,
> >>>>> the property Address is left "0x54" if that's what it is.  And the
> >>>>> code is templated, so it just adds that property of type string to the
> >>>>> dbus sensor configuration.  As this is definitely what I'm seeing.
> >>>>> Json doesn't support ints that are written raw as hex, so wrapping
> >>>>> them as strings is what's required to make the json parse.  I've
> >>>>> worked around this problem by just using decimal everywhere, but
> >>>>> that's harder to read when comparing to schematics.
> >>>>
> >>>> Based on this, I think this line might be your issue:
> >>>>
> >>>> https://github.com/openbmc/entity-manager/blob/3b80d7c51ff5d5859c0ca1f2b517c18f4766a1a6/src/EntityManager.cpp#L1336
> >>>>
> >>>>
> >>>> If found device is nullopt, you still want to call this line, but you
> >>>> want to call it with an empty flat_map.
> >>>>
> >>>> I verified if this happens it should work here:
> >>>>
> >>>> https://gerrit.openbmc-project.xyz/c/openbmc/entity-manager/+/24787
> >>>
> >>> So, we're still hitting the error:
> >>>
> >>> ...
> >>>       "Exposes": [
> >>>           {
> >>>              "Address": "0x54",
> >>>              "Bus": 31,
> >>>              "Name": "i2cool 0",
> >>>              "Type": "MAX31725"
> >>>           },
> >>> ....
> >>
> >> Did you change the above line? What have you tried? Guessing it has to
> >> do with templateCharReplace not getting hit, if it gets hit it should be
> >> replacing it.
> >
> > Yeah, I see your new unit-test validates the replacement behavior.  So
> > it must be that it's not hitting that line you pointed to.  I'll see
> > if I can grab a test platform tomorrow and see if I can replicate it
> > (we have a config that consistently fails).  And see if I can figure
> > out why the device isn't being "found."
>
> This would make sense if you had a probe that is set to "TRUE". We
> normally probe for everything, so this would make sense why we haven't
> seen it.

}
...
   "Name": "...",
    "Probe": "TRUE",
    "Type": "Board"
}

Yeah.  This is in our baseboard one that is always loaded.  So, an
interesting bug case to hit :D  If we had used the fru data for this
probe it would have hidden the issue.  Neat.

Will validate your patch hopefully shortly.

>
> >
> >>
> >>>
> >>> Aug 15 22:38:58 MACHINE hwmontempsensor[2697]: terminate called after
> >>> throwing an instance of 'std::bad_variant_access'
> >>>
> >>> It's failing because the configuration is a string "0x54" in dbus.
> >>>
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>> Would it make sense to make the add property code less field agnostic
> >>>>> so that if the field is Address and the Interface for
> >>>>> configuration.XXX that it checks to see if it's a hex string?  Or,
> >>>>> maybe the templateChar replace -- if that supports converting the hex
> >>>>> string to a raw integer value type should always get hit?
> >
> >
> >
> >>>>>
> >>>>> Thanks,
> >>>>> Patrick
> >>>>>