openbmc / phosphor-led-manager Goto Github PK

License: Apache License 2.0

Shell 1.01% C++ 94.12% Python 1.79% Meson 3.08%

phosphor-led-manager's Introduction

phosphor-led-manager

This project manages LED groups on dbus. Sometimes many LEDs must be driven together to indicate some system state.

For example, there can be multiple identify LEDs. When the user wants to identify the system, they should all light up together.

Configuration

The configuration can happen via json or yaml.

Configuration Example (JSON)

This is our configuration file. It describes 2 LEDs for the 'enclosure_identify' group, with their respective states and duty cycles.

$ cat example.json
{
  "leds": [
    {
      "group": "enclosure_identify",
      "members": [
        {
          "Name": "pca955x_front_sys_id0",
          "Action": "On",
          "DutyOn": 50,
          "Period": 0,
          "Priority": "Blink"
        },
        {
          "Name": "led_rear_enc_id0",
          "Action": "On",
          "DutyOn": 50,
          "Period": 0,
          "Priority": "Blink"
        }
      ]
    }
  ]
}

Then start the program with

$ ./phosphor-led-manager --config example.json

Dbus interface

When starting the program, our LED group shows up on dbus. Usually there will be many more groups.

$ busctl tree xyz.openbmc_project.LED.GroupManager
`- /xyz
  `- /xyz/openbmc_project
    `- /xyz/openbmc_project/led
      `- /xyz/openbmc_project/led/groups
        `- /xyz/openbmc_project/led/groups/enclosure_identify


$ busctl introspect xyz.openbmc_project.LED.GroupManager /xyz/openbmc_project/led/groups/enclosure_identify
NAME                                TYPE      SIGNATURE RESULT/VALUE FLAGS
...
xyz.openbmc_project.Led.Group       interface -         -            -
.Asserted                           property  b         false        emits-change writable

In the above output, the usual org.freedesktop.* interfaces have been removed to keep it readable.

We can now drive the entire group by setting it's 'Asserted' property on dbus.

$ busctl set-property \
xyz.openbmc_project.LED.GroupManager \
/xyz/openbmc_project/led/groups/enclosure_identify \
xyz.openbmc_project.Led.Group Asserted b true

The program can then use the xyz.openbmc_project.Led.Physical dbus interface exposed by phosphor-led-sysfs to set each LED state.

How to Build

meson setup build
cd build
ninja

phosphor-led-manager's People

Contributors

Stargazers

Watchers

Forkers

vsainath foxconn-bmc-ks lyuny usi-shrd vishwabmc steven-rd hclopenbmc lkammath giridharikrishna alpana07 hyt15-obmc nvidia

phosphor-led-manager's Issues

phosphor-led-manager doesn't override trigger type specified in kernel

Consider as a FR:
when I configured an LED as trigger type 'heartbeat' and expects phosphor-led-manager to override the trigger type in userspace, it doesn't work. Seems like phosphor-led-manager expects that the trigger type must be none or timer.

Performance improvements to Group Assertion Algorithm

Consider the following scenario :

1/. Fan_1 is Faulted and that results in Fan_1's LED to be in solid ON
2/. User requests a Identify operation on Fan_1 and that results in Fan_1's LED to be blinking
3/. User then requests that the Fan_1 identification be turned off.

The end result of this is that "Fan_1" must go back so solid ON as it was in step #1. https://gerrit.openbmc-project.xyz/#/c/1339/ is already doing it but the way it is doing it is ::

3.1) Turns off Fan_1 LED
3.2) Asserts Fan_1 as it was required by step # 1

However, a GroupManager is not required to know these. It should be the responsibility of whoever that is managing the actual LED to make the right configurations to go one from state to another state.

GroupManager must just say that "now go back and put it on solid on" and the job of powering off and then on is an abstraction that should be conceived by physical manager.

[BUG]: Missing ',' causes CI test to report an error

[problem 1]: In /test/config/led-group-config-malformed.json line 4 -> Missing a comma after "group": "bmc_booted" .
[problem 2]: After problem 1 is solved, another problem appears:

Setting an "Asserted" property on xyz.openbmc_project.LED.GroupManager takes a long time

This is to bring the attention to a problem that was discovered from phosphor-nvme openbmc/phosphor-nvme#4

I found that phosphor-nvme daemon was causing issues with the D-Bus, and realized that while the D-Bus is being stressed, it can take almost 133ms to call a setProperty to xyz.openbmc_project.LED.GroupManager and xyz.openbmc_project.Led.Group for "Asserted" property. This is in contrast to

Looking at the code for the setter, we can see that it should compare and return right away if it's the same

phosphor-led-manager/group.cpp

Lines 12 to 19 in 87fd11c

 bool Group::asserted(bool value) 

 { 

 // If the value is already what is before, return right away 

 if (value == 

 sdbusplus::xyz::openbmc_project::Led::server::Group::asserted()) 

 { 

 return value; 

 }

The mitigation for phosphor-nvme was to call the getter manually and not call the setter when the property did not have to change - this decreased the average time by 87% which was much more reasonable.

Why could this be the case? The "setter" in this case "should" just be doing what a "getter" should be doing if the property is the same, however we're seeing a huge performance drop for doing a set.

Workaround:

https://github.com/openbmc/phosphor-nvme/blob/fdffe5c37f0d1feaa90558433f688c3757d2e79a/nvme_manager.cpp#L116-L125

https://github.com/openbmc/phosphor-nvme/blob/fdffe5c37f0d1feaa90558433f688c3757d2e79a/nvme_manager.cpp#L155-L161

FR: support mutual exclusive groups

Currently I believe asserting a group doesn't actually do anything to other groups. If one has groups corresponding to various mutual exclusive states, e.g.

boot_start:
// Some LED actions
boot_complete:
// Some other LED actions

in code one has to explicitly set other groups deasserted first before asserting a group.

This can be simplified by supporting exclusivity in YAML, for example:

boot_start:
conflict: boot_complete
// Some LED actions
boot_complete:
conflict: boot_start
// Some other LED actions

Changes to python parser for led yaml

During review, Brad gave these comments :

1/. Use mako templates
2/. provide an option telling the location of input and generated files.

Changes to use .so's having generated bindings

Currently, we need to compile the generated sdbusplus binding code into the main application. idea is to link the main application against the .so that is obtained by compiling generated code.

When we build generating .so into the phosphor-dbus-interface repo, this issue can be addressed.

The `reply.read` method reports an error after adding clang-tidy

I tried to add clang-tidy locally to support static analysis code, but UT failed due to the following error (even if I added an empty .clang-tidy)

In file included from ../fault-monitor/fru-fault-monitor.cpp:1:
In file included from ../fault-monitor/fru-fault-monitor.hpp:5:
In file included from /usr/local/include/sdbusplus/bus.hpp:6:
In file included from /usr/local/include/sdbusplus/exception.hpp:5:
In file included from /usr/local/include/sdbusplus/sdbus.hpp:5:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/chrono:45:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/sstream:40:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/istream:40:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ios:44:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ios_base.h:41:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/locale_classes.h:40:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string:58:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/memory_resource.h:41:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/uses_allocator_args.h:38:
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:691:2: error: pack expansion contains parameter pack '_UTypes' that has a different length (1 vs. 3) from outer parameter packs
  691 |         using __convertible = __and_<is_convertible<_UTypes, _Types>...>;
      |         ^~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:853:27: note: in instantiation of template type alias '__convertible' requested here
  853 |           = _TCC<true>::template __convertible<_Args...>::value;
      |                                  ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:948:12: note: in instantiation of static data member 'std::tuple<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>> &>::__convertible<std::basic_string<char> &, std::basic_string<char> &, std::basic_string<char> &>' requested here
  948 |         explicit(!__convertible<_UElements&...>)
      |                   ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:2014:36: note: while substituting deduced template arguments into function template 'tuple' [with _UElements = <std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>]
 2014 |     { return tuple<_Elements&&...>(std::forward<_Elements>(__args)...); }
      |                                    ^
/usr/local/include/sdbusplus/message/read.hpp:549:30: note: in instantiation of function template specialization 'std::forward_as_tuple<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>> &>' requested here
  549 |     read_tuple(intf, m, std::forward_as_tuple(std::forward<Arg>(arg)));
      |                              ^
/usr/local/include/sdbusplus/message/read.hpp:585:14: note: in instantiation of function template specialization 'sdbusplus::message::details::read_grouping<std::tuple<>, std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>> &>' requested here
  585 |     details::read_grouping(intf, m, std::make_tuple(),
      |              ^
/usr/local/include/sdbusplus/message/read.hpp:235:33: note: (skipping 6 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
  235 |             sdbusplus::message::read(intf, m, s);
      |                                 ^
/usr/local/include/sdbusplus/message/read.hpp:478:34: note: in instantiation of function template specialization 'sdbusplus::message::details::read_single<std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>>>::op<std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>> &>' requested here
  478 |         read_single_t<itemType>::op(intf, m,
      |                                  ^
/usr/local/include/sdbusplus/message/read.hpp:549:5: note: in instantiation of function template specialization 'sdbusplus::message::details::read_tuple<std::tuple<std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>> &>>' requested here
  549 |     read_tuple(intf, m, std::forward_as_tuple(std::forward<Arg>(arg)));
      |     ^
/usr/local/include/sdbusplus/message/read.hpp:585:14: note: in instantiation of function template specialization 'sdbusplus::message::details::read_grouping<std::tuple<>, std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>> &>' requested here
  585 |     details::read_grouping(intf, m, std::make_tuple(),
      |              ^
/usr/local/include/sdbusplus/message.hpp:156:29: note: in instantiation of function template specialization 'sdbusplus::message::read<std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>> &>' requested here
  156 |         sdbusplus::message::read(_intf, _msg.get(),
      |                             ^
../fault-monitor/fru-fault-monitor.cpp:240:19: note: in instantiation of function template specialization 'sdbusplus::message::message::read<std::variant<std::vector<std::tuple<std::basic_string<char>, std::basic_string<char>, std::basic_string<char>>>> &>' requested here
  240 |             reply.read(assoc);
      |                   ^

After tracing the code, I found that an error was reported when calling msg.read in fault-monitor, because InterfaceMap contained AssociationList. I did a test and after deleting AssociationList, UT passed.

I'm not sure if it's a problem with sdbusplus, I hope experts can help check it out, thanks.

Get all the physical LED service names in one call

Today's code makes a call to mapper to get the service names of each and every physical LED. Since caching is not safe, this method has been chosen.

However, this can be changed to get ALL the Services that offer /xyz/openbmc_project/led/physical/ and thus can eliminate the need for every mapper call and instead use this once gotten tree.

phosphor-led-manager sometimes failed to reconfigure in userspace

Noticed it when implementing the BMC heartbeat LED.

When kernel has configured LED trigger as 'timer' and has set a blinking frequency, sometime phosphor-led-manager doesn't override the timer frequency when it starts.

Passing a custom led yaml path to meson

When phosphor-led-manager was transitioning from autotools to meson,
the ability to set your own yaml path through EXTRA_OECONF = "YAML_PATH=${STAGING_DATADIR_NATIVE}/${PN}"
was never ported.

Instead, it always uses meson.project_source_root().

There are several machines installing led.yaml to that yaml_path, and they seem to be broken now.

For example. the lastSuccessfulBuild of romulus from jenkins still has the wrong led groups:

root@romulus:~# busctl tree xyz.openbmc_project.LED.GroupManager
-/xyz
  -/xyz/openbmc_project
    -/xyz/openbmc_project/led
      -/xyz/openbmc_project/led/groups
        |-/xyz/openbmc_project/led/groups/bmc_booted
        |-/xyz/openbmc_project/led/groups/enclosure_fault
        |-/xyz/openbmc_project/led/groups/enclosure_identify
        |-/xyz/openbmc_project/led/groups/fan_fault
        |-/xyz/openbmc_project/led/groups/fan_identify
        -/xyz/openbmc_project/led/groups/power_on`

This is the led yaml, and it clearly has more than 6 groups.

Is removing led yaml path option intended and someone's going to update those metas?

Use variables from Physical LED than the ones in Group Manager

LED Group manager and Physical LED manager have Blink / On / Off in their own repositories and it kind of not maintainable so need to use the definitions that are in Physical LED inside the Group Manager.

Example code that is of concern :

// than giving one in ledlayout
if(action == Layout::Action::On) --> Group definition
{
return server::convertForMessage(server::Physical::Action::On); --> Physical definition
}

	bool Group::asserted(bool value)
	{
	// If the value is already what is before, return right away
	if (value ==
	sdbusplus::xyz::openbmc_project::Led::server::Group::asserted())
	{
	return value;
	}