Skip to content

Refactor liquid engine gui#8

Open
niekky wants to merge 1 commit into
mainfrom
nn/refactor
Open

Refactor liquid engine gui#8
niekky wants to merge 1 commit into
mainfrom
nn/refactor

Conversation

@niekky

@niekky niekky commented Apr 16, 2026

Copy link
Copy Markdown
Contributor
  • Implemented DashboardWindow class for the engine dashboard using Tkinter.
  • Created ButtonFactory for centralized button creation for valve and sequence buttons.
  • Developed CallbackHandler to manage button callbacks and window lifecycle.
  • Introduced Liquid_Engine_State class to manage engine state transitions.
  • Added Sensor_Data_Buffer for FIFO low-pass filtering of sensor data.
  • Created TelemetryProcessor to handle telemetry data processing and logging.

- Implemented DashboardWindow class for the engine dashboard using Tkinter.
- Created ButtonFactory for centralized button creation for valve and sequence buttons.
- Developed CallbackHandler to manage button callbacks and window lifecycle.
- Introduced Liquid_Engine_State class to manage engine state transitions.
- Added Sensor_Data_Buffer for FIFO low-pass filtering of sensor data.
- Created TelemetryProcessor to handle telemetry data processing and logging.

@ETSells ETSells left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm happy with this! feel free to ship once tested :)

Comment thread app_window.py
root.protocol( "WM_DELETE_WINDOW", self._callbacks.close_window )

# Load logo images (kept on self to prevent garbage collection)
self._sdr_logo = tk.PhotoImage(file='images/SDRLogo5.png')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: bad (non-descriptive) file and variable naming here w/ sdr_logo vs sdr_img

Comment thread app_window.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

while we're working on a redesign, consider licensing the gui.

the primary library in use is SDEC, which uses the BSD-3-clause

Comment thread button_factory.py
"""Abstract factory that centralises construction of valve and sequence buttons.

Having widget constructor details in one place means a single edit propagates
to every button rather than hunting through the initialisation block."""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[too nitpicky, feel free to ignore, no change necessary] okay, i suppose this is a valid reason, but you could also use variables in the initialization block like "valve_initialization_fg_color" or something. the factory seems like a bit of a roundabout way to do it.

Comment thread callbacks.py
"""
Parameters
----------
engine_state : Liquid_Engine_State

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

consider using actual strong typing for these parameters if you know what they're expected to be

Comment thread callbacks.py
self._terminal = terminal
self._valve_buttons = valve_buttons
self._exit_flag = exit_flag
self._window = None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

be careful with this -- very easy way to get an AttributeError if set_window isn't called before you call other methods. there's no handling for issues that could come from _window being None

Comment thread callbacks.py
engine_state : Liquid_Engine_State
terminal : sdec.terminalData
valve_buttons : dict — shared mutable dict populated by DashboardWindow
exit_flag : list — single-element mutable bool, e.g. [False]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this a list?

Comment thread liquid_engine_state.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider something like a strenum for this so you're not doing string comparisons as much:

from enum import StrEnum, auto

class HttpMethod(StrEnum):
    GET = auto()     # Value is "get"
    POST = "POST"    # Value is "POST"
    DELETE = auto()  # Value is "delete"

Comment thread main.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as before, let's try to get licensing in here. crediting devs is fine, but we should make it clear that this is explicitly the IP of Sun Devil Rocketry as an org and not that of the individual contributors.

Comment thread main.py
# Widget initializations #
################################################################################
# --- Wire components --------------------------------------------------------
exit_flag = [False] # mutable container so CallbackHandler can set it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we not just use the tk mutable containers or something? this list thing seems really hacky

Comment thread sensor_data_buffer.py
self.data_buffer.append( sensor_data )
self.filter_buffer.append( self.filter_data() )

def filter_data( self ):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

niiiiice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants