Skip to content

Fix Modbus connection recovery for both master and slave plugins #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thiagoralves merged 2 commits into from
Dec 7, 2025
Merged

Fix Modbus connection recovery for both master and slave plugins #42

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbea42c
Fix Modbus master connection recovery after network interruption
devin-ai-integration[bot] Dec 6, 2025
5a75438
Improve Modbus slave plugin error recovery and reduce logging
devin-ai-integration[bot] Dec 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions core/src/drivers/plugins/python/modbus_master/modbus_master_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

import time
from typing import Optional

from pymodbus.client import ModbusTcpClient
from pymodbus.exceptions import ConnectionException


Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Unnecessary blank line added after imports. The import section already has proper spacing.

Suggested change

Copilot uses AI. Check for mistakes.
class ModbusConnectionManager:
class ModbusConnectionManager: # pylint: disable=too-many-instance-attributes

Copilot AI Dec 7, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The pylint disable comment uses a symbolic name (too-many-instance-attributes) which is good practice. However, this conflicts with the approach in the modbus_slave file which uses numeric codes. Consider standardizing on symbolic names across both files for consistency and better maintainability.

Copilot uses AI. Check for mistakes.
"""Manages Modbus TCP connections with retry logic."""

def __init__(self, host: str, port: int, timeout_ms: int):
Expand All @@ -15,11 +15,12 @@ def __init__(self, host: str, port: int, timeout_ms: int):
self.timeout = timeout_ms / 1000.0 # Convert to seconds

# Retry configuration
self.retry_delay_base = 2.0 # initial delay between attempts (seconds)
self.retry_delay_max = 30.0 # maximum delay between attempts (seconds)
self.retry_delay_base = 2.0 # initial delay between attempts (seconds)
self.retry_delay_max = 30.0 # maximum delay between attempts (seconds)
self.retry_delay_current = self.retry_delay_base

# Connection state
# Connection state - is_connected is the authoritative flag for connection health
# It is set to False when any error occurs, forcing reconnection on next cycle
self.client: Optional[ModbusTcpClient] = None
self.is_connected = False

Expand All @@ -42,17 +43,17 @@ def connect_with_retry(self, stop_event=None) -> bool:
if self.client:
try:
self.client.close()
except:
except Exception:
pass
self.client = ModbusTcpClient(
host=self.host,
port=self.port,
timeout=self.timeout
host=self.host, port=self.port, timeout=self.timeout

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The formatting change from multi-line to single-line parameters reduces readability. The previous multi-line format was clearer and more consistent with Python style guidelines.

Suggested change
host=self.host, port=self.port, timeout=self.timeout
host=self.host,
port=self.port,
timeout=self.timeout

Copilot uses AI. Check for mistakes.
)

# Attempt to connect
if self.client.connect():
print(f"(PASS) Connected to {self.host}:{self.port} (attempt {retry_count + 1})")
print(
f"(PASS) Connected to {self.host}:{self.port} (attempt {retry_count + 1})"
)
self.is_connected = True
self.retry_delay_current = self.retry_delay_base # Reset delay
return True
Expand Down Expand Up @@ -88,22 +89,49 @@ def ensure_connection(self, stop_event=None) -> bool:
"""
Ensures there is a valid connection, reconnecting if necessary.

This method checks BOTH the pymodbus client state AND our is_connected flag.
The is_connected flag is set to False when any communication error occurs,
which forces a full reconnection even if pymodbus thinks the socket is still open.

Args:
stop_event: Optional threading.Event to allow early termination

Returns:
True if connection is available, False if interrupted
"""
# Check if already connected
if self.client and self.client.connected:
# Check if already connected - must check BOTH conditions:
# 1. is_connected flag (set to False on any error)
# 2. client.connected (pymodbus internal socket state)
if self.is_connected and self.client and self.client.connected:
return True

# Mark as disconnected
# Connection is broken or marked as unhealthy - force full reconnection
# First, close any existing client to clean up dead sockets
if self.client:
try:
self.client.close()
except Exception:
pass # Ignore errors during cleanup
self.client = None

self.is_connected = False

# Try to reconnect
# Try to reconnect with infinite retry
return self.connect_with_retry(stop_event)

def mark_disconnected(self):
"""
Mark the connection as broken, forcing reconnection on next ensure_connection() call.

This should be called when any communication error occurs (timeout, broken pipe,
ModbusIOException, etc.) to ensure the connection is properly re-established.
"""
self.is_connected = False
print(
f"Connection to {self.host}:{self.port} marked as disconnected, "
"will reconnect on next cycle"
)

def disconnect(self):
"""Close the connection and clean up resources."""
try:
Expand Down
20 changes: 10 additions & 10 deletions core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,15 @@ def run(self): # pylint: disable=too-many-locals
f"(FC {point.fc}, addr {address}): {response}"
)
# Mark as disconnected to force reconnection on next cycle
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()
continue
if response.isError():
print(
f"[{self.name}] (FAIL) Modbus read failed "
f"(FC {point.fc}, addr {address}): {response}"
)
# Mark as disconnected to force reconnection on next cycle
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()
continue

# Extract data from response
Expand All @@ -205,14 +205,14 @@ def run(self): # pylint: disable=too-many-locals
f"FC {point.fc}, offset {point.offset}: {ce}"
)
# Mark as disconnected to force reconnection
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()
except Exception as e:
print(
f"[{self.name}] (FAIL) Error reading "
f"FC {point.fc}, offset {point.offset}: {e}"
)
# For other errors also mark disconnected as precaution
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()

# Batch update IEC buffers with single mutex acquisition
if read_results_to_update:
Expand Down Expand Up @@ -316,14 +316,14 @@ def run(self): # pylint: disable=too-many-locals
f"(FC {point.fc}, addr {address}): {response}"
)
# Mark as disconnected to force reconnection on next cycle
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()
elif response.isError():
print(
f"[{self.name}] (FAIL) Modbus write failed "
f"(FC {point.fc}, addr {address}): {response}"
)
# Mark as disconnected to force reconnection on next cycle
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()

except ValueError as ve:
print(
Expand All @@ -336,14 +336,14 @@ def run(self): # pylint: disable=too-many-locals
f"FC {point.fc}, offset {point.offset}: {ce}"
)
# Mark as disconnected to force reconnection
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()
except Exception as e:
print(
f"[{self.name}] (FAIL) Error writing "
f"FC {point.fc}, offset {point.offset}: {e}"
)
# For other errors also mark disconnected as precaution
self.connection_manager.is_connected = False
self.connection_manager.mark_disconnected()

# 3. CYCLE TIMING - Sleep for GCD cycle time
cycle_elapsed = time.monotonic() - cycle_start_time
Expand All @@ -363,8 +363,8 @@ def run(self): # pylint: disable=too-many-locals

except ConnectionException as ce:
print(f"[{self.name}] (FAIL) Connection failed: {ce}")
# Try to reconnect
self.connection_manager.is_connected = False
# Mark as disconnected to force reconnection
self.connection_manager.mark_disconnected()
except Exception as e:
print(f"[{self.name}] (FAIL) Unexpected error in thread: {e}")
traceback.print_exc()
Expand Down