diff mbox series

[matchbox-panel-2,v4] panel: added sysfs support

Message ID 20241107161901.43148-1-dixitparmar19@gmail.com
State New
Headers show
Series [matchbox-panel-2,v4] panel: added sysfs support | expand

Commit Message

Dixit Parmar Nov. 7, 2024, 4:19 p.m. UTC
battery applet supports reading battery and supply
information from sysfs entries, /sys/class/power_supply/.

[YOCTO #12904]
https://bugzilla.yoctoproject.org/show_bug.cgi?id=12904

Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
---
 applets/battery/Makefile.am     |   4 +
 applets/battery/battery-sysfs.c | 142 ++++++++++++++++++++++++++++++++
 applets/battery/battery.h       |   3 +-
 configure.ac                    |   4 +
 4 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 applets/battery/battery-sysfs.c

Comments

Dixit Parmar Nov. 28, 2024, 5:34 p.m. UTC | #1
Hi Ross,
Can you please review if everything looks good.
Ross Burton June 23, 2025, 4:02 p.m. UTC | #2
On 7 Nov 2024, at 16:19, Dixit Parmar via lists.yoctoproject.org <dixitparmar19=gmail.com@lists.yoctoproject.org> wrote:
> battery applet supports reading battery and supply
> information from sysfs entries, /sys/class/power_supply/.

Yay!

> +++ b/applets/battery/battery-sysfs.c
> @@ -0,0 +1,142 @@

> +static char batt_dev[128] = "";
> +static char ac_dev[128] = “";

Hardcoded and relatively-small buffer sizes make me anxious. These strings are dynamically allocated, so don’t use fixed-size buffers and just use heap allocations everywhere.

> +int read_file(const char *dev, const char *f, char *buff, size_t len)

This should be a static function.

> +{
> + if(dev != NULL && f != NULL && buff != NULL) {
> + gchar *file_path = g_build_filename(PROCFS_PS_DIR, dev, f, NULL);
> + gchar *file_content = NULL;
> +                gsize length;
> + if (g_file_get_contents(file_path, &file_content, &length, NULL)) {
> + if(length <= len) {
> + memset(buff, 0, len);
> + memcpy(buff, file_content, length);
> + } else {
> + length = -1;
> + }

Why do we need to copy the newly allocated buffer and then free it?  I think this function should basically just be g_build_filename;g_file_get_contents();g_free(filename);

> + /* Check battery presence */
> + ret = read_file(batt_dev, "present", buff, sizeof(buff));
> + if(ret > 0) {
> + batt_present = g_ascii_strtoll(buff, &endptr, 10);

This pattern of read_file/stroll() happens a few times, pull that out to a little function.

> --- a/applets/battery/battery.h
> +++ b/applets/battery/battery.h
> @@ -6,8 +6,9 @@
>
> #ifndef MB_APPLET_BATTERY_H
> #define MB_APPLET_BATTERY_H
> -
> +#include <stdio.h>
> #include <string.h>
> +#include <stdlib.h>

The header should only include what is needed for the header: string.h shouldn’t be in there, and stdio/stdlib should be included in the .c if needed (I can’t see any stdio references though).

Sorry again for the incredibly late review!

Thanks,
Ross
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff mbox series

Patch

diff --git a/applets/battery/Makefile.am b/applets/battery/Makefile.am
index 9b02240..1b93a0b 100644
--- a/applets/battery/Makefile.am
+++ b/applets/battery/Makefile.am
@@ -5,6 +5,10 @@  applet_LTLIBRARIES = libbattery.la
 libbattery_la_SOURCES = battery.c battery.h
 libbattery_la_CPPFLAGS = $(AM_CPPFLAGS) -DDATADIR=\"$(pkgdatadir)/battery/\"
 
+if HAVE_SYSFS
+libbattery_la_SOURCES += battery-sysfs.c
+endif
+
 if HAVE_LIBAPM
 libbattery_la_LIBADD = -lapm
 libbattery_la_SOURCES += battery-apm.c
diff --git a/applets/battery/battery-sysfs.c b/applets/battery/battery-sysfs.c
new file mode 100644
index 0000000..c71d8cf
--- /dev/null
+++ b/applets/battery/battery-sysfs.c
@@ -0,0 +1,142 @@ 
+/*
+ * (C) Dixit Parmar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Licensed under the GPL v2 or greater.
+ */
+
+#include "battery.h"
+#include <glib.h>
+
+#define PROCFS_PS_DIR "/sys/class/power_supply"
+
+static char batt_dev[128] = "";
+static char ac_dev[128] = "";
+static char batt_exist = 0;
+static char ac_exist = 0;
+
+int pm_support(void)
+{
+	GDir *dir = g_dir_open(PROCFS_PS_DIR, 0, NULL);
+	if (dir != NULL) {
+		const gchar *subdir_name;
+		while ((subdir_name = g_dir_read_name(dir)) != NULL) {
+			gchar *subdir_path = g_build_filename(PROCFS_PS_DIR, subdir_name, NULL);
+			// Check if it's a directory
+			if (g_file_test(subdir_path, G_FILE_TEST_IS_DIR)) {
+				gchar *type_file_path = g_build_filename(subdir_path, "type", NULL);
+				// Check if the "type" file exists
+				if (g_file_test(type_file_path, G_FILE_TEST_EXISTS)) {
+					gchar *file_content = NULL;
+					gsize length;
+
+					if (g_file_get_contents(type_file_path, &file_content, &length, NULL)) {
+						if(!batt_exist && strncmp(file_content, "Battery", strlen("Battery")) == 0) {
+							strncpy(batt_dev, subdir_name, strlen(subdir_name));
+							batt_exist = 1;
+						}
+						if(!ac_exist && strncmp(file_content, "Mains", strlen("Mains")) == 0) {
+							strncpy(ac_dev, subdir_name, strlen(subdir_name));
+							ac_exist = 1;
+						}
+						g_free(file_content);
+					}
+				}
+				g_free(type_file_path);
+			}
+			g_free(subdir_path);
+			if(batt_exist && ac_exist)
+				break;
+		}
+		g_dir_close(dir);
+	}
+	return (int)batt_exist;
+}
+
+int read_file(const char *dev, const char *f, char *buff, size_t len)
+{
+	if(dev != NULL && f != NULL && buff != NULL) {
+		gchar *file_path = g_build_filename(PROCFS_PS_DIR, dev, f, NULL);
+		gchar *file_content = NULL;
+                gsize length;
+		if (g_file_get_contents(file_path, &file_content, &length, NULL)) {
+			if(length <= len) {
+				memset(buff, 0, len);
+				memcpy(buff, file_content, length);
+			} else {
+				length = -1;
+			}
+		}
+		g_free(file_content);
+		g_free(file_path);
+		return length;
+	}
+	return -1;
+}
+
+const char* pm_battery_icon(void)
+{
+	const char *icon;
+	char buff[128] = "";
+	int ret = 0;
+
+	gchar *endptr;
+	gint64 ac_sts = 0;
+	gint64 batt_present = 0;
+	gint64 batt_percentage = 0;
+
+	if(batt_exist == 0)
+		return NULL;
+
+	/* Check battery presence */
+	ret = read_file(batt_dev, "present", buff, sizeof(buff));
+	if(ret > 0) {
+		batt_present = g_ascii_strtoll(buff, &endptr, 10);
+	}
+	if(batt_present == 0) {
+		/* Battery is removed */
+		icon = "ac-adapter.png";
+		return icon;
+	}
+
+	/* Check Adaptor status*/
+	ret = read_file(ac_dev, "online", buff, sizeof(buff));
+	if(ret > 0)
+		ac_sts = g_ascii_strtoll(buff, &endptr, 10);;
+
+	/* Check battery percentage */
+	ret = read_file(batt_dev, "capacity", buff, sizeof(buff));
+	if(ret > 0)
+		batt_percentage = g_ascii_strtoll(buff, &endptr, 10);
+
+	if(ac_exist && ac_sts == 1) {
+		/* We're charging */
+		if (batt_percentage < 10)
+			icon = "battery-charging-000.png";
+		else if (batt_percentage < 30)
+			icon = "battery-charging-020.png";
+		else if (batt_percentage < 50)
+			icon = "battery-charging-040.png";
+		else if (batt_percentage < 70)
+			icon = "battery-charging-060.png";
+		else if (batt_percentage < 90)
+			icon = "battery-charging-080.png";
+		else
+			icon = "battery-charging-100.png";
+	} else {
+		if (batt_percentage < 10)
+			icon = "battery-discharging-000.png";
+		else if (batt_percentage < 30)
+			icon = "battery-discharging-020.png";
+		else if (batt_percentage < 50)
+			icon = "battery-discharging-040.png";
+		else if (batt_percentage < 70)
+			icon = "battery-discharging-060.png";
+		else if (batt_percentage < 90)
+			icon = "battery-discharging-080.png";
+		else
+			icon = "battery-discharging-100.png";
+	}
+	return icon;
+}
diff --git a/applets/battery/battery.h b/applets/battery/battery.h
index cf23431..a941aec 100644
--- a/applets/battery/battery.h
+++ b/applets/battery/battery.h
@@ -6,8 +6,9 @@ 
 
 #ifndef MB_APPLET_BATTERY_H
 #define MB_APPLET_BATTERY_H
-
+#include <stdio.h>
 #include <string.h>
+#include <stdlib.h>
 
 int pm_support(void);
 const char* pm_battery_icon(void);
diff --git a/configure.ac b/configure.ac
index d6c57d8..7038212 100644
--- a/configure.ac
+++ b/configure.ac
@@ -61,6 +61,9 @@  AC_ARG_WITH(
 )
 
 case "$with_battery" in
+     "sysfs") enable_linux_sysfs=yes
+     enable_battery=yes
+     ;;
      "acpi") AC_CHECK_HEADERS(libacpi.h, enable_linux_acpi=yes, AC_MSG_FAILURE([You need to install libacpi]))
      enable_battery=yes
      ;;
@@ -75,6 +78,7 @@  esac
 AM_CONDITIONAL(HAVE_BATTERY, test x$enable_battery = xyes)
 AM_CONDITIONAL(HAVE_LIBAPM, test x$enable_linux_apm = xyes)
 AM_CONDITIONAL(HAVE_LIBACPI, test x$enable_linux_acpi = xyes)
+AM_CONDITIONAL(HAVE_SYSFS, test x$enable_linux_sysfs = xyes)
 
 # glib-genmarshal
 GLIB_GENMARSHAL=`$PKG_CONFIG --variable=glib_genmarshal glib-2.0`