linuswck
/
thermostat
forked from M-Labs/thermostat
1
0
Fork 0
Code Pull Requests 1 Activity

GUI: Control Panel changes #2

Open
atse wants to merge 21 commits from gui_dev-ctrl_panel into gui_dev
pull from: gui_dev-ctrl_panel
merge into: linuswck:gui_dev
Conversation 13 Commits 21 Files Changed 6 +733 -450
atse commented 2024-07-19 15:00:21 +08:00
Collaborator
Copy Link

Some improvements in usability and readability of the control panel portion of the GUI.

M-Labs/kirdy#14
M-Labs/kirdy#15

Some improvements in usability and readability of the control panel portion of the GUI. https://git.m-labs.hk/M-Labs/kirdy/issues/14 https://git.m-labs.hk/M-Labs/kirdy/issues/15
atse added 13 commits 2024-07-19 15:00:21 +08:00
8ac4a32106 ctrl_panel: Refer to Parameters by concise `name`s 
For displayed string representations, use the `title` key, or for
`ListParameter`s, use the dictionary mapping method instead.
eb3719044f ctrl_panel: Add tooltips 
For users' better understanding of what the parameters do
25c7a070d9 ctrl_panel: Approriate units for measured current 
Allow the readonly display of current to vary its SI prefix in the unit,
since as a display entry it won't have the unit adjustment problem.
39018d2360 ctrl_panel: Stop crushing spinboxes 
It might not be the case on some themes, but on the default Qt theme the
spinbox are a bit too short for the containing numbers. See
https://github.com/pyqtgraph/pyqtgraph/issues/701.
d76e015893 ctrl_panel: Code cleanup 
* Remove unnecessary duplication of `THERMOSTAT_PARAMETERS`

* i -> ch

* Separate ParameterTree and Parameter initiation

* Remove extra "channel" option to root parameters, as the "value"
option is already the channel number
a1c260d52e ctrl_panel: Limits fixes 
* PID Autotune test current should be positive

* Maximum absolute voltage should be 4 V not 5 V
89b908fab1 ctrl_panel: Remove MutexParameter 
Use the standard ListParamenter instead, and hook up UI changes
elsewhere.
e1fdc86e40 ctrl_panel: Keep i_set visible when PID engaged 
Since i_set is also plotted, we would also want to see its precise value
too.
atse force-pushed gui_dev-ctrl_panel from e1fdc86e40 to 21eb13ed6f 2024-07-19 15:14:52 +08:00 Compare
atse force-pushed gui_dev-ctrl_panel from 21eb13ed6f to 3552a582f8 2024-07-19 16:10:45 +08:00 Compare
linuswck commented 2024-07-22 11:41:47 +08:00
Owner
Copy Link

Bug:
On the GUI if you select "Temperature PID" Control Mode, you cannot switch it back to Constant Current Mode.

Bug: On the GUI if you select "Temperature PID" Control Mode, you cannot switch it back to Constant Current Mode.
👍 1
linuswck commented 2024-07-22 11:45:47 +08:00
Owner
Copy Link

Bug:
Click "Report" Mode. Temperature is not plotted.

Bug: Click "Report" Mode. Temperature is not plotted.
linuswck commented 2024-07-22 11:48:28 +08:00
Owner
Copy Link

Usability:
I think most users do not need to limit of TEC voltage with accuracy of less than 1mV.
How about fixing the unit voltage to be "V" only?
image

Usability: I think most users do not need to limit of TEC voltage with accuracy of less than 1mV. How about fixing the unit voltage to be "V" only? ![image](/attachments/bd1a8845-d2de-4a69-867f-22c1591295a5)
image.png
63 KiB
👍 1
linuswck commented 2024-07-22 11:50:25 +08:00
Owner
Copy Link

Usability:
Then, what does Off do? What is the sampling rate?
image

Usability: Then, what does Off do? What is the sampling rate? ![image](/attachments/63c06102-4954-469c-a007-c57ac4c4c762)
image.png
75 KiB
linuswck commented 2024-07-22 11:52:57 +08:00
Owner
Copy Link

Readability:
Better to Add a Readings group to "Temperature" and "Current Through TEC". More intuitive for first time user.
image

Readability: Better to Add a Readings group to "Temperature" and "Current Through TEC". More intuitive for first time user. ![image](/attachments/4e948766-8f13-4568-9d41-45937193cf18)
image.png
25 KiB
👍 1
linuswck reviewed 2024-07-22 12:28:21 +08:00
pytec/tec_qt.py Outdated
@ -268,3 +268,3 @@
if change == "value":
if inner_param.opts.get("param", None) is not None:
if inner_param.opts.get("suffix", None) == "mA":
if inner_param.opts.get("title", None).endswith(" (mA)"):
linuswck commented 2024-07-22 12:28:21 +08:00
Owner
Copy Link

Looks like a hack. The unit conversion should not be affected by the title.

Looks like a hack. The unit conversion should not be affected by the title.
atse commented 2024-07-31 17:19:39 +08:00
Author
Collaborator
Copy Link

Removed the need for it in latest force-push with the pinSiPrefix option of UnitfulSpinBox.

Removed the need for it in latest force-push with the `pinSiPrefix` option of `UnitfulSpinBox`.
atse marked this conversation as resolved
linuswck reviewed 2024-07-22 12:56:31 +08:00
pytec/pytec/gui/view/ctrl_panel.py Outdated
@ -84,0 +53,4 @@
param.child(*handle[0]).sigActivated.connect(handle[1])
param.child("output", "control_method").sigValueChanged.connect(
lambda param, value: param.child("i_set").setWritable(
linuswck commented 2024-07-22 12:56:31 +08:00
Owner
Copy Link

Usability:
I think this is duplicated. "Set Current" should not reflect the value being set by "Thermostat". That area of ctrl panel is configured by user afterall. It is confusing to me if it suddenly becomes a "Reading" item. Also, the current readings already reflect the current being set already.

Instead, I think we should keep the "Set Current" and and "Setpoint" always shown and configurable. And there are some sorts of indicator indicating which value is being used for control.

image

Usability: I think this is duplicated. "Set Current" should not reflect the value being set by "Thermostat". That area of ctrl panel is configured by user afterall. It is confusing to me if it suddenly becomes a "Reading" item. Also, the current readings already reflect the current being set already. Instead, I think we should keep the "Set Current" and and "Setpoint" always shown and configurable. And there are some sorts of indicator indicating which value is being used for control. ![image](/attachments/1dba729b-7728-400b-94db-46c8a997d50e)
👍 1
linuswck commented 2024-07-22 13:05:31 +08:00
Owner
Copy Link

Readability:
ADC Filter sampling rate is unrelated to Thermistor. Move it up a layer.
image

Readability: ADC Filter sampling rate is unrelated to Thermistor. Move it up a layer. ![image](/attachments/b7a0773a-aee1-4364-938b-9551bd05c661)
image.png
94 KiB
👍 1
linuswck commented 2024-07-22 13:06:23 +08:00
Owner
Copy Link

Can you bold the "Control Method" in ctrl panel?

Can you **bold** the "Control Method" in ctrl panel?
👍 1
atse force-pushed gui_dev-ctrl_panel from 3552a582f8 to 38099d6d7b 2024-07-31 17:18:52 +08:00 Compare
atse commented 2024-07-31 17:31:05 +08:00
Author
Collaborator
Copy Link

The above force-push fixes these mentioned issues in the ctrl_panel:

  1. max_v: Pin down the unit of voltage to be just "V"
  2. Put plotted values into readings group
  3. Removed the need for mA hack for all current values by adding the option pinSiPrefix
  4. Fixed not being able to switch back to constant current mode

In addition, it adds new changes to the ctrl_panel such as:

  1. Adding option noUnitEditing to SpinBoxes to disallow unit editing, instead of moving the unit to the title, so that the unit stays to the right of the number.
  2. Some appropriate tweaks to step values
  3. Fixed fields with unit °C that caused weird parsing due to a bug in PyQtGraph
The above force-push fixes these mentioned issues in the ctrl_panel: 1. `max_v`: Pin down the unit of voltage to be just "V" 2. Put plotted values into readings group 3. Removed the need for mA hack for all current values by adding the option `pinSiPrefix` 4. Fixed not being able to switch back to constant current mode In addition, it adds new changes to the ctrl_panel such as: 1. Adding option `noUnitEditing` to SpinBoxes to disallow unit editing, instead of moving the unit to the title, so that the unit stays to the right of the number. 2. Some appropriate tweaks to step values 3. Fixed fields with unit °C that caused weird parsing due to a bug in PyQtGraph
linuswck reviewed 2024-08-02 11:22:32 +08:00
pytec/pytec/gui/view/param_tree.json Outdated
@ -341,0 +89,4 @@
],
"format": "{value:.4f} {suffix}",
"suffix": "°C",
"regex": "(?P<number>[+-]?((((\\d+(\\.\\d*)?)|(\\d*\\.\\d+))([eE][+-]?\\d+)?)|((?i:nan)|(inf))))\\s*((?P<siPrefix>[uyzafpnµm kMGTPEZY]?)(?P<suffix>.*))?$",
linuswck commented 2024-08-02 11:22:32 +08:00
Owner
Copy Link

Replacing the incorrectly defined FLOAT_REGEX on pyqtgraph.functions is more preferable.

Replacing the incorrectly defined FLOAT_REGEX on pyqtgraph.functions is more preferable.
atse commented 2024-08-02 11:30:59 +08:00
Author
Collaborator
Copy Link

How do I do this as a patch to PyQtGraph itself? Seems to be a cleaner solution compared to patching it in our own code.

How do I do this as a patch to PyQtGraph itself? Seems to be a cleaner solution compared to patching it in our own code.
linuswck reviewed 2024-08-02 11:24:33 +08:00
pytec/pytec/gui/view/param_tree.json Outdated
@ -341,0 +221,4 @@
"step": 100,
"siPrefix": true,
"suffix": "Ω",
"noUnitEditing": true,
linuswck commented 2024-08-02 11:24:33 +08:00
Owner
Copy Link

You should fix all the siPrefix. Especially when we do not allow unit to be modified now. It will be confusing if the siPrefix of the unit suddenly changes.

You should fix **all** the siPrefix. Especially when we do not allow unit to be modified now. It will be confusing if the siPrefix of the unit suddenly changes.
👍 1
atse force-pushed gui_dev-ctrl_panel from 38099d6d7b to a6c852369f 2024-08-07 18:03:22 +08:00 Compare
atse force-pushed gui_dev-ctrl_panel from a6c852369f to c6fa3cdb80 2024-08-08 17:53:34 +08:00 Compare
atse commented 2024-08-08 17:58:06 +08:00
Author
Collaborator
Copy Link

The above force-push addresses these mentioned issues:

  1. Moved Postfilter settings back to its own layer
  2. r0 now has pinned SI prefix k.
  3. FLOAT_REGEX patched
  4. "Control Method" text is bolded.
  5. "Set Current" and "Setpoint" is now always shown and configurable, with the active control parameter's title underlined and bolded.
  6. Added sample rate text for when 50/60 Hz rejection filter is off.

In addition, it adds new changes such as:

  1. Use the new locking mechanism, ported over from the Kirdy GUI, to prevent spinboxes from changing when the user is editing it.
  2. Reformat SpinBoxes even when the submitted value after interpretation is the same as before.
The above force-push addresses these mentioned issues: 1. Moved Postfilter settings back to its own layer 2. `r0` now has pinned SI prefix `k`. 3. FLOAT_REGEX patched 4. "Control Method" text is **bolded**. 5. "Set Current" and "Setpoint" is now always shown and configurable, with the active control parameter's title underlined and bolded. 6. Added sample rate text for when 50/60 Hz rejection filter is off. In addition, it adds new changes such as: 1. Use the new locking mechanism, ported over from the Kirdy GUI, to prevent spinboxes from changing when the user is editing it. 2. Reformat SpinBoxes even when the submitted value after interpretation is the same as before.
linuswck reviewed 2024-08-09 18:11:59 +08:00
pytec/pytec/gui/view/unitful.py Outdated
@ -0,0 +91,4 @@
super().setOpts(**opts)
def editingFinishedEvent(self):
linuswck commented 2024-08-09 18:11:59 +08:00
Owner
Copy Link

The parameter SpinBoxes previously would only update if the interpreted
value was changed, missing cases where the text would have changed but
the value stays the same, e.g. removing trailing decimal zeros.

In what other scenario, you are bothered by this behavior. eg. 14 and 14.000 have the same value. Then, no need to send command to change to the same value?

> The parameter SpinBoxes previously would only update if the interpreted value was changed, missing cases where the text would have changed but the value stays the same, e.g. removing trailing decimal zeros. In what other scenario, you are bothered by this behavior. eg. 14 and 14.000 have the same value. Then, no need to send command to change to the same value?
atse commented 2024-08-12 11:03:06 +08:00
Author
Collaborator
Copy Link

It doesn't send any commands at all, it just updates the textual representation of the value within the SpinBox even if the evaluated value is the same. Since multiple textual strings can evaluate into the same value this is needed.

It doesn't send any commands at all, it just updates the textual representation of the value within the SpinBox even if the evaluated value is the same. Since multiple textual strings can evaluate into the same value this is needed.
atse commented 2024-08-12 11:04:20 +08:00
Author
Collaborator
Copy Link

For the record let me describe the scenario better with images:

This is the initial state of a SpinBox:

Screenshot from 2024-08-12 10-52-26.png

You can type 25 and hit enter, and this will display:

Screenshot from 2024-08-12 10-16-02.png

Crucially, the text doesn't end up going back to 25.0000. This means you can do the following:

Screenshot from 2024-08-12 10-16-38.png

Screenshot from 2024-08-12 10-16-54.png

Hit enter, and the text inside the SpinBox will stay like that, as they all represent the same value.

Yet, if you click away from the SpinBox, it'll still show the normal textual representation:

Screenshot from 2024-08-12 10-16-32.png

...and when you click back in again, the SpinBox still displays whatever text you've inputted previously.

For the record let me describe the scenario better with images: This is the initial state of a SpinBox: ![Screenshot from 2024-08-12 10-52-26.png](https://git.m-labs.hk/attachments/5474e2f0-4adb-45c6-bc5b-b678994250e8) You can type 25 and hit enter, and this will display: ![Screenshot from 2024-08-12 10-16-02.png](https://git.m-labs.hk/attachments/26a7b431-a873-4670-9c4a-d52f856a1fe9) Crucially, the text doesn't end up going back to 25.0000. This means you can do the following: ![Screenshot from 2024-08-12 10-16-38.png](https://git.m-labs.hk/attachments/67f99ad6-f4fb-416e-a6dd-c7c9cece3d61) ![Screenshot from 2024-08-12 10-16-54.png](https://git.m-labs.hk/attachments/ae4e537c-92d2-446a-a712-c3e3c98bdd9e) Hit enter, and the text inside the SpinBox will stay like that, as they all represent the same value. Yet, if you click away from the SpinBox, it'll still show the normal textual representation: ![Screenshot from 2024-08-12 10-16-32.png](https://git.m-labs.hk/attachments/bb15eca0-d2c6-41a1-944f-aa1bcdbea9ee) ...and when you click back in again, the SpinBox still displays whatever text you've inputted previously.
Screenshot from 2024-08-12 10-16-02.png
8.8 KiB
Screenshot from 2024-08-12 10-16-38.png
9.2 KiB
Screenshot from 2024-08-12 10-52-26.png
14 KiB
Screenshot from 2024-08-12 10-16-54.png
9.4 KiB
Screenshot from 2024-08-12 10-16-32.png
8.9 KiB
atse force-pushed gui_dev-ctrl_panel from c6fa3cdb80 to 01d023c3a6 2024-08-13 13:19:45 +08:00 Compare
atse commented 2024-08-13 13:20:49 +08:00
Author
Collaborator
Copy Link

The above force-push makes these changes:

  1. Renamed UnitfulSpinBox to LockableUnitSpinBox
  2. Fixed control parameter title not being emphasised if constant current was the control method on GUI connection to Thermostat.
  3. Reverted including rejection and effective sample rate to postfilter settings, and switched to better explanations in titles and tooltips
  4. Docstring changes on LockableUnitSpinBox
The above force-push makes these changes: 1. Renamed `UnitfulSpinBox` to `LockableUnitSpinBox` 2. Fixed control parameter title not being emphasised if constant current was the control method on GUI connection to Thermostat. 3. Reverted including rejection and effective sample rate to `postfilter` settings, and switched to better explanations in titles and tooltips 4. Docstring changes on `LockableUnitSpinBox`
atse force-pushed gui_dev-ctrl_panel from 01d023c3a6 to 5574559ac6 2024-08-16 14:09:30 +08:00 Compare
atse commented 2024-08-16 14:09:54 +08:00
Author
Collaborator
Copy Link

Missed a spot where mA scaling still existed in the PID Autotuner, removed that in the above force-push.

Missed a spot where mA scaling still existed in the PID Autotuner, removed that in the above force-push.
atse force-pushed gui_dev-ctrl_panel from 5574559ac6 to c9aa0eaab8 2024-10-07 16:38:30 +08:00 Compare
atse commented 2024-10-07 16:39:23 +08:00
Author
Collaborator
Copy Link

Rebased to the latest HEAD of the gui_dev branch.

Rebased to the latest HEAD of the `gui_dev` branch.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
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 gui_dev-ctrl_panel gui_dev
git pull origin gui_dev-ctrl_panel

Step 2:

Merge the changes and update on Gitea.
git checkout gui_dev
git merge --no-ff gui_dev-ctrl_panel
git push origin gui_dev
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
2 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: linuswck/thermostat#2
No description provided.
Delete Branch "gui_dev-ctrl_panel"

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?