From e185efb810f0befb26276c776d71ed8e56059a1a Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Thu, 24 May 2018 12:59:06 +0930 Subject: Add sysfs LED class wrapper The Physical class implements private templated read() and write() methods. There are several properties that make this approach less than ideal: 1. read() and write() are non-virtual template functions, and we would have to link-seam mock the internals, which means hijacking std::fstream. 2. Even if read() and write() were virtual functions, this is made irrelevant by the fact that we want to traverse execution paths in setInitialState(), which is called from the Physical constructor. Methods invoked by class constructors are bound to the implementations specified in the constructor's class and will never dispatch to a descendant's override. As such we have no mechanism to manipulate the execution path without resorting to the pre-processor seam, which is undesirable for other reasons. 3. The abstraction is poor. Physical implements the business logic of converting interactions with the DBus interface into compatible interactions with the sysfs LED class attributes, and shouldn't have knowledge of how to directly interact with sysfs itself. 4. read() and write() are template functions, but the only type parameter used in the code-base is std::string, and conversions are left to the caller. This needlessly complicates the caller logic and reduces readability of the callee code. The change defines a separate class, SysfsLed, to map actions onto the LED sysfs class attributes. SysfsLed will be provided by the dependency-injection pattern to the Physical class by passing an instance reference through its constructor. The lifetime of the SysfsLed instance must exceed the lifetime of the associated Physical instance. Further, the methods of SysfsLed are all marked as virtual and defined to return concrete types (either unsigned long or std::string as appropriate). This opens the door for mocking without resorting to techniques such as using link seams, and removes templates as a point of complication. Further, defining only a concrete class and not an abstract base class minimises the boilerplate required as we're likely never going to have another descendant of SysfsLed that isn't a mock implementation (we don't need to exclude implementations by way of sibling types). Integration tests are provided for SysfsLed, which is necessary as the class must write to the filesystem (again, unless we want to hijack std::fstream, which seems unpalatable). Isolated temporary directories are used to ensure the tests can be run in parallel without interference. The tests provide 100% line coverage of SysfsLed. Change-Id: I81fc7d9fd07eed54035f515502f563f25aa1e58f Signed-off-by: Andrew Jeffery --- sysfs.cpp | 107 +++++++++++++++++++++++++++++++++++++++++++++ sysfs.hpp | 55 +++++++++++++++++++++++ test/Makefile.am.include | 8 +++- test/sysfs.cpp | 111 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 sysfs.cpp create mode 100644 sysfs.hpp create mode 100644 test/sysfs.cpp diff --git a/sysfs.cpp b/sysfs.cpp new file mode 100644 index 0000000..44ad8d6 --- /dev/null +++ b/sysfs.cpp @@ -0,0 +1,107 @@ +/** + * Copyright © 2019 IBM Corporation + * + * 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 "sysfs.hpp" + +#include +#include +#include + +namespace fs = std::experimental::filesystem; + +namespace phosphor +{ +namespace led +{ + +constexpr char SysfsLed::BRIGHTNESS[]; +constexpr char SysfsLed::MAX_BRIGHTNESS[]; +constexpr char SysfsLed::TRIGGER[]; +constexpr char SysfsLed::DELAY_ON[]; +constexpr char SysfsLed::DELAY_OFF[]; + +template +T get_sysfs_attr(const fs::path& path); + +template <> +std::string get_sysfs_attr(const fs::path& path) +{ + std::string content; + std::ifstream file(path); + file >> content; + return content; +} + +template <> +unsigned long get_sysfs_attr(const fs::path& path) +{ + std::string content = get_sysfs_attr(std::move(path)); + return std::strtoul(content.c_str(), nullptr, 0); +} + +template +void set_sysfs_attr(const fs::path& path, const T& value) +{ + std::ofstream file(path); + file << value; +} + +unsigned long SysfsLed::getBrightness() +{ + return get_sysfs_attr(root / BRIGHTNESS); +} + +void SysfsLed::setBrightness(unsigned long brightness) +{ + set_sysfs_attr(root / BRIGHTNESS, brightness); +} + +unsigned long SysfsLed::getMaxBrightness() +{ + return get_sysfs_attr(root / MAX_BRIGHTNESS); +} + +std::string SysfsLed::getTrigger() +{ + return get_sysfs_attr(root / TRIGGER); +} + +void SysfsLed::setTrigger(const std::string& trigger) +{ + set_sysfs_attr(root / TRIGGER, trigger); +} + +unsigned long SysfsLed::getDelayOn() +{ + return get_sysfs_attr(root / DELAY_ON); +} + +void SysfsLed::setDelayOn(unsigned long ms) +{ + set_sysfs_attr(root / DELAY_ON, ms); +} + +unsigned long SysfsLed::getDelayOff() +{ + return get_sysfs_attr(root / DELAY_OFF); +} + +void SysfsLed::setDelayOff(unsigned long ms) +{ + set_sysfs_attr(root / DELAY_OFF, ms); +} +} // namespace led +} // namespace phosphor diff --git a/sysfs.hpp b/sysfs.hpp new file mode 100644 index 0000000..290c97f --- /dev/null +++ b/sysfs.hpp @@ -0,0 +1,55 @@ +/** + * Copyright © 2019 IBM Corporation + * + * 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. + */ + +#pragma once +#include + +namespace phosphor +{ +namespace led +{ +class SysfsLed +{ + public: + SysfsLed(std::experimental::filesystem::path&& root) : root(std::move(root)) + { + } + SysfsLed() = delete; + SysfsLed(const SysfsLed& other) = delete; + + virtual ~SysfsLed() = default; + + virtual unsigned long getBrightness(); + virtual void setBrightness(unsigned long value); + virtual unsigned long getMaxBrightness(); + virtual std::string getTrigger(); + virtual void setTrigger(const std::string& trigger); + virtual unsigned long getDelayOn(); + virtual void setDelayOn(unsigned long ms); + virtual unsigned long getDelayOff(); + virtual void setDelayOff(unsigned long ms); + + protected: + static constexpr char BRIGHTNESS[] = "brightness"; + static constexpr char MAX_BRIGHTNESS[] = "max_brightness"; + static constexpr char TRIGGER[] = "trigger"; + static constexpr char DELAY_ON[] = "delay_on"; + static constexpr char DELAY_OFF[] = "delay_off"; + + std::experimental::filesystem::path root; +}; +} // namespace led +} // namespace phosphor diff --git a/test/Makefile.am.include b/test/Makefile.am.include index dd41dd8..95fe19e 100644 --- a/test/Makefile.am.include +++ b/test/Makefile.am.include @@ -14,6 +14,7 @@ AM_CXXFLAGS = \ $(PHOSPHOR_DBUS_INTERFACES_CXXFLAGS) \ $(CODE_COVERAGE_CXXFLAGS) AM_LDFLAGS = \ + -lstdc++fs \ $(SDBUSPLUS_LIBS) \ $(PHOSPHOR_DBUS_INTERFACES_LIBS) \ $(CODE_COVERAGE_LIBS) \ @@ -22,4 +23,9 @@ AM_LDFLAGS = \ test_physical_SOURCES = %reldir%/physical.cpp test_physical_LDADD = physical.o -check_PROGRAMS += %reldir%/physical +test_sysfs_SOURCES = %reldir%/sysfs.cpp +test_sysfs_LDADD = sysfs.cpp + +check_PROGRAMS += \ + %reldir%/physical \ + %reldir%/sysfs diff --git a/test/sysfs.cpp b/test/sysfs.cpp new file mode 100644 index 0000000..e8339e7 --- /dev/null +++ b/test/sysfs.cpp @@ -0,0 +1,111 @@ +/** + * Copyright © 2019 IBM Corporation + * + * 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 "sysfs.hpp" + +#include + +#include +#include +#include + +#include + +namespace fs = std::experimental::filesystem; + +constexpr unsigned long MAX_BRIGHTNESS_VAL = 128; + +class FakeSysfsLed : public phosphor::led::SysfsLed +{ + public: + static FakeSysfsLed create() + { + static constexpr auto tmplt = "/tmp/FakeSysfsLed.XXXXXX"; + char buffer[MAXPATHLEN] = {0}; + + strncpy(buffer, tmplt, sizeof(buffer) - 1); + char* dir = mkdtemp(buffer); + if (!dir) + throw std::system_error(errno, std::system_category()); + + return FakeSysfsLed(fs::path(dir)); + } + + ~FakeSysfsLed() + { + fs::remove_all(root); + } + + private: + FakeSysfsLed(fs::path&& path) : SysfsLed(std::move(path)) + { + std::string attrs[4] = {BRIGHTNESS, TRIGGER, DELAY_ON, DELAY_OFF}; + for (const auto& attr : attrs) + { + fs::path p = root / attr; + std::ofstream f(p, std::ios::out); + f.exceptions(f.failbit); + } + + fs::path p = root / MAX_BRIGHTNESS; + std::ofstream f(p, std::ios::out); + f.exceptions(f.failbit); + f << MAX_BRIGHTNESS_VAL; + } +}; + +TEST(Sysfs, getBrightness) +{ + constexpr unsigned long brightness = 127; + FakeSysfsLed fsl = FakeSysfsLed::create(); + + fsl.setBrightness(brightness); + ASSERT_EQ(brightness, fsl.getBrightness()); +} + +TEST(Sysfs, getMaxBrightness) +{ + FakeSysfsLed fsl = FakeSysfsLed::create(); + + ASSERT_EQ(MAX_BRIGHTNESS_VAL, fsl.getMaxBrightness()); +} + +TEST(Sysfs, getTrigger) +{ + constexpr auto trigger = "none"; + FakeSysfsLed fsl = FakeSysfsLed::create(); + + fsl.setTrigger(trigger); + ASSERT_EQ(trigger, fsl.getTrigger()); +} + +TEST(Sysfs, getDelayOn) +{ + constexpr unsigned long delayOn = 250; + FakeSysfsLed fsl = FakeSysfsLed::create(); + + fsl.setDelayOn(delayOn); + ASSERT_EQ(delayOn, fsl.getDelayOn()); +} + +TEST(Sysfs, getDelayOff) +{ + constexpr unsigned long delayOff = 750; + FakeSysfsLed fsl = FakeSysfsLed::create(); + + fsl.setDelayOff(delayOff); + ASSERT_EQ(delayOff, fsl.getDelayOff()); +} -- cgit v1.2.1