M-Labs/thermostat
9
0
Fork 5
Code Issues 9 Pull Requests 5 Releases Wiki Activity

GUI #101

Merged
esavkin merged 1 commits from atse/thermostat:GUI into dev-gui 2024-08-17 17:37:18 +08:00
Conversation 8 Commits 1 Files Changed 11 +1243 -528
atse commented 2024-03-27 12:04:58 +08:00
Contributor
Copy Link

Adds a GUI for easier control of the Thermostat.

Core functionalities include plotting of temperatures and currents, reporting and adjustment of parameters, and PID autotuning.

Adds a GUI for easier control of the Thermostat. Core functionalities include plotting of temperatures and currents, reporting and adjustment of parameters, and PID autotuning.
esavkin reviewed 2024-03-27 12:30:46 +08:00
pytec/tec_qt.py Outdated
@ -0,0 +389,4 @@
self.thermostat_menu = QtWidgets.QMenu()
self.thermostat_menu.setTitle('Thermostat settings')
self.fan_group = QtWidgets.QWidget()
esavkin commented 2024-03-27 12:30:46 +08:00
Owner
Copy Link

Is there a good reason why these menu widgets are in the code instead of UI files? You may also separate UI into different reusable widgets in most cases

Is there a good reason why these menu widgets are in the code instead of UI files? You may also separate UI into different reusable widgets in most cases
esavkin reviewed 2024-03-27 12:34:34 +08:00
pytec/tec_qt.py Outdated
@ -0,0 +104,4 @@
while True:
time = loop.time()
await self.update_params()
await asyncio.sleep(self._update_s - (loop.time() - time))
esavkin commented 2024-03-27 12:34:34 +08:00
Owner
Copy Link

Well, you could have just skipped wait in update_params and there should not be such drift

Well, you could have just skipped wait in update_params and there should not be such drift
esavkin commented 2024-03-27 12:47:54 +08:00
Owner
Copy Link

In general I think this GUI needs more decoupling data from UI. It can be some state management, or custom data classes as well. I would look into something like https://doc.qt.io/qt-6/qstatemachine.html , or analogs of JS's redux/zustand, but in python.

In general I think this GUI needs more decoupling data from UI. It can be some state management, or custom data classes as well. I would look into something like https://doc.qt.io/qt-6/qstatemachine.html , or analogs of JS's redux/zustand, but in python.
esavkin reviewed 2024-03-27 14:07:25 +08:00
pytec/tec_qt.py Outdated
@ -0,0 +230,4 @@
DEFAULT_MAX_SAMPLES = 1000
"""Thermostat parameters that are particular to a channel"""
THERMOSTAT_PARAMETERS = [[
esavkin commented 2024-03-27 14:04:06 +08:00
Owner
Copy Link

Also I think it is possible to make this define not only the UI, but also getters/setters from the data state object.

Also I think it is possible to make this define not only the UI, but also getters/setters from the data state object.
esavkin commented 2024-04-08 10:25:04 +08:00
Owner
Copy Link

It is not an exactly straight-forward way, but you can inherit the ParameterTree or similar to add the additional fields. The getters/setters may be just functions, that accept state object and the incoming value, alternatively you can add lambda's, but that may need wrapping this list into class or function.

It is not an exactly straight-forward way, but you can inherit the ParameterTree or similar to add the additional fields. The getters/setters may be just functions, that accept state object and the incoming value, alternatively you can add lambda's, but that may need wrapping this list into class or function.
pytec/tec_qt.py Outdated
@ -0,0 +504,4 @@
self,
"About Thermostat",
f"""
<h1>Sinara 8451 Thermostat v{self.hw_rev_data['rev']['major']}.{self.hw_rev_data['rev']['minor']}</h1>
esavkin commented 2024-03-27 14:05:09 +08:00
Owner
Copy Link

This doesn't feel to be hardcoded.

This doesn't feel to be hardcoded.
atse changed title from GUI to WIP: GUI 2024-04-03 18:45:29 +08:00
linuswck reviewed 2024-04-09 13:42:37 +08:00
pytec/tec_qt.py Outdated
@ -0,0 +585,4 @@
@pyqtSlot(int)
def set_max_samples(self, samples: int):
for channel_graph in self.channel_graphs:
channel_graph.t_connector.max_points = samples
linuswck commented 2024-04-09 13:36:02 +08:00
Owner
Copy Link

The "max_points" feature does not work as I tested on GUI.
Upon inspection on the source code of the library. You cannot change it dynamically with the built-in fns

I tried to re-declare the data queue into the DataConnector class and it works as expected. Not sure if there is better way of doing it.

connector.max_points = self.max_samples
connector.x = deque(maxlen=int(connector.max_points))
connector.y = deque(maxlen=int(connector.max_points))
The "max_points" feature does not work as I tested on GUI. Upon inspection on the source code of the library. You cannot change it dynamically with the built-in fns I tried to re-declare the data queue into the DataConnector class and it works as expected. Not sure if there is better way of doing it. ``` Python connector.max_points = self.max_samples connector.x = deque(maxlen=int(connector.max_points)) connector.y = deque(maxlen=int(connector.max_points)) ```
esavkin reviewed 2024-04-09 15:51:44 +08:00
pytec/tec_qt.ui Outdated
@ -0,0 +27,4 @@
</property>
<property name="windowIcon">
<iconset>
<normaloff>thermostat-icon-640x640.png</normaloff>thermostat-icon-640x640.png</iconset>
esavkin commented 2024-04-09 15:51:44 +08:00
Owner
Copy Link

Well, I see two main problems with it (even if it is working):

  1. Such large PNG is not really suitable to be an icon, the standard is to use SVG and generate appropriate-sized PNGs if needed (in most cases SVG is fine).
  2. Photos are not really suitable to be an icons in modern UIs, as they usually have a lot of details, that will be just removed. Also it makes it feel the software to be 20+ years old. This particular photo will be looking like some black brick once it is displayed as a window icon of size 48px.

In this case, I would go with one of our standard ARTIQ icons, or create one from scratch with fewest details possible, targeting sizes 16x16, 32x32, 48x48, 64x64 and 256x256 pixels (it should just look okay being scaled to all sizes, no need to create different versions). For this, SVGs are most suitable, and they can be created in Inkscape (their UX is not good though) or any other software of your choice, and exported as Optimized SVG, but also preserving the original is desired in case of future needs (possibly can be uploaded to private repo).

Well, I see two main problems with it (even if it is working): 1. Such large PNG is not really suitable to be an icon, the standard is to use SVG and generate appropriate-sized PNGs if needed (in most cases SVG is fine). 2. Photos are not really suitable to be an icons in modern UIs, as they usually have a lot of details, that will be just removed. Also it makes it feel the software to be 20+ years old. This particular photo will be looking like some black brick once it is displayed as a window icon of size 48px. In this case, I would go with one of our standard ARTIQ icons, or create one from scratch with fewest details possible, targeting sizes 16x16, 32x32, 48x48, 64x64 and 256x256 pixels (it should just look okay being scaled to all sizes, no need to create different versions). For this, SVGs are most suitable, and they can be created in Inkscape (their UX is not good though) or any other software of your choice, and exported as Optimized SVG, but also preserving the original is desired in case of future needs (possibly can be uploaded to private repo).
esavkin commented 2024-04-10 15:41:45 +08:00
Owner
Copy Link

Actually better it be in separate PR, because the icon creation can take a while, and it is not really necessary to be merged with this PR

Actually better it be in separate PR, because the icon creation can take a while, and it is not really necessary to be merged with this PR
👍 1
linuswck commented 2024-04-12 13:45:29 +08:00
Owner
Copy Link

In general. When the widgets are in the "Disable State", the widget should be "gray-ed out" as a hint for the user that they cannot interact with them.

You can add the following line with QT Designer into the "styleSheet" properties of various types of widget.
QPushButton:disabled { color: gray }

In general. When the widgets are in the "Disable State", the widget should be "gray-ed out" as a hint for the user that they cannot interact with them. You can add the following line with QT Designer into the "styleSheet" properties of various types of widget. `QPushButton:disabled { color: gray }`
👍 1
linuswck reviewed 2024-04-12 13:51:19 +08:00
pytec/tec_qt.py Outdated
@ -0,0 +451,4 @@
async def network_settings(_):
ask_network = QtWidgets.QInputDialog(self)
ask_network.setWindowTitle("Network Settings")
ask_network.setLabelText("Set the Thermostat's IPv4 address, netmask and gateway (optional)")
linuswck commented 2024-04-12 13:49:58 +08:00
Owner
Copy Link

For the network related configuration, there should be a form for the user to fill in instead of entering the configuration as a line of text.

For the network related configuration, there should be a form for the user to fill in instead of entering the configuration as a line of text.
esavkin commented 2024-04-12 13:55:40 +08:00
Owner
Copy Link

Not really, making smooth transition between dots may be troublesome. Some regex validation though would be nice.

Not really, making smooth transition between dots may be troublesome. Some regex validation though would be nice.
linuswck commented 2024-04-12 16:09:22 +08:00
Owner
Copy Link

QT supports tab orderings. Will that be enough?

QT supports tab orderings. Will that be enough?
atse commented 2024-04-15 12:01:19 +08:00
Author
Contributor
Copy Link

Will Thermostat connections always be configured through an IPv4 address though? Don't we need to support more than that, like for instance hostnames resolved through DNS?

A use-case of this is using SSH local forwarding to remotely control a Thermostat with the GUI; this worked just fine once I adjusted the host and port.

Will Thermostat connections always be configured through an IPv4 address though? Don't we need to support more than that, like for instance hostnames resolved through DNS? A use-case of this is using SSH local forwarding to remotely control a Thermostat with the GUI; this worked just fine once I adjusted the host and port.
👍 1
esavkin commented 2024-04-12 14:17:21 +08:00
Owner
Copy Link

You can add the following line with QT Designer into the "styleSheet" properties of various types of widget.

I would go even further and create a separate QSS (Qt Style Sheets) file, so we can lock the appearance of the app, because in different DE configurations (especially Qt based like KDE) it may look different, and sometimes it may be broken. Also @atse already had issues with Adwaita.
Probably locking the style is not "libre software way", but I would say it is better to be usable and do not fit into systems style than be unusable (like barely visible icons or texts) while the appearance matches the system.

> You can add the following line with QT Designer into the "styleSheet" properties of various types of widget. I would go even further and create a separate QSS (Qt Style Sheets) file, so we can lock the appearance of the app, because in different DE configurations (especially Qt based like KDE) it may look different, and sometimes it may be broken. Also @atse already had issues with Adwaita. Probably locking the style is not "libre software way", but I would say it is better to be usable and do not fit into systems style than be unusable (like barely visible icons or texts) while the appearance matches the system.
atse force-pushed GUI from 68e6de7215 to 9806774382 2024-04-24 12:43:04 +08:00 Compare
atse force-pushed GUI from 9806774382 to d3dc9f9b37 2024-04-24 12:47:07 +08:00 Compare
linuswck commented 2024-04-25 16:35:14 +08:00
Owner
Copy Link

If PID autotune finishes, there should be a message/diag box to notify the user especially when autotune fails.

If PID autotune finishes, there should be a message/diag box to notify the user especially when autotune fails.
linuswck commented 2024-04-25 17:46:09 +08:00
Owner
Copy Link

When thermostat is autotuning and the connected device is disconnected, then it will end up in this interface.
image

The control panel is grey-ed out. Clicking the "connect" button does not do anything. You will need to re-run the GUI.

Here is the log. The following error message appears multiple times.

ERROR:root:Failed communicating to 192.168.1.26:23: [Errno 104] Connection reset by peer
Traceback (most recent call last):
  File "/nix/store/r31awq14kskwwq8jb0vfv7a08hsihlzc-python3.10-qasync-0.27.1/lib/python3.10/site-packages/qasync/__init__.py", line 780, in _error_handler
    task.result()
  File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 699, in bail
    await self._on_connection_changed(False)
  File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 615, in _on_connection_changed
    await self.client_watcher.set_report_mode(False)
  File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 134, in set_report_mode
    await self._report_mode_task
ConnectionResetError: [Errno 104] Connection reset by peer
Traceback (most recent call last):
  File "/nix/store/r31awq14kskwwq8jb0vfv7a08hsihlzc-python3.10-qasync-0.27.1/lib/python3.10/site-packages/qasync/__init__.py", line 780, in _error_handler
    task.result()
  File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 695, in on_connect_btn_clicked
    await self.bail()
  File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 691, in on_connect_btn_clicked
    await self.bail()
ConnectionResetError: [Errno 104] Connection reset by peer
When thermostat is autotuning and the connected device is disconnected, then it will end up in this interface. ![image](/attachments/867fb8b6-d72e-4101-b00c-2a2d1b31af82) The control panel is grey-ed out. Clicking the "connect" button does not do anything. You will need to re-run the GUI. Here is the log. The following error message appears multiple times. ``` ERROR:root:Failed communicating to 192.168.1.26:23: [Errno 104] Connection reset by peer Traceback (most recent call last): File "/nix/store/r31awq14kskwwq8jb0vfv7a08hsihlzc-python3.10-qasync-0.27.1/lib/python3.10/site-packages/qasync/__init__.py", line 780, in _error_handler task.result() File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 699, in bail await self._on_connection_changed(False) File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 615, in _on_connection_changed await self.client_watcher.set_report_mode(False) File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 134, in set_report_mode await self._report_mode_task ConnectionResetError: [Errno 104] Connection reset by peer Traceback (most recent call last): File "/nix/store/r31awq14kskwwq8jb0vfv7a08hsihlzc-python3.10-qasync-0.27.1/lib/python3.10/site-packages/qasync/__init__.py", line 780, in _error_handler task.result() File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 695, in on_connect_btn_clicked await self.bail() File "/home/linuswck/Desktop/thermostat_atse/pytec/tec_qt.py", line 691, in on_connect_btn_clicked await self.bail() ConnectionResetError: [Errno 104] Connection reset by peer ```
image.png
126 KiB
linuswck commented 2024-04-26 11:55:59 +08:00
Owner
Copy Link

Consider limiting the output current limit according to the specs(+-2A), I tried sourcing 3A into a TEC and Thermostat results in power failure. It requires full power cycle for Ethernet communication to function again.

Consider limiting the output current limit according to the specs([+-2A](https://github.com/sinara-hw/Thermostat/wiki)), I tried sourcing 3A into a TEC and Thermostat results in power failure. It requires full power cycle for Ethernet communication to function again.
esavkin commented 2024-04-29 17:36:27 +08:00
Owner
Copy Link

What about using tabs system instead of fixed panels? At least for settings area, it would make them longer and more convenient to operate one channel. Also in case of 4 channels thermostat tabs are easier to expand...
In this case, I guess current and temp graphs/charts can be merged into one, or as an alternative they can be detachable or removed, resized etc.

What about using tabs system instead of fixed panels? At least for settings area, it would make them longer and more convenient to operate one channel. Also in case of 4 channels thermostat tabs are easier to expand... In this case, I guess current and temp graphs/charts can be merged into one, or as an alternative they can be detachable or removed, resized etc.
esavkin commented 2024-04-29 17:55:54 +08:00
Owner
Copy Link

I feel this PR is going into a huge mess, since developing a GUI with good UI/UX is rarely an easy task, especially in this case, where there are quite a few nuances that may brake the whole picture. Also I think nobody will truly read it as whole.
I would suggest merging this GUI into some development branch of the main repo, and then iteratively refactor/redesign/add features/fix bugs. I would also suggest starting from specifying the requirements and drawing user/data flow diagrams.
As far as I saw it is not exactly this company policy, but it is a standard practice elsewhere, and I believe it would greatly improve the development process and the GUI itself, as other colleagues would also be able to participate in it.

I feel this PR is going into a huge mess, since developing a GUI with good UI/UX is rarely an easy task, especially in this case, where there are quite a few nuances that may brake the whole picture. Also I think nobody will truly read it as whole. I would suggest merging this GUI into some development branch of the main repo, and then iteratively refactor/redesign/add features/fix bugs. I would also suggest starting from specifying the requirements and drawing user/data flow diagrams. As far as I saw it is not exactly this company policy, but it is a standard practice elsewhere, and I believe it would greatly improve the development process and the GUI itself, as other colleagues would also be able to participate in it.
👍 1
esavkin changed target branch from master to dev-gui 2024-05-08 14:47:36 +08:00
esavkin changed title from WIP: GUI to GUI 2024-05-08 14:48:46 +08:00
esavkin force-pushed GUI from d3dc9f9b37 to 8753f4a0fc 2024-05-08 14:49:04 +08:00 Compare
esavkin merged commit 8753f4a0fc into dev-gui 2024-05-08 14:50:12 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/thermostat#101
No description provided.
Delete Branch "atse/thermostat:GUI"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?