sysfs: Refactor LED property parsing
Added new test for parsing led description from sysfs.
Since there are some edge cases that can happen, to make sure the
parsing happens as expected in all cases.
The edge cases primarily come from the different led properties that can
be present or absent in devicetree. I have tested some combinations
thereof and would prefer the label to be generated by led sysfs instead
of manually providing the 3-component label.
However for that to work phosphor-led-sysfs must be able to extract the
labels components in all cases.
This modifies the behavior slightly but it will stay the same
for led names that have 1 or 3 components.
Change-Id: I8def089e4c8dc5d3a341cf6f6b1d6356f5aefe48
Signed-off-by: Alexander Hansen <alexander.hansen@9elements.com>
diff --git a/interfaces/internal_interface.cpp b/interfaces/internal_interface.cpp
index 95a61e9..917b8ec 100644
--- a/interfaces/internal_interface.cpp
+++ b/interfaces/internal_interface.cpp
@@ -31,36 +31,21 @@
bus(bus), serverInterface(bus, path, internalInterface, vtable.data(), this)
{}
-void InternalInterface::getLedDescr(const std::string& name, LedDescr& ledDescr)
-{
- std::vector<std::string> words;
- boost::split(words, name, boost::is_any_of(":"));
- try
- {
- ledDescr.devicename = words.at(0);
- ledDescr.color = words.at(1);
- ledDescr.function = words.at(2);
- }
- catch (const std::out_of_range& e)
- {
- lg2::warning("LED description {DESC} not well formed, error {ERR}",
- "DESC", name, "ERR", e.what());
- throw e;
- }
-}
-
std::string InternalInterface::getDbusName(const LedDescr& ledDescr)
{
std::vector<std::string> words;
- words.emplace_back(ledDescr.devicename);
- if (!ledDescr.function.empty())
+ if (ledDescr.devicename.has_value())
{
- words.emplace_back(ledDescr.function);
+ words.emplace_back(ledDescr.devicename.value());
+ }
+ if (ledDescr.function.has_value())
+ {
+ words.emplace_back(ledDescr.function.value());
}
- if (!ledDescr.color.empty())
+ if (ledDescr.color.has_value())
{
- words.emplace_back(ledDescr.color);
+ words.emplace_back(ledDescr.color.value());
}
std::string s = boost::join(words, "_");
@@ -81,17 +66,11 @@
return;
}
+ auto sled = std::make_unique<phosphor::led::SysfsLed>(fs::path(path));
+
// Convert LED name in sysfs into DBus name
- LedDescr ledDescr;
- try
- {
- getLedDescr(ledName, ledDescr);
- }
- catch (...)
- {
- // Ignore the error, for simple LED which was not added in 3-part form.
- // The simple LED can appear with it's plain name
- }
+ const LedDescr ledDescr = sled->getLedDescr();
+
name = getDbusName(ledDescr);
lg2::debug("LED {NAME} receives dbus name {DBUSNAME}", "NAME", ledName,
@@ -106,10 +85,9 @@
return;
}
- auto sled = std::make_unique<phosphor::led::SysfsLed>(fs::path(path));
-
leds.emplace(objPath, std::make_unique<phosphor::led::Physical>(
- bus, objPath, std::move(sled), ledDescr.color));
+ bus, objPath, std::move(sled),
+ ledDescr.color.value_or("")));
}
void InternalInterface::addLED(const std::string& name)
diff --git a/interfaces/internal_interface.hpp b/interfaces/internal_interface.hpp
index 25d530d..a588226 100644
--- a/interfaces/internal_interface.hpp
+++ b/interfaces/internal_interface.hpp
@@ -13,7 +13,6 @@
static constexpr auto busName = "xyz.openbmc_project.LED.Controller";
static constexpr auto ledPath = "/xyz/openbmc_project/led";
static constexpr auto physParent = "/xyz/openbmc_project/led/physical";
-static constexpr auto devParent = "/sys/class/leds/";
static constexpr auto internalInterface =
"xyz.openbmc_project.Led.Sysfs.Internal";
static constexpr auto ledAddMethod = "AddLED";
@@ -73,17 +72,6 @@
leds;
/**
- * @brief Structure to define the LED sysfs name
- */
-
- struct LedDescr
- {
- std::string devicename;
- std::string color;
- std::string function;
- };
-
- /**
* @brief sdbusplus D-Bus connection.
*/
@@ -125,18 +113,6 @@
void createLEDPath(const std::string& ledName);
- /** @brief Parse LED name in sysfs
- *
- * Parse sysfs LED name in format "devicename:colour:function"
- * or "devicename:colour" or "devicename" and sets corresponding
- * fields in LedDescr struct.
- *
- * @param[in] name - LED name in sysfs
- * @param[out] ledDescr - LED description
- */
-
- static void getLedDescr(const std::string& name, LedDescr& ledDescr);
-
/** @brief Generates LED DBus name from LED description
*
* @param[in] name - LED description
diff --git a/sysfs.cpp b/sysfs.cpp
index b1ea456..137218d 100644
--- a/sysfs.cpp
+++ b/sysfs.cpp
@@ -16,9 +16,14 @@
#include "sysfs.hpp"
+#include "phosphor-logging/lg2.hpp"
+
+#include <boost/algorithm/string.hpp>
+
#include <fstream>
-#include <iostream>
+#include <optional>
#include <string>
+#include <vector>
namespace fs = std::filesystem;
@@ -119,5 +124,83 @@
{
setSysfsAttr<unsigned long>(root / attrDelayOff, ms);
}
+
+/* LED sysfs name can be any of
+ *
+ * - devicename:color:function
+ * - devicename::function
+ * - color:function (e.g. "red:fault")
+ * - label (e.g. "identify")
+ * - :function (e.g. ":boot")
+ * - color: (e.g. "green:")
+ *
+ * but no one prevents us from making all of this up and creating
+ * a label with colons inside, e.g. "mylabel:mynoncolorstring:lala".
+ *
+ * Reference: https://www.kernel.org/doc/html/latest/leds/leds-class.html
+ *
+ * Summary: It's bonkers (not my words, but describes it well)
+ */
+LedDescr SysfsLed::getLedDescr()
+{
+ std::string name = std::string(root).substr(strlen(devParent));
+ LedDescr ledDescr;
+
+ std::vector<std::optional<std::string>> words;
+ std::stringstream ss(name);
+ std::string item;
+
+ while (std::getline(ss, item, ':'))
+ {
+ if (item.empty())
+ {
+ words.emplace_back(std::nullopt);
+ }
+ else
+ {
+ words.emplace_back(item);
+ }
+ }
+
+ if (name.ends_with(":"))
+ {
+ words.emplace_back(std::nullopt);
+ }
+
+ if (name.empty())
+ {
+ lg2::warning("LED description '{DESC}' was empty", "DESC", name);
+ throw std::out_of_range("expected non-empty LED name");
+ }
+
+ if (words.size() != 3)
+ {
+ lg2::warning(
+ "LED description '{DESC}' not well formed, expected 3 parts but got {NPARTS}",
+ "DESC", name, "NPARTS", words.size());
+ }
+
+ switch (words.size())
+ {
+ default:
+ case 3:
+ ledDescr.function = words.at(2);
+ ledDescr.color = words.at(1);
+ ledDescr.devicename = words.at(0);
+ break;
+ case 2:
+ ledDescr.color = words.at(0);
+ ledDescr.function = words.at(1);
+ break;
+ case 1:
+ ledDescr.devicename = words.at(0);
+ break;
+ case 0:
+ throw std::out_of_range("expected non-empty LED name");
+ }
+
+ // if there is more than 3 parts we ignore the rest
+ return ledDescr;
+}
} // namespace led
} // namespace phosphor
diff --git a/sysfs.hpp b/sysfs.hpp
index 57c6769..2e078b2 100644
--- a/sysfs.hpp
+++ b/sysfs.hpp
@@ -16,11 +16,25 @@
#pragma once
#include <filesystem>
+#include <optional>
+
+static constexpr auto devParent = "/sys/class/leds/";
namespace phosphor
{
namespace led
{
+
+struct LedDescr
+{
+ // at least one of the members shall be non-empty
+ // after initialization
+
+ std::optional<std::string> devicename;
+ std::optional<std::string> color;
+ std::optional<std::string> function;
+};
+
class SysfsLed
{
public:
@@ -43,6 +57,12 @@
virtual unsigned long getDelayOff();
virtual void setDelayOff(unsigned long ms);
+ /** @brief parse LED name in sysfs
+ * Parse sysfs LED name and sets corresponding
+ * fields in LedDescr struct.
+ */
+ LedDescr getLedDescr();
+
protected:
static constexpr const char* attrBrightness = "brightness";
static constexpr const char* attrMaxBrightness = "max_brightness";
diff --git a/test/meson.build b/test/meson.build
index 574c136..eb82249 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -27,6 +27,7 @@
tests = [
'physical.cpp',
'sysfs.cpp',
+ 'test_led_description.cpp',
]
foreach t : tests
@@ -43,4 +44,4 @@
]
)
)
-endforeach
\ No newline at end of file
+endforeach
diff --git a/test/test_led_description.cpp b/test/test_led_description.cpp
new file mode 100644
index 0000000..708794b
--- /dev/null
+++ b/test/test_led_description.cpp
@@ -0,0 +1,92 @@
+/**
+ * Copyright © 2024 9elements
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "interfaces/internal_interface.hpp"
+#include "sysfs.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace phosphor::led;
+
+static LedDescr runtest(const std::string& name)
+{
+ std::string path = devParent + name;
+ SysfsLed led(path);
+ return led.getLedDescr();
+}
+
+TEST(LEDDescr, Has4PartsInvalid)
+{
+ LedDescr d = runtest("devicename:color:function:part4");
+
+ ASSERT_EQ("devicename", d.devicename);
+ ASSERT_EQ("color", d.color);
+ ASSERT_EQ("function", d.function);
+}
+
+TEST(LEDDescr, Has3Parts)
+{
+ LedDescr d = runtest("devicename:color:function");
+
+ ASSERT_EQ("devicename", d.devicename);
+ ASSERT_EQ("color", d.color);
+ ASSERT_EQ("function", d.function);
+}
+
+TEST(LEDDescr, Has2PartsColorFunction)
+{
+ LedDescr d = runtest("red:fault");
+
+ ASSERT_EQ(std::nullopt, d.devicename);
+ ASSERT_EQ("red", d.color);
+ ASSERT_EQ("fault", d.function);
+}
+
+TEST(LEDDescr, Has2PartsDevicenameFunction)
+{
+ LedDescr d = runtest("input9::capslock");
+
+ ASSERT_EQ("input9", d.devicename);
+ ASSERT_EQ(std::nullopt, d.color);
+ ASSERT_EQ("capslock", d.function);
+}
+
+TEST(LEDDescr, Has1PartColor)
+{
+ LedDescr d = runtest("green:");
+
+ ASSERT_EQ(std::nullopt, d.devicename);
+ ASSERT_EQ("green", d.color);
+ ASSERT_EQ(std::nullopt, d.function);
+}
+
+TEST(LEDDescr, Has1PartFunction)
+{
+ LedDescr d = runtest(":boot");
+
+ ASSERT_EQ(std::nullopt, d.devicename);
+ ASSERT_EQ(std::nullopt, d.color);
+ ASSERT_EQ("boot", d.function);
+}
+
+TEST(LEDDescr, Has1PartLabel)
+{
+ LedDescr d = runtest("identify");
+
+ ASSERT_EQ("identify", d.devicename);
+ ASSERT_EQ(std::nullopt, d.color);
+ ASSERT_EQ(std::nullopt, d.function);
+}