Fix all 20 code review issues

gui/config_dialog.py

@@ -233,6 +233,15 @@ def create_rtlsdr_tab(self):
         defaults_layout.addWidget(QLabel("Gain:"), 2, 0)
         self.sdr_gain = QComboBox()
         self.sdr_gain.addItems(['Auto', '10 dB', '20 dB', '30 dB', '40 dB'])
+        # Set gain combo to configured value
+        gain = self.config.get('rtlsdr', {}).get('gain', 'auto')
+        if gain == 'auto':
+            self.sdr_gain.setCurrentIndex(0)
+        else:
+            gain_text = f"{gain} dB"
+            idx = self.sdr_gain.findText(gain_text)
+            if idx >= 0:
+                self.sdr_gain.setCurrentIndex(idx)
         defaults_layout.addWidget(self.sdr_gain, 2, 1)
 
         defaults_group.setLayout(defaults_layout)
@@ -349,8 +358,15 @@ def get_config(self):
         config['rtc']['type'] = self.rtc_type.currentText()
         config['rtc']['i2c_bus'] = self.rtc_i2c_bus.value()
         try:
-            config['rtc']['i2c_address'] = int(self.rtc_i2c_addr.text(), 16)
+            addr_text = self.rtc_i2c_addr.text().strip()
+            # Support both 0x prefix and plain hex
+            if addr_text.startswith('0x') or addr_text.startswith('0X'):
+                config['rtc']['i2c_address'] = int(addr_text, 16)
+            else:
+                config['rtc']['i2c_address'] = int(addr_text, 16)
         except ValueError:
+            # Log warning and use default
+            logger.warning(f"Invalid I2C address '{self.rtc_i2c_addr.text()}', using default 0x51")
             config['rtc']['i2c_address'] = 0x51
 
         # Update USB settings

gui/gps_panel.py

@@ -22,17 +22,20 @@
 class GPSPanel(QWidget):
     """GPS module control panel with gpsd support"""
 
-    # Signal for thread-safe UI updates
+    # Signals for thread-safe UI updates
     data_updated = pyqtSignal(dict)
+    log_message_signal = pyqtSignal(str)  # Thread-safe log updates
 
     def __init__(self, config):
         super().__init__()
         self.config = config
         self.gpsd_socket = None
         self.gpsd_thread = None
+        self.serial_port = None  # Initialize serial_port attribute
         self.running = False
         self.is_logging = False
         self.log_file = None
+        self._config_enabled = config.get('enabled', False)  # Track configured state
 
         # GPS data
         self.latitude = 0.0
@@ -49,8 +52,9 @@ def __init__(self, config):
 
         self.init_ui()
 
-        # Connect signal for thread-safe updates
+        # Connect signals for thread-safe updates
         self.data_updated.connect(self._update_ui_from_data)
+        self.log_message_signal.connect(self._do_append_log)
 
     def init_ui(self):
         """Initialize user interface"""
@@ -161,9 +165,8 @@ def toggle_connection(self):
 
     def connect_gps(self):
         """Connect to GPS via gpsd or serial"""
-        mode = self.config.get('mode', 'gpsd')
-
-        if mode == 'gpsd' or self.mode_combo.currentIndex() == 0:
+        # Use combo box as single source of truth for mode
+        if self.mode_combo.currentIndex() == 0:
             self._connect_gpsd()
         else:
             self._connect_serial()
@@ -373,10 +376,12 @@ def _update_ui_from_data(self, data):
 
         if 'time_utc' in data:
             self.time_utc = data['time_utc']
-            # Format time nicely
+            # Format time nicely - extract time part after 'T'
             if self.time_utc and 'T' in self.time_utc:
-                time_part = self.time_utc.split('T')[1].split('.')[0] if 'T' in self.time_utc else self.time_utc
+                time_part = self.time_utc.split('T')[1].split('.')[0]
                 self.time_label.setText(f"UTC Time:  {time_part}")
+            elif self.time_utc:
+                self.time_label.setText(f"UTC Time:  {self.time_utc}")
 
         if 'hdop' in data:
             self.hdop = data['hdop']
@@ -388,9 +393,12 @@ def _update_ui_from_data(self, data):
 
         if 'satellites' in data:
             self.satellites = data['satellites']
+            # Update display when either value changes
+            self.sat_count_label.setText(f"In View: {self.satellites} | Used: {self.satellites_used}")
 
         if 'satellites_used' in data:
             self.satellites_used = data['satellites_used']
+            # Update display when either value changes
             self.sat_count_label.setText(f"In View: {self.satellites} | Used: {self.satellites_used}")
 
         if 'satellite_list' in data:
@@ -430,6 +438,19 @@ def _update_satellite_table(self, satellites):
 
     def _append_log(self, text):
         """Append text to raw log (thread-safe via signal)"""
+        # Emit signal for thread-safe UI update
+        self.log_message_signal.emit(text)
+
+        # Write to file if logging (file I/O is safe from background thread)
+        if self.is_logging and self.log_file:
+            try:
+                self.log_file.write(f"{datetime.now().isoformat()} {text}\n")
+                self.log_file.flush()
+            except Exception as e:
+                logger.error(f"Log write error: {e}")
+
+    def _do_append_log(self, text):
+        """Actually append text to raw log (called from main thread)"""
         # Limit log size
         if self.raw_log.document().lineCount() > 100:
             cursor = self.raw_log.textCursor()
@@ -439,14 +460,6 @@ def _append_log(self, text):
 
         self.raw_log.append(text)
 
-        # Write to file if logging
-        if self.is_logging and self.log_file:
-            try:
-                self.log_file.write(f"{datetime.now().isoformat()} {text}\n")
-                self.log_file.flush()
-            except Exception as e:
-                logger.error(f"Log write error: {e}")
-
     def disconnect_gps(self):
         """Disconnect from GPS"""
         self.running = False

gui/lora_panel.py

@@ -34,6 +34,7 @@ def __init__(self, config):
         self.meshtastic_session = None
         self.polling_thread = None
         self.running = False
+        self._config_enabled = config.get('enabled', False)  # Track configured state
 
         self.last_rssi = None
         self.last_snr = None
@@ -280,20 +281,34 @@ def _poll_meshtastic(self):
         """Poll Meshtastic for new messages"""
         host = self.config.get('meshtastic_host', 'localhost')
         port = self.config.get('meshtastic_port', 443)
+        consecutive_errors = 0
+        max_consecutive_errors = 5
+
+        while self.running:
+            # Check if session is still valid
+            if not self.meshtastic_session:
+                logger.warning("Meshtastic session lost, stopping polling")
+                break
 
-        while self.running and self.meshtastic_session:
             try:
                 url = f"https://{host}:{port}/api/v1/fromradio?all=false"
                 response = self.meshtastic_session.get(url, timeout=10)
 
                 if response.status_code == 200 and response.content:
                     # Process received data
                     # In a full implementation, this would decode protobuf messages
-                    pass
+                    consecutive_errors = 0  # Reset on success
 
             except Exception as e:
+                consecutive_errors += 1
                 if self.running:
-                    logger.debug(f"Meshtastic poll: {e}")
+                    logger.debug(f"Meshtastic poll error ({consecutive_errors}/{max_consecutive_errors}): {e}")
+
+                # Stop polling after too many consecutive errors
+                if consecutive_errors >= max_consecutive_errors:
+                    logger.error("Too many consecutive polling errors, stopping")
+                    self.status_changed.emit("Connection lost", "red")
+                    break
 
             time.sleep(1)
 
@@ -392,8 +407,7 @@ def send_message(self):
             else:
                 self._send_direct(message)
 
-            timestamp = datetime.now().strftime("%H:%M:%S")
-            self.log_message(f"[{timestamp}] TX: {message}")
+            self.log_message(f"TX: {message}")
             self.tx_input.clear()
 
         except Exception as e:
@@ -455,9 +469,9 @@ def get_last_rssi(self):
         return self.last_rssi
 
     def get_config(self):
-        """Get current configuration"""
+        """Get current configuration (returns configured state, not runtime state)"""
         return {
-            'enabled': self.lora_active,
+            'enabled': self._config_enabled,  # Return configured state, not runtime
             'mode': 'meshtastic' if self.mode_combo.currentIndex() == 0 else 'direct',
             'meshtastic_host': self.config.get('meshtastic_host', 'localhost'),
             'meshtastic_port': self.config.get('meshtastic_port', 443),
@@ -474,6 +488,7 @@ def get_config(self):
     def apply_config(self, config):
         """Apply configuration"""
         self.config = config
+        self._config_enabled = config.get('enabled', False)  # Track configured state
 
         mode = config.get('mode', 'meshtastic')
         self.mode_combo.setCurrentIndex(0 if mode == 'meshtastic' else 1)

gui/rtc_panel.py

@@ -5,7 +5,7 @@
 """
 
 import logging
-import os
+import subprocess
 from datetime import datetime
 
 from PyQt5.QtWidgets import (QWidget, QVBoxLayout, QHBoxLayout, QGroupBox,
@@ -179,9 +179,9 @@ def connect_rtc(self):
     def _connect_system_rtc(self):
         """Use system RTC (hwclock)"""
         try:
-            # Check if we can access system RTC
-            result = os.popen('hwclock -r 2>/dev/null').read()
-            if result:
+            # Check if we can access system RTC using subprocess
+            result = subprocess.run(['hwclock', '-r'], capture_output=True, text=True, timeout=5)
+            if result.returncode == 0 and result.stdout:
                 self.rtc_active = True
                 self.rtc_type = 'system'
                 self.connect_btn.setText("Disconnect RTC")
@@ -193,6 +193,10 @@ def _connect_system_rtc(self):
                 logger.info("Connected to system RTC")
             else:
                 raise Exception("hwclock not accessible")
+        except subprocess.TimeoutExpired:
+            self.status_label.setText("System RTC: timeout")
+            self.status_label.setStyleSheet("color: red;")
+            logger.error("System RTC connection timed out")
         except Exception as e:
             self.status_label.setText(f"System RTC failed: {str(e)}")
             self.status_label.setStyleSheet("color: red;")
@@ -365,8 +369,9 @@ def _read_ds3231(self):
         months = bcd_to_dec(data[5] & 0x1F)
         years = bcd_to_dec(data[6]) + 2000
 
-        weekday_names = ['', 'Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']
-        weekday_name = weekday_names[weekday] if weekday < 8 else 'Unknown'
+        # DS3231 uses 1-7 for weekday (1=Sunday, 7=Saturday)
+        weekday_names = ['Unknown', 'Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']
+        weekday_name = weekday_names[weekday] if 1 <= weekday <= 7 else 'Unknown'
 
         self.time_label.setText(f"{hours:02d}:{minutes:02d}:{seconds:02d}")
         self.date_label.setText(f"{years}-{months:02d}-{days:02d} ({weekday_name})")
@@ -396,9 +401,16 @@ def set_rtc_time(self):
             logger.error(f"Failed to set RTC time: {e}")
 
     def _set_system_rtc(self, date, time):
-        """Set system RTC time"""
+        """Set system RTC time (safe from command injection)"""
+        # Format datetime string safely - no user input reaches the shell
         datetime_str = f"{date.year()}-{date.month():02d}-{date.day():02d} {time.hour():02d}:{time.minute():02d}:{time.second():02d}"
-        os.system(f'sudo hwclock --set --date="{datetime_str}"')
+        # Use subprocess with list args to avoid shell injection
+        result = subprocess.run(
+            ['sudo', 'hwclock', '--set', f'--date={datetime_str}'],
+            capture_output=True, text=True, timeout=10
+        )
+        if result.returncode != 0:
+            raise Exception(f"hwclock failed: {result.stderr}")
 
     def _set_pcf85063a(self, date, time):
         """Set PCF85063A RTC time"""
@@ -466,10 +478,19 @@ def sync_to_system(self):
                 QMessageBox.information(self, "Info", "Already using system RTC.")
                 return
 
-            # Read RTC time and set system clock
-            os.system('sudo hwclock --hctosys')
-            QMessageBox.information(self, "Success", "System time synchronized from RTC!")
-            logger.info("System time synchronized from RTC")
+            # Read RTC time and set system clock using subprocess
+            result = subprocess.run(
+                ['sudo', 'hwclock', '--hctosys'],
+                capture_output=True, text=True, timeout=10
+            )
+            if result.returncode == 0:
+                QMessageBox.information(self, "Success", "System time synchronized from RTC!")
+                logger.info("System time synchronized from RTC")
+            else:
+                raise Exception(result.stderr)
+        except subprocess.TimeoutExpired:
+            QMessageBox.critical(self, "Error", "Sync operation timed out")
+            logger.error("Sync to system timed out")
         except Exception as e:
             QMessageBox.critical(self, "Error", f"Failed to sync:\n{str(e)}")
             logger.error(f"Failed to sync system time from RTC: {e}")
@@ -506,4 +527,7 @@ def rescan(self):
 
     def cleanup(self):
         """Cleanup resources"""
+        # Stop timer first to prevent callbacks on destroyed widgets
+        if hasattr(self, 'update_timer') and self.update_timer:
+            self.update_timer.stop()
         self.disconnect_rtc()