fix(network): Proper handling of connectivity testing

README.md

@@ -575,10 +575,7 @@ See [the bspwm module](#user-content-dependencies) for details on `label:dimmed`
   ~~~
 
 
-### 🟊 Module `internal/network`
+### Module `internal/network`
-
-The module is still marked as WIP since the packetloss testing is not
-fully functional yet. If you notice any other anomalies, please [create an issue](https://github.com/jaagr/lemonbuddy/issues).
 
  **NOTE:** If you use both a wired and a wireless network, just add 2 module definitions.
  For example:
@@ -599,12 +596,14 @@ fully functional yet. If you notice any other anomalies, please [create an issue
   interface = wlan1
 
   ; Seconds to sleep between updates
-  interval = 2.0
+  interval = 3.0
 
-  ; Seconds to sleep between connectivity tests
+  ; Test connectivity every Nth update
-  ; A value of 0 disables the testing
+  ; A value of 0 disables the feature
+  ; Recommended minimum value: round(10 / interval)
+  ;   - which would test the connection approx. every 10th sec.
   ; Default: 0
-  ;connectivity_test_interval = 10
+  ;ping_interval = 3
   ~~~
 
 ##### Extra formatting (example)
@@ -618,6 +617,12 @@ fully functional yet. If you notice any other anomalies, please [create an issue
   ;   <label:disconnected> (default)
   ;format:disconnected = <label:disconnected>
 
+  ; Available tags:
+  ;   <label:connected> (default)
+  ;   <label:packetloss>
+  ;   <animation:packetloss>
+  format:packetloss = <animation:packetloss> <label:connected>
+
   ; Available tokens:
   ;   %ifname%    [wireless+wired]
   ;   %local_ip%  [wireless+wired]
@@ -634,12 +639,28 @@ fully functional yet. If you notice any other anomalies, please [create an issue
   ;label:disconnected = not connected
   ;label:disconnected:foreground = #66ffffff
 
+  ; Available tokens:
+  ;   %ifname%    [wireless+wired]
+  ;   %local_ip%  [wireless+wired]
+  ;   %essid%     [wireless]
+  ;   %signal%    [wireless]
+  ;   %linkspeed% [wired]
+  ; Default: (none)
+  ;label:packetloss = %essid%
+  ;label:packetloss:foreground = #eefafafa
+
   ramp:signal:0 = 😱
   ramp:signal:1 = 😠
   ramp:signal:2 = 😒
   ramp:signal:3 = 😊
   ramp:signal:4 = 😃
   ramp:signal:5 = 😈
+
+  animation:packetloss:0 = ⚠
+  animation:packetloss:0:foreground = #ffa64c
+  animation:packetloss:1 = 📶
+  animation:packetloss:1:foreground = #000000
+  animation:packetloss:framerate_ms = 500
   ~~~
 
 

config

@@ -632,12 +632,14 @@ type = internal/network
 interface = net1
 
 ; Seconds to sleep between updates
-interval = 2.0
+interval = 3.0
 
-; Seconds to sleep between connectivity tests
+; Test connectivity every Nth update
-; A value of 0 disables the testing
+; A value of 0 disables the feature
 ; Default: 0
-connectivity_test_interval = 10
+; Recommended minimum value: round(10 / interval)
+;   - which would test the connection approx. every 10th sec.
+ping_interval = 3
 
 ; Available tags:
 ;   <label:connected> (default)
@@ -649,9 +651,10 @@ format:connected = <ramp:signal> <label:connected>
 ;format:disconnected = <label:disconnected>
 
 ; Available tags:
-;   <label:packetloss> (default)
+;   <label:connected> (default)
+;   <label:packetloss>
 ;   <animation:packetloss>
-; format:packetloss = <animation:packetloss> <label:packetloss>
+;format:packetloss = <animation:packetloss> <label:connected>
 
 ; Available tokens:
 ;   %ifname%    [wireless+wired]
@@ -675,8 +678,7 @@ label:disconnected:foreground = #66ffffff
 ;   %essid%     [wireless]
 ;   %signal%    [wireless]
 ;   %linkspeed% [wired]
-; Default: %ifname% %local_ip%
+; Default: (none)
-; ------------------------- NOT ACTIVATED (Needs more testing)
 ;label:packetloss = %essid%
 ;label:packetloss:foreground = #eefafafa
 

include/interfaces/net.hpp

@@ -16,8 +16,9 @@ namespace net
 {
   bool is_wireless_interface(const std::string& ifname);
 
+  //
   // Network
-
+  //
   class NetworkException : public Exception
   {
     public:
@@ -46,9 +47,9 @@ namespace net
       std::string get_ip() throw(NetworkException);
   };
 
-
+  //
   // WiredNetwork
-
+  //
   class WiredNetworkException : public NetworkException {
     using NetworkException::NetworkException;
   };
@@ -63,9 +64,9 @@ namespace net
       std::string get_link_speed();
   };
 
-
+  //
   // WirelessNetwork
-
+  //
   class WirelessNetworkException : public NetworkException {
     using NetworkException::NetworkException;
   };

include/modules/network.hpp

@@ -28,8 +28,6 @@ namespace modules
     const char *TAG_LABEL_PACKETLOSS = "<label:packetloss>";
     const char *TAG_ANIMATION_PACKETLOSS = "<animation:packetloss>";
 
-    const int PING_EVERY_NTH_UPDATE = 10;
-
     std::unique_ptr<net::WiredNetwork> wired_network;
     std::unique_ptr<net::WirelessNetwork> wireless_network;
 
@@ -42,19 +40,20 @@ namespace modules
     std::unique_ptr<drawtypes::Label> label_packetloss_tokenized;
 
     std::string interface;
-    bool connected = false;
-    int signal_quality = 0;
-    bool conseq_packetloss = false;
-    int counter = -1; // -1 to avoid ping the first run
-    int connectivity_test_interval;
 
-    // std::thread t_animation;
+    concurrency::Atomic<bool> connected;
-    // void animation_thread_runner();
+    concurrency::Atomic<bool> conseq_packetloss;
+    concurrency::Atomic<int> signal_quality;
+
+    int ping_nth_update;
+    int counter = -1; // Set to -1 to ignore the first run
+
+    void subthread_routine();
 
     public:
       NetworkModule(const std::string& name);
-      ~NetworkModule();
 
+      void start();
       bool update();
 
       std::string get_format();

src/modules/network.cpp

@@ -14,35 +14,49 @@ using namespace modules;
 
 NetworkModule::NetworkModule(const std::string& name_) : TimerModule(name_, 1s)
 {
-  this->interval = std::chrono::duration<double>(
+  static const auto DEFAULT_FORMAT_CONNECTED = TAG_LABEL_CONNECTED;
-    config::get<float>(name(), "interval", 1));
+  static const auto DEFAULT_FORMAT_DISCONNECTED = TAG_LABEL_DISCONNECTED;
-  this->connectivity_test_interval = config::get<int>(
+  static const auto DEFAULT_FORMAT_PACKETLOSS = TAG_LABEL_CONNECTED;
-    name(), "connectivity_test_interval", 0);
+
-  this->interface = config::get<std::string>(name(), "interface");
+  static const auto DEFAULT_LABEL_CONNECTED = "%ifname% %local_ip%";
+  static const auto DEFAULT_LABEL_DISCONNECTED = "";
+  static const auto DEFAULT_LABEL_PACKETLOSS = "";
+
   this->connected = false;
+  this->conseq_packetloss = false;
 
-  this->formatter->add(FORMAT_CONNECTED, TAG_LABEL_CONNECTED, { TAG_RAMP_SIGNAL, TAG_LABEL_CONNECTED });
+  // Load configuration values
-  this->formatter->add(FORMAT_DISCONNECTED, TAG_LABEL_DISCONNECTED, { TAG_LABEL_DISCONNECTED });
+  this->interface = config::get<std::string>(name(), "interface");
+  this->interval = std::chrono::duration<double>(config::get<float>(name(), "interval", 1));
+  this->ping_nth_update = config::get<int>(name(), "ping_interval", 0);
 
+  // Add formats
+  this->formatter->add(FORMAT_CONNECTED, DEFAULT_FORMAT_CONNECTED, { TAG_RAMP_SIGNAL, TAG_LABEL_CONNECTED });
+  this->formatter->add(FORMAT_DISCONNECTED, DEFAULT_FORMAT_DISCONNECTED, { TAG_LABEL_DISCONNECTED });
+
+  // Create elements for format-connected
   if (this->formatter->has(TAG_RAMP_SIGNAL, FORMAT_CONNECTED))
     this->ramp_signal = drawtypes::get_config_ramp(name(), get_tag_name(TAG_RAMP_SIGNAL));
   if (this->formatter->has(TAG_LABEL_CONNECTED, FORMAT_CONNECTED))
-    this->label_connected = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_CONNECTED), "%ifname% %local_ip%");
+    this->label_connected = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_CONNECTED), DEFAULT_LABEL_CONNECTED);
 
+  // Create elements for format-disconnected
   if (this->formatter->has(TAG_LABEL_DISCONNECTED, FORMAT_DISCONNECTED)) {
-    this->label_disconnected = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_DISCONNECTED), "");
+    this->label_disconnected = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_DISCONNECTED), DEFAULT_LABEL_DISCONNECTED);
     this->label_disconnected->replace_token("%ifname%", this->interface);
   }
 
-  if (this->connectivity_test_interval > 0) {
+  // Create elements for format-packetloss if we are told to test connectivity
-    this->formatter->add(FORMAT_PACKETLOSS, "", { TAG_ANIMATION_PACKETLOSS, TAG_LABEL_PACKETLOSS });
+  if (this->ping_nth_update > 0) {
+    this->formatter->add(FORMAT_PACKETLOSS, DEFAULT_FORMAT_PACKETLOSS, { TAG_ANIMATION_PACKETLOSS, TAG_LABEL_PACKETLOSS, TAG_LABEL_CONNECTED });
 
     if (this->formatter->has(TAG_LABEL_PACKETLOSS, FORMAT_PACKETLOSS))
-      this->label_packetloss = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_PACKETLOSS), "%ifname% %local_ip%");
+      this->label_packetloss = drawtypes::get_optional_config_label(name(), get_tag_name(TAG_LABEL_PACKETLOSS), DEFAULT_LABEL_PACKETLOSS);
     if (this->formatter->has(TAG_ANIMATION_PACKETLOSS, FORMAT_PACKETLOSS))
       this->animation_packetloss = drawtypes::get_config_animation(name(), get_tag_name(TAG_ANIMATION_PACKETLOSS));
   }
 
+  // Get an intstance of the network interface
   try {
     if (net::is_wireless_interface(this->interface)) {
       this->wireless_network = std::make_unique<net::WirelessNetwork>(this->interface);
@@ -54,54 +68,46 @@ NetworkModule::NetworkModule(const std::string& name_) : TimerModule(name_, 1s)
   }
 }
 
-NetworkModule::~NetworkModule()
+void NetworkModule::start()
 {
-  std::lock_guard<concurrency::SpinLock> lck(this->update_lock);
+  this->TimerModule::start();
-  // if (this->t_animation.joinable())
+
-  //   this->t_animation.join();
+  // We only need to start the subthread if the packetloss animation is used
+  if (this->animation_packetloss)
+    this->threads.emplace_back(std::thread(&NetworkModule::subthread_routine, this));
 }
 
-// void NetworkModule::dispatch()
+void NetworkModule::subthread_routine()
-// {
+{
-//   this->EventModule::dispatch();
+  std::this_thread::yield();
-//
-//   // if (this->animation_packetloss)
-//   //   this->t_animation = std::thread(&NetworkModule::animation_thread_runner, this);
-// }
 
-// bool NetworkModule::has_event()
+  const auto dur = std::chrono::duration<double>(
-// {
+    float(this->animation_packetloss->get_framerate()) / 1000.0f);
-//   std::this_thread::sleep_for(this->interval);
-//   return true;
-// }
 
-// void NetworkModule::animation_thread_runner()
+  while (this->enabled()) {
-// {
+    std::unique_lock<concurrency::SpinLock> lck(this->broadcast_lock);
-//   while (this->enabled()) {
+
-//     std::unique_lock<std::mutex> lck(this->mtx);
+    if (this->connected && this->conseq_packetloss) {
-//
+      lck.unlock();
-//     if (this->connected && this->conseq_packetloss)
+      this->broadcast();
-//       this->Module::notify_change();
+    }
-//     else
+
-//       this->cv.wait(lck, [&]{
+    std::this_thread::sleep_for(dur);
-//         return this->connected && this->conseq_packetloss; });
+  }
-//
+
-//     std::this_thread::sleep_for(std::chrono::duration<double>(
+  log_debug("Reached end of network subthread");
-//       float(this->animation_packetloss->get_framerate()) / 1000));
+}
-//   }
-// }
 
 bool NetworkModule::update()
 {
   std::string ip, essid, linkspeed;
   int signal_quality = 0;
 
+  net::Network *network = nullptr;
+
+  // Process data for wireless network interfaces
   if (this->wireless_network) {
-    try {
+    network = this->wireless_network.get();
-      ip = this->wireless_network->get_ip();
-    } catch (net::NetworkException &e) {
-      get_logger()->debug(e.what());
-    }
 
     try {
       essid = this->wireless_network->get_essid();
@@ -110,26 +116,33 @@ bool NetworkModule::update()
       get_logger()->debug(e.what());
     }
 
-    this->connected = this->wireless_network->connected();
     this->signal_quality = signal_quality;
 
-    if (this->connectivity_test_interval > 0 && this->connected && this->counter++ % PING_EVERY_NTH_UPDATE == 0)
+  // Process data for wired network interfaces
-      this->conseq_packetloss = !this->wireless_network->test();
   } else if (this->wired_network) {
+    network = this->wired_network.get();
+    linkspeed = this->wired_network->get_link_speed();
+  }
+
+  if (network != nullptr) {
     try {
-      ip = this->wired_network->get_ip();
+      ip = network->get_ip();
     } catch (net::NetworkException &e) {
       get_logger()->debug(e.what());
     }
 
-    linkspeed = this->wired_network->get_link_speed();
+    this->connected = network->connected();
 
-    this->connected = this->wired_network->connected();
+    // Ignore the first run
-
+    if (this->counter == -1) {
-    if (this->connectivity_test_interval > 0 && this->connected && this->counter++ % PING_EVERY_NTH_UPDATE == 0)
+      this->counter = 0;
-      this->conseq_packetloss = !this->wired_network->test();
+    } else if (this->ping_nth_update > 0 && this->connected && (++this->counter % this->ping_nth_update) == 0) {
+      this->conseq_packetloss = !network->test();
+      this->counter = 0;
+    }
   }
 
+  // Update label contents
   if (this->label_connected || this->label_packetloss) {
     auto replace_tokens = [&](std::unique_ptr<drawtypes::Label> &label){
       label->replace_token("%ifname%", this->interface);
@@ -161,8 +174,6 @@ bool NetworkModule::update()
     }
   }
 
-  // this->cv.notify_all();
-
   return true;
 }
 
@@ -170,7 +181,7 @@ std::string NetworkModule::get_format()
 {
   if (!this->connected)
     return FORMAT_DISCONNECTED;
-  else if (this->conseq_packetloss)
+  else if (this->conseq_packetloss && this->ping_nth_update > 0)
     return FORMAT_PACKETLOSS;
   else
     return FORMAT_CONNECTED;