M-Labs
/
thermostat
9
0
Fork 4
Code Issues 8 Pull Requests 7 Releases Wiki Activity

WIP: GUI #101

Draft
atse wants to merge 13 commits from atse/thermostat:GUI into master
pull from: atse/thermostat:GUI
merge into: M-Labs:master
Conversation 6 Commits 13 Files Changed 13
atse commented 2024-03-27 12:04:58 +08:00

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.
atse added 12 commits 2024-03-27 12:04:59 +08:00
549dacd2c6 Try move from Qthreads to qasync 
Signed-off-by: Egor Savkin <es@m-labs.hk>
6c08a7fb07 Finish moving over to qasync 
Also:

* Add aioclient

The old client is synchronous and blocking, and the only way to achieve
true asynchronous IO is to create a new client that interfaces with
asyncio.

* Finish Nix Flake description and make the GUI available for `nix run`
ecf89f45bf Stop polling drift 
Just waiting for the update_s doesn't take into account the time to
execute update_params, and causes time drift.
6e91373af4 Add paramtree view, without updates 
Signed-off-by: Egor Savkin <es@m-labs.hk>

Fix signal blocker argument -atse
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

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

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

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

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

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

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

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

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

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

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

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

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

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
Poster

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

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

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

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

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.
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b atse-GUI master
git pull GUI

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff atse-GUI
git push origin master
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
There is no content yet.
Delete Branch "atse/thermostat:GUI"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?