Skip to content

Commit c5d1766

Browse files
committed
fix(connection-selection): cache history, fix symlink persistence, hermetic tests
Address three unresolved code-review issues from PR #1374: - Cache ProgramSettings.get_connection_history() once at widget init as _connection_history_cache instead of calling it (a disk read) on every 3-second periodic port-refresh cycle, preventing potential UI stalls on slow disks or remote home directories. - Update _connection_history_cache in-memory (prepend + deduplicate) in both reconnect() and add_connection() after calling store_connection(), keeping the cache consistent without further disk reads. - Fix reconnect() to persist `selected_connection or device` instead of always persisting self.flight_controller.comport.device. This avoids storing resolved, unstable device paths (e.g. /dev/ttyACM0) when the user explicitly selected a /dev/serial/by-id/* symlink, preventing duplicates in the connection history. - Patch ProgramSettings.store_connection in test_periodic_refresh_stops_on_connection to prevent writes to the real settings.json during tests, and assert the correct connection string was persisted to verify the fix above.
1 parent abd1d29 commit c5d1766

File tree

2 files changed

+26
-6
lines changed

2 files changed

+26
-6
lines changed

ardupilot_methodic_configurator/frontend_tkinter_connection_selection.py

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,10 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen
7777
# Create a label for port
7878
port_label = ttk.Label(selection_frame, text=_("Port:"))
7979
port_label.pack(side=tk.LEFT, padx=(0, 5))
80-
# Load saved connection history from ProgramSettings
81-
for conn in ProgramSettings.get_connection_history():
80+
# Load saved connection history from ProgramSettings and cache it to avoid
81+
# repeated disk reads on every periodic port-refresh cycle.
82+
self._connection_history_cache: list[str] = ProgramSettings.get_connection_history()
83+
for conn in self._connection_history_cache:
8284
self.flight_controller.add_connection(conn)
8385

8486
# Create a read-only combobox for flight controller connection selection
@@ -136,7 +138,7 @@ def _refresh_ports(self) -> None:
136138
current_selection = self.conn_selection_combobox.get_selected_key()
137139
self.flight_controller.discover_connections(
138140
progress_callback=None,
139-
preserved_connections=ProgramSettings.get_connection_history(),
141+
preserved_connections=self._connection_history_cache,
140142
)
141143
new_connection_tuples = self.flight_controller.get_connection_tuples()
142144

@@ -211,6 +213,10 @@ def add_connection(self) -> str:
211213
)
212214
if selected_connection:
213215
ProgramSettings.store_connection(selected_connection)
216+
# Update cache in-memory (most-recent-first, deduplicated) to avoid a disk re-read.
217+
self._connection_history_cache = [selected_connection] + [
218+
c for c in self._connection_history_cache if c != selected_connection
219+
]
214220
error_msg = _("Will add new connection: {selected_connection} if not duplicated")
215221
logging_debug(error_msg.format(**locals()))
216222
self.flight_controller.add_connection(selected_connection)
@@ -261,11 +267,20 @@ def reconnect(self, selected_connection: str = "") -> bool: # Default is auto-c
261267
# Store the current connection as the previous selection
262268
if self.flight_controller.comport and hasattr(self.flight_controller.comport, "device"):
263269
device: str = self.flight_controller.comport.device
264-
self.previous_selection = device
270+
# Prefer the user-selected connection string when available to avoid persisting
271+
# a possibly resolved and unstable device path (e.g. /dev/ttyACM0 instead of
272+
# the original /dev/serial/by-id/* symlink). For auto-connect (empty
273+
# selected_connection), fall back to the detected device path.
274+
connection_identifier: str = selected_connection or device
275+
self.previous_selection = connection_identifier
265276
# Persist the connection so it is available next time the program opens
266277
# and so it survives periodic auto-discovery refreshes during the current session.
267-
ProgramSettings.store_connection(device)
268-
self.flight_controller.add_connection(device)
278+
ProgramSettings.store_connection(connection_identifier)
279+
# Update cache in-memory (most-recent-first, deduplicated) to avoid a disk re-read.
280+
self._connection_history_cache = [connection_identifier] + [
281+
c for c in self._connection_history_cache if c != connection_identifier
282+
]
283+
self.flight_controller.add_connection(connection_identifier)
269284
if self.destroy_parent_on_connect:
270285
self.parent.root.destroy()
271286
if self.download_params_on_connect and hasattr(self.parent, "download_flight_controller_parameters"):

tests/test_frontend_tkinter_connection_selection.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -991,6 +991,9 @@ def test_periodic_refresh_stops_on_connection(self) -> None:
991991
"ardupilot_methodic_configurator.frontend_tkinter_connection_selection.ProgressWindow"
992992
) as mock_progress_window,
993993
patch("ardupilot_methodic_configurator.frontend_tkinter_connection_selection.show_no_connection_error"),
994+
patch(
995+
"ardupilot_methodic_configurator.frontend_tkinter_connection_selection.ProgramSettings.store_connection"
996+
) as mock_store_connection,
994997
):
995998
mock_progress_window.return_value.destroy = MagicMock()
996999

@@ -1000,6 +1003,8 @@ def test_periodic_refresh_stops_on_connection(self) -> None:
10001003
# Assert: Refresh should be stopped and timer cleared
10011004
assert result is False # Connection successful
10021005
assert self.widget._refresh_timer_id is None
1006+
# Assert: The connection was persisted without filesystem side-effects in the test
1007+
mock_store_connection.assert_called_once_with("COM1")
10031008

10041009
def test_periodic_refresh_stops_on_window_close(self) -> None:
10051010
"""

0 commit comments

Comments
 (0)