add a hardware backlight interface and gui controls #14970

lrusak · 2018-11-29T18:09:13Z

This adds a new interface to allow adjusting a hardware backlight (like those found in laptops or touchscreens). This change required a rework of the sysfs utility to comply with the kernel documentation (as interpreted by me). This backlight interface is also quite invasive as it touches many parts of Kodi internals so it could use a thorough review as maybe I did more than what is needed.

This change add no new dependencies or ifdefs. The interface is opt in and requires registration. The backlight utility can be used for amlogic and rpi windowing too if someone wants to implement that.

I also have basically zero knowledge of kodi dialogs and skinning, this is the first time I have touched these areas and am open for input about these changes. I also created the backlight icon myself so if someone wants to do it better feel free 🎨

This image shows both the touch screen interface and the onscreen indicator (hidden between the gallium hud elements).

I will outline the commits below with their messages:

SysfsUtils: cleanup and improve interface according to the documentation

as written in the kernel docs:

sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the
method. Sysfs will call the method exactly once for each read or
write.

When writing sysfs files, userspace processes should first read the
entire file, modify the values it wishes to change, then write the
entire buffer back.

for more info please consult the kernel documentation:

https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt

Add new backlight interface and gui

this adds a new interface to be able to access a hardware
backlight. this also adds the gui elements needed to show
the backlight brightness change and to change it via the
touch screen interface. this change is quite invasive but
it is based off the way the volume control works.

the backlight controls will not be show if no backlight
has been register to CServiceBroker

CLibInputKeyboard: add brightness keys

this adds the keyboard backlight brightness keys to the libinput
keyboard mapping.

CBacklight: add linux backlight helper utility

this utility interacts with the kernel sysfs backlight control
by querying the backlight device and matching it to the drm device.

windowing/gbm: use CBacklight

add the linux backlight utility to gbm windowing.

garbear · 2018-12-01T16:44:36Z

I see you're entered the realm of the GUI :) Still time constrained so I can't offer much feedback than that, but good job. Cool feature!

enen92 · 2018-12-01T20:27:27Z

xbmc/GUIInfoManager.cpp

@@ -1645,7 +1645,9 @@ const infomap system_labels[] =  {{ "hasnetwork",       SYSTEM_ETHERNET_LINK_ACT
                                  { "stereoscopicmode", SYSTEM_STEREOSCOPIC_MODE },
                                  { "hascms",           SYSTEM_HAS_CMS },
                                  { "privacypolicy",    SYSTEM_PRIVACY_POLICY },
-                                  { "haspvraddon",      SYSTEM_HAS_PVR_ADDON }};
+                                  { "haspvraddon",      SYSTEM_HAS_PVR_ADDON },
+                                  { "hasbacklight",     SYSTEM_HAS_BACKLIGHT },


Missing doxygen entry at the top of the map like all the others

a1rwulf · 2018-12-04T20:35:09Z

xbmc/utils/SysfsUtils.h

-  static int SetInt(const std::string& path, const int val);
-  static int GetInt(const std::string& path, int& val);
+  static bool SetString(const std::string& path, const std::string& value);
+  static const std::string GetString(const std::string& path);


Any reason why you return const std::string and not std::string ?
Same for the const int below.

+1; never return const

a1rwulf · 2018-12-04T20:43:15Z

xbmc/platform/linux/Backlight.cpp

+  }
+
+  return false;
+}


Make sure to call closedir after you finished.

a1rwulf · 2018-12-04T20:49:47Z

xbmc/utils/IBacklight.h

+  static void Register(std::shared_ptr<IBacklight> backlight) { CServiceBroker::RegisterBacklight(backlight); }
+
+  virtual bool SetBrightness(int brightness) = 0;
+  virtual int GetBrightness() = 0;


While the implementation actually hasn't even a member for it, I'd still make this
virtual int GetBrightness() const = 0;
Same for other Getters.

a1rwulf · 2018-12-04T20:52:38Z

Please make sure the IBacklight interface is already there when you implement CBacklight so it doesn't break bisect.

yol

Lots of minors, but at the very least you have to split the commits differently so that they follow logically and you can bisect.

👍 for changing the SysfsUtils interface :-)

yol · 2018-12-13T16:06:30Z

xbmc/utils/AMLUtils.cpp

  {
-    if (SysfsUtils::GetString("/sys/class/amhdmitx/amhdmitx0/disp_cap", valstr) < 0)
+    std::string valstr = SysfsUtils::GetString("/sys/class/amhdmitx/amhdmitx0/disp_cap");


Variable redeclaration (curious that you didn't get a warning at least)

yol · 2018-12-13T16:07:15Z

xbmc/utils/AMLUtils.cpp

      return false;

-    if (SysfsUtils::GetString("/sys/class/amhdmitx/amhdmitx0/vesa_cap", vesastr) == 0)
+    std::string vesastr = SysfsUtils::GetString("/sys/class/amhdmitx/amhdmitx0/vesa_cap");
+    if (vesastr.length() > 0)


100% unimportant, but out of curiosity: Why not !vesastr.empty()? (considering the check 4 lines above)

yol · 2018-12-13T16:10:08Z

xbmc/utils/SysfsUtils.cpp

-    if (write(fd, valstr.c_str(), valstr.size()) < 0)
-      ret = -1;
-    close(fd);
+    return true;


Are you sure this is a good idea? Does the kernel interface guarantee that writing a value that already "seems" to be there never has any (desired) side-effect?

yol · 2018-12-13T16:10:45Z

xbmc/utils/SysfsUtils.cpp

-#ifdef TARGET_WINDOWS_STORE
-#include <io.h>
-#endif
+static const unsigned int PAGESIZE = getpagesize();


namespace {} instead of static

yol · 2018-12-13T16:12:22Z

xbmc/utils/SysfsUtils.cpp

+  int fd = open(path.c_str(), O_RDWR);
+  if (fd < 0)
+  {
+    CLog::Log(LOGERROR, "{}: error opening {}", __FUNCTION__, path);


I think with SetString: error opening xxxxx the SetString: prefix is not going to be especially helpful. Rather write something descriptive (sysfs utils or the like).

yol · 2018-12-13T16:33:47Z

xbmc/dialogs/GUIDialogBacklightBar.cpp

@@ -0,0 +1,47 @@
+/*
+ *  Copyright (C) 2005-2018 Team Kodi


yol · 2018-12-13T16:33:57Z

xbmc/dialogs/GUIDialogBacklightBar.h

@@ -0,0 +1,19 @@
+/*
+ *  Copyright (C) 2005-2018 Team Kodi


yol · 2018-12-13T16:35:24Z

xbmc/interfaces/builtins/ApplicationBuiltins.cpp

@@ -85,6 +85,20 @@ static int NotifyAll(const std::vector<std::string>& params)
  return 0;
 }

+/*! \brief Set volume.


yol · 2018-12-13T16:36:27Z

xbmc/utils/IBacklight.h

+class IBacklight
+{
+public:
+  static void Register(std::shared_ptr<IBacklight> backlight) { CServiceBroker::RegisterBacklight(backlight); }


I'm not completely sure what the purpose of this function is?
EDIT: to clarify, why is having this better than just calling CServiceBroker::RegisterBacklight?

yol · 2018-12-13T16:36:56Z

xbmc/platform/linux/CMakeLists.txt

@@ -27,6 +25,11 @@ set(HEADERS Backlight.h
            XMemUtils.h
            XTimeUtils.h)

+if(CORE_SYSTEM_NAME STREQUAL linux)


Why shouldn't it be linux in this directory?

phunkyfish · 2020-03-08T20:35:38Z

@lrusak do you want to progress this, close it or backburner?

lrusak · 2020-03-21T17:30:50Z

I would still like to see this get merged in, I'll revisit it at some point

this adds a new interface to be able to access a hardware backlight. this also adds the gui elements needed to show the backlight brightness change and to change it via the touch screen interface. this change is quite invasive but it is based off the way the volume control works. the backlight controls will not be show if no backlight has been register to CServiceBroker

this adds the keyboard backlight brightness keys to the libinput keyboard mapping.

this utility interacts with the kernel sysfs backlight control by querying the backlight device and matching it to the drm device.

add the linux backlight utility to gbm windowing.

garbear · 2020-09-23T05:07:22Z

xbmc/utils/IBacklight.h

+
+  virtual bool SetBrightness(int brightness) = 0;
+  virtual int GetBrightness() const = 0;
+  virtual int GetMaxBrightness() const = 0;


int is not very descriptive of what a brightness is, so could use some documentation. typing could also improve semantics. could I suggest a float in the range [0, 1]?

garbear · 2020-09-23T05:08:13Z

xbmc/input/actions/ActionIDs.h

@@ -337,6 +337,9 @@

 #define ACTION_HDR_TOGGLE 260 //!< Toggle display HDR on/off

+#define ACTION_BACKLIGHT_BRIGHTNESS_UP 251


ID is a duplicate of ACTION_QUEUE_ITEM_NEXT, you probably want 261

garbear · 2020-09-23T05:09:08Z

xbmc/guilib/guiinfo/SystemGUIInfo.cpp

@@ -645,6 +653,10 @@ bool CSystemGUIInfo::GetBool(bool& value, const CGUIListItem *gitem, int context
      }
      break;
    }
+    case SYSTEM_HAS_BACKLIGHT:
+      std::shared_ptr<IBacklight> backlight = CServiceBroker::GetBacklight();
+      value = !!backlight;


maybe static_cast<bool>(backlight) to be a little more explicit about what is going on here?

garbear · 2020-09-23T05:13:20Z

xbmc/dialogs/GUIDialogBacklightBar.cpp

+      return CGUIDialog::OnMessage(message);
+  }
+
+  return false; // don't process anything other than what we need!


it just saddens me to see lengthy backlight logic added to CApplication. With your work, we now have a beautiful minimal file to add logic to. What if we move all backlight logic from CApplication into this file?

garbear · 2020-09-23T05:37:23Z

Glad to see you've revisited this. please, please, please don't increase the size of service broker. these are effectively globals (ok, singletons with global scope, so their lifetime is managed relative to just each other, yay?). let's get creative in how we can not nuke architecture. remember, architecture is not what you build, but what you restrict.

Looking at the technical debt we've currently saddled service broken with, I see two promising options. Peripheral manager and power manager have something to do with backlight. Maybe we can use these.

I'm not that familiar with power manager, but it seems less appropriate for backlight control. backlight changes are not always done from a power perspective. and from a high level, backlight is just responding to user input and controlling hardware. Power manager controls hardware too, but that seems like kind of a stretch.

To use peripheral manager, there seems to be some overhead. this requires treating the display as a peripheral, so we need a class that might not be used for anything else. it doesn't make sense to "scan" for displays, because there can only ever be one (as distinct from monitors). I actually faced this with keyboards and mice and created a virtual "application" bus that has one keyboard and mouse per application. Scanning the "application" bus could also return a monitor/display. And we're in luck. All peripherals can see all actions.

Maybe peripherals, made for hardware, is a good place for hardware backlight control?

ronie · 2020-09-23T16:35:14Z

addons/skin.estuary/xml/Custom_1110_BacklightSlider.xml

@@ -0,0 +1,71 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<window type="dialog" id="1110">


id="1110" is already in use, please use 1106 instead

ronie · 2020-09-23T16:35:54Z

addons/skin.estuary/xml/Custom_1110_BacklightSlider.xml

@@ -0,0 +1,71 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<window type="dialog" id="1110">
+	<defaultcontrol>11</defaultcontrol>


drop this line, there's no control with id="11" in this dialog

ronie · 2020-09-23T16:39:57Z

addons/skin.estuary/xml/Includes.xml

+					<width>120</width>
+					<height>120</height>
+					<label></label>
+					<onclick>ActivateWindow(1110)</onclick>


change to 1106 here as well please

jenkins4kodi · 2020-10-14T09:43:19Z

@lrusak this needs a rebase

lrusak added Type: Feature non-breaking change which adds functionality Platform: Linux v19 Matrix labels Nov 29, 2018

lrusak requested review from ronie, ksooo, yol and a1rwulf November 29, 2018 18:09

lrusak mentioned this pull request Dec 1, 2018

GBM roadmap, todo list and issue list #14876

Open

7 tasks

enen92 reviewed Dec 1, 2018

View reviewed changes

a1rwulf reviewed Dec 4, 2018

View reviewed changes

xbmc/platform/linux/Backlight.cpp

}

return false;

}

Copy link

Member

a1rwulf Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to call closedir after you finished.

a1rwulf reviewed Dec 4, 2018

View reviewed changes

yol requested changes Dec 13, 2018

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 19, 2019

lrusak mentioned this pull request May 8, 2019

SysfsUtils: cleanup and improve interface according to the documentation #16097

Merged

phunkyfish added the PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. label Mar 8, 2020

phunkyfish added PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. and removed PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. labels Mar 23, 2020

lrusak added 7 commits September 22, 2020 17:27

CLibInputKeyboard: add brightness keys

6dab123

this adds the keyboard backlight brightness keys to the libinput keyboard mapping.

platform/linux: CSysfsPath make get const correct

7f135db

platform/linux: CSysfsPath add setter template

9d475a3

CBacklight: add linux backlight helper utility

a1066bf

this utility interacts with the kernel sysfs backlight control by querying the backlight device and matching it to the drm device.

windowing/gbm: use CBacklight

757b29d

add the linux backlight utility to gbm windowing.

[test] TestSysfsPath: fix number comparisons

7b7bf4e

lrusak force-pushed the sysfs branch from ff25c90 to 7b7bf4e Compare September 23, 2020 00:50

lrusak requested a review from garbear as a code owner September 23, 2020 00:50

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Sep 23, 2020

garbear reviewed Sep 23, 2020

View reviewed changes

ronie reviewed Sep 23, 2020

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 14, 2020

lrusak added v20 Nexus and removed v19 Matrix labels Jan 2, 2021

This comment has been minimized.

Sign in to view

fuzzard removed the v20 Nexus label Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a hardware backlight interface and gui controls #14970

add a hardware backlight interface and gui controls #14970

lrusak commented Nov 29, 2018

garbear commented Dec 1, 2018

enen92 Dec 1, 2018

a1rwulf Dec 4, 2018

yol Dec 13, 2018

a1rwulf Dec 4, 2018

a1rwulf Dec 4, 2018

a1rwulf commented Dec 4, 2018

yol left a comment

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018

yol Dec 13, 2018 •

edited

yol Dec 13, 2018

phunkyfish commented Mar 8, 2020

lrusak commented Mar 21, 2020

garbear Sep 23, 2020

garbear Sep 23, 2020

garbear Sep 23, 2020

garbear Sep 23, 2020

garbear commented Sep 23, 2020

ronie Sep 23, 2020

ronie Sep 23, 2020

ronie Sep 23, 2020

jenkins4kodi commented Oct 14, 2020

This comment has been minimized.

		@@ -337,6 +337,9 @@

		#define ACTION_HDR_TOGGLE 260 //!< Toggle display HDR on/off

		#define ACTION_BACKLIGHT_BRIGHTNESS_UP 251

		@@ -0,0 +1,71 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<window type="dialog" id="1110">

add a hardware backlight interface and gui controls #14970

Are you sure you want to change the base?

add a hardware backlight interface and gui controls #14970

Conversation

lrusak commented Nov 29, 2018

SysfsUtils: cleanup and improve interface according to the documentation

Add new backlight interface and gui

CLibInputKeyboard: add brightness keys

CBacklight: add linux backlight helper utility

windowing/gbm: use CBacklight

garbear commented Dec 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a1rwulf commented Dec 4, 2018

yol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yol Dec 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phunkyfish commented Mar 8, 2020

lrusak commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbear commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkins4kodi commented Oct 14, 2020

This comment has been minimized.

yol Dec 13, 2018 •

edited