openbmc / phosphor-inventory-manager

Apache License 2.0
8 stars 9 forks source link

PIM responses failed when issuing Notify command. #8

Open petertom51 opened 1 year ago

petertom51 commented 1 year ago

Hi,

I'm new to openbmc. I'm trying to create a inventory by issuing Notify method and it works if I issue the command.

[20221213 updated] When I try on the latest code to add with uint16_t and uint64_t property, it failed. However, if I issue with uint32_t property, it works well. Is there anything I can do to solve the problem?

Cannot work with uint16_t

$ busctl call "xyz.openbmc_project.Inventory.Manager" \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/Cpu0" \
              1 "xyz.openbmc_project.Inventory.Item.Cpu" \
              1 "CoreCount" q 2

Call failed: Remote peer disconnected

work fine with uint32_t

$ busctl call "xyz.openbmc_project.Inventory.Manager" \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/Cpu0" \
              1 "xyz.openbmc_project.Inventory.Item.Cpu" \
              1 "MaxSpeedInMhz" u 4000

Fix below problem with https://gerrit.openbmc.org/c/openbmc/phosphor-inventory-manager/+/54444 mentioned in https://github.com/openbmc/phosphor-inventory-manager/issues/8#issuecomment-1339655508

$ busctl call "xyz.openbmc_project.Inventory.Manager"  \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/newInventory"  \
              1 "xyz.openbmc_project.Inventory.Item.PCIeDevice"  \
              1 "DeviceType" s "MultipleFunction"

However, if I issue it with GenerationSupported, it will failed.

$ busctl call "xyz.openbmc_project.Inventory.Manager"  \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/newInventory"  \
              1 "xyz.openbmc_project.Inventory.Item.PCIeDevice"  \
              1 "GenerationSupported" s "xyz.openbmc_project.Inventory.Item.PCIeSlot.Generations.Gen1"

Call failed: Remote peer disconnected

The same as GenerationInUse property. I try to see what's happened in phosphor-inventory. It said

 phosphor-inventory[1746]: terminate called after throwing an instance of 'std::bad_variant_access'
 phosphor-inventory[1746]:   what():  std::get: wrong index for variant

I introspect the success case and the GenerationInUse and GenerationSupported are strings.

Could you help to give me some advice about it that I could create it with GenerationSupported or GenerationInUse property? Thanks in advanced.

spinler commented 1 year ago

I think you're hitting the problem with using an enum that https://gerrit.openbmc.org/c/openbmc/phosphor-inventory-manager/+/54444 should fix.

petertom51 commented 1 year ago

Thanks @spinler! You save my day!

After applying this change and it works as expected. Thanks again.

petertom51 commented 1 year ago

Sorry, I need some more help here.

I continue to call Notify method to add some inventory by the latest code. But when I try to add with uint16_t and uint64_t property, it failed again. However, if I issue with uint32_t property, it works well.

Cannot work with uint16_t

$ busctl call "xyz.openbmc_project.Inventory.Manager" \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/Cpu0" \
              1 "xyz.openbmc_project.Inventory.Item.Cpu" \
              1 "CoreCount" q 2

Call failed: Remote peer disconnected

work fine with uint32_t

$ busctl call "xyz.openbmc_project.Inventory.Manager" \
              "/xyz/openbmc_project/inventory" \
              "xyz.openbmc_project.Inventory.Manager" Notify a{oa{sa{sv}}} \
              1 "/Cpu0" \
              1 "xyz.openbmc_project.Inventory.Item.Cpu" \
              1 "MaxSpeedInMhz" u 4000

The log in phosphor-inventory said it's a runtime error:

Dec 13 08:34:04 oe22-sled phosphor-inventory[3791]:   what():  Invalid conversion in MakeVariantVisitor::static auto phosphor::inventory::manager::MakeVariantVisitor<V>::Make<T, Arg, Enable>::make(Arg&&) [with T = std::variant<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<sdbusplus::xyz::openbmc_project::Inventory::Item::server::Cpu::Capability, std::allocator<sdbusplus::xyz::openbmc_project::Inventory::Item::server::Cpu::Capability> >, short unsigned int, unsigned int, long long unsigned int>; Arg = const bool&; Enable = void; V = std::variant<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<sdbusplus::xyz::openbmc_project::Inventory::Item::server::Cpu::Capability, std::allocator<sdbusplus::xyz::openbmc_project::Inventory::Item::server::Cpu::Capability> >, short unsigned int, unsigned int, long long unsigned int>]

Is there anything I can do for the problem? Thanks in advanced.

spinler commented 1 year ago

Is it failing in your code, or only with busctl?

petertom51 commented 1 year ago

It failed in my code in the first place so I tried it by the busctl command and found it failed as well. Does it work fine in your env?

spinler commented 1 year ago

@santoshpuranik Does this sound familiar?

santoshpuranik commented 1 year ago

@santoshpuranik Does this sound familiar?

I think adding the missing types to this variant : https://github.com/openbmc/phosphor-inventory-manager/blob/c919947156495db183ca1b758f481f52069f567a/types.hpp#L22 will fix it.

petertom51 commented 1 year ago

@santoshpuranik, thanks for helping.

Unfortunately, it seems that the build will fail if I add uint16_t to the list.

  /** @brief Inventory manager supported property types. */
  using InterfaceVariantType =
     std::variant<bool, size_t, uint16_t, int64_t, std::string, std::vector<uint8_t>>;

The log is here: log.do_compile.log. Sorry, I am trying to catch up on C++ syntax but still working on it, so I just uploaded the log file here.

I also tried to add the Association interface since the pcie_slot.hpp in bmcweb will try to get its association for verification. However, the signature of the xyz.openbmc_project.Association.Definitions.Associations is a(sss). If it goes this way in the end, do we need to add a(sss) into the list of InterfaceVariantType?

santoshpuranik commented 1 year ago

@santoshpuranik, thanks for helping.

Unfortunately, it seems that the build will fail if I add uint16_t to the list.

  /** @brief Inventory manager supported property types. */
  using InterfaceVariantType =
     std::variant<bool, size_t, uint16_t, int64_t, std::string, std::vector<uint8_t>>;

The log is here: log.do_compile.log. Sorry, I am trying to catch up on C++ syntax but still working on it, so I just uploaded the log file here.

I also tried to add the Association interface since the pcie_slot.hpp in bmcweb will try to get its association for verification. However, the signature of the xyz.openbmc_project.Association.Definitions.Associations is a(sss). If it goes this way in the end, do we need to add a(sss) into the list of InterfaceVariantType?

You will also need to update the D-Bus interface for notify: https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/yaml/xyz/openbmc_project/Inventory/Manager.interface.yaml and add the required types to the variant there.

As for creating associations, did you read the section here: https://github.com/openbmc/phosphor-inventory-manager#creating-associations?

petertom51 commented 1 year ago

@santoshpuranik I appreciated it! Yes, I can issue the notify method with different types by adding them to both phosphor-inventory-manager and phosphor-dbus-interfaces.

Regarding the associations part, your're right and I missed that part. It appears that I need to add a JSON data to allow PIM to understands how to create the association if the inventory is created. I will give it a try.

Thanks again.

williamspatrick commented 1 year ago

Unfortunately, it seems that the build will fail if I add uint16_t to the list.

Generally we don’t specify that closely the int size in dbus as if it were a hardware field. I’m not likely to accept a dbus-interface proposal that adds uint16 to an inventory item. This is over optimization and probably tight coupling to a particular hardware definition which isn’t future proof.