Merge pull request #923 from pedrocr/fix-battery-calculations

Simplify and improve battery state calculations

include/modules/battery.hpp

@@ -31,16 +31,16 @@ class Battery : public ALabel {
  private:
   static inline const fs::path data_dir_ = "/sys/class/power_supply/";
 
-  void                                          getBatteries();
+  void                                          refreshBatteries();
   void                                          worker();
   const std::string                             getAdapterStatus(uint8_t capacity) const;
   const std::tuple<uint8_t, float, std::string> getInfos() const;
   const std::string                             formatTimeRemaining(float hoursRemaining);
 
-  std::vector<fs::path> batteries_;
+  int                   global_watch;
+  std::map<fs::path,int> batteries_;
   fs::path              adapter_;
   int                   fd_;
-  std::vector<int>      wds_;
   std::string           old_status_;
 
   util::SleeperThread   thread_;

man/waybar-battery.5.scd

@@ -20,7 +20,7 @@ The *battery* module displays the current capacity and state (eg. charging) of y
 
 *full-at*: ++
 	typeof: integer ++
-	Define the max percentage of the battery, useful for an old battery, e.g. 96
+	Define the max percentage of the battery, for when you've set the battery to stop charging at a lower level to save it. For example, if you've set the battery to stop at 80% that will become the new 100%.
 
 *interval*: ++
 	typeof: integer ++

src/modules/battery.cpp

@@ -4,23 +4,31 @@
 
 waybar::modules::Battery::Battery(const std::string& id, const Json::Value& config)
     : ALabel(config, "battery", id, "{capacity}%", 60) {
-  getBatteries();
   fd_ = inotify_init1(IN_CLOEXEC);
   if (fd_ == -1) {
     throw std::runtime_error("Unable to listen batteries.");
   }
-  for (auto const& bat : batteries_) {
+
-    auto wd = inotify_add_watch(fd_, (bat / "uevent").c_str(), IN_ACCESS);
+  // Watch the directory for any added or removed batteries
-    if (wd != -1) {
+  global_watch = inotify_add_watch(fd_, data_dir_.c_str(), IN_CREATE | IN_DELETE);
-      wds_.push_back(wd);
+  if (global_watch < 0) {
-    }
+    throw std::runtime_error("Could not watch for battery plug/unplug");
   }
+
+  refreshBatteries();
   worker();
 }
 
 waybar::modules::Battery::~Battery() {
-  for (auto wd : wds_) {
+  if (global_watch >= 0) {
-    inotify_rm_watch(fd_, wd);
+    inotify_rm_watch(fd_, global_watch);
+  }
+  for (auto it = batteries_.cbegin(); it != batteries_.cend(); it++) {
+    auto watch_id = (*it).second;
+    if (watch_id >= 0) {
+      inotify_rm_watch(fd_, watch_id);
+    }
+    batteries_.erase(it);
   }
   close(fd_);
 }
@@ -43,7 +51,13 @@ void waybar::modules::Battery::worker() {
   };
 }
 
-void waybar::modules::Battery::getBatteries() {
+void waybar::modules::Battery::refreshBatteries() {
+  // Mark existing list of batteries as not necessarily found
+  std::map<fs::path, bool> check_map;
+  for (auto const& bat : batteries_) {
+    check_map[bat.first] = false;
+  }
+
   try {
     for (auto& node : fs::directory_iterator(data_dir_)) {
       if (!fs::is_directory(node)) {
@@ -54,12 +68,22 @@ void waybar::modules::Battery::getBatteries() {
       if (((bat_defined && dir_name == config_["bat"].asString()) || !bat_defined) &&
           fs::exists(node.path() / "capacity") && fs::exists(node.path() / "uevent") &&
           fs::exists(node.path() / "status") && fs::exists(node.path() / "type")) {
-            std::string type;
+        std::string type;
-            std::ifstream(node.path() / "type") >> type;
+        std::ifstream(node.path() / "type") >> type;
 
-            if (!type.compare("Battery")){
+        if (!type.compare("Battery")){
-              batteries_.push_back(node.path());
+          check_map[node.path()] = true;
+          auto search = batteries_.find(node.path());
+          if (search == batteries_.end()) {
+            // We've found a new battery save it and start listening for events
+            auto event_path = (node.path() / "uevent");
+            auto wd = inotify_add_watch(fd_, event_path.c_str(), IN_ACCESS);
+            if (wd < 0) {
+              throw std::runtime_error("Could not watch events for " + node.path().string());
             }
+            batteries_[node.path()] = wd;
+          }
+        }
       }
       auto adap_defined = config_["adapter"].isString();
       if (((adap_defined && dir_name == config_["adapter"].asString()) || !adap_defined) &&
@@ -76,22 +100,31 @@ void waybar::modules::Battery::getBatteries() {
     }
     throw std::runtime_error("No batteries.");
   }
+
+  // Remove any batteries that are no longer present and unwatch them
+  for (auto const& check : check_map) {
+    if (!check.second) {
+      auto watch_id = batteries_[check.first];
+      if (watch_id >= 0) {
+        inotify_rm_watch(fd_, watch_id);
+      }
+      batteries_.erase(check.first);
+    }
+  }
 }
 
 const std::tuple<uint8_t, float, std::string> waybar::modules::Battery::getInfos() const {
   try {
-    uint16_t    total = 0;
     uint32_t    total_power = 0;   // μW
     uint32_t    total_energy = 0;  // μWh
     uint32_t    total_energy_full = 0;
     std::string status = "Unknown";
-    for (auto const& bat : batteries_) {
+    for (auto const& item : batteries_) {
-      uint16_t    capacity;
+      auto bat = item.first;
       uint32_t    power_now;
       uint32_t    energy_full;
       uint32_t    energy_now;
       std::string _status;
-      std::ifstream(bat / "capacity") >> capacity;
       std::ifstream(bat / "status") >> _status;
       auto rate_path = fs::exists(bat / "current_now") ? "current_now" : "power_now";
       std::ifstream(bat / rate_path) >> power_now;
@@ -102,7 +135,6 @@ const std::tuple<uint8_t, float, std::string> waybar::modules::Battery::getInfos
       if (_status != "Unknown") {
         status = _status;
       }
-      total += capacity;
       total_power += power_now;
       total_energy += energy_now;
       total_energy_full += energy_full;
@@ -119,19 +151,33 @@ const std::tuple<uint8_t, float, std::string> waybar::modules::Battery::getInfos
       time_remaining = (float)total_energy / total_power;
     } else if (status == "Charging" && total_power != 0) {
       time_remaining = -(float)(total_energy_full - total_energy) / total_power;
+      if (time_remaining > 0.0f) {
+        // If we've turned positive it means the battery is past 100% and so
+        // just report that as no time remaining
+        time_remaining = 0.0f;
+      }
     }
-    uint16_t capacity = total / batteries_.size();
+    float capacity = ((float)total_energy * 100.0f / (float) total_energy_full);
     // Handle full-at
     if (config_["full-at"].isUInt()) {
       auto full_at = config_["full-at"].asUInt();
       if (full_at < 100) {
         capacity = 100.f * capacity / full_at;
-        if (capacity > full_at) {
-          capacity = full_at;
-        }
       }
     }
-    return {capacity, time_remaining, status};
+    if (capacity > 100.f) {
+      // This can happen when the battery is calibrating and goes above 100%
+      // Handle it gracefully by clamping at 100%
+      capacity = 100.f;
+    }
+    uint8_t cap = round(capacity);
+    if (cap == 100) {
+      // If we've reached 100% just mark as full as some batteries can stay
+      // stuck reporting they're still charging but not yet done
+      status = "Full";
+    }
+
+    return {cap, time_remaining, status};
   } catch (const std::exception& e) {
     spdlog::error("Battery: {}", e.what());
     return {0, 0, "Unknown"};
@@ -158,6 +204,10 @@ const std::string waybar::modules::Battery::formatTimeRemaining(float hoursRemai
   uint16_t full_hours = static_cast<uint16_t>(hoursRemaining);
   uint16_t minutes = static_cast<uint16_t>(60 * (hoursRemaining - full_hours));
   auto     format = std::string("{H} h {M} min");
+  if (full_hours == 0 && minutes == 0) {
+    // Migh as well not show "0h 0min"
+    return "";
+  }
   if (config_["format-time"].isString()) {
     format = config_["format-time"].asString();
   }
@@ -165,6 +215,11 @@ const std::string waybar::modules::Battery::formatTimeRemaining(float hoursRemai
 }
 
 auto waybar::modules::Battery::update() -> void {
+  // Make sure we have the correct set of batteries, in case of hotplug
+  // TODO: split the global watch into it's own event and only run the refresh
+  //       when there's been a CREATE/DELETE event
+  refreshBatteries();
+
   auto [capacity, time_remaining, status] = getInfos();
   if (status == "Unknown") {
     status = getAdapterStatus(capacity);