Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display mirror angle #680

Merged
merged 14 commits into from
Nov 14, 2024
Merged

Display mirror angle #680

merged 14 commits into from
Nov 14, 2024

Conversation

dc2917
Copy link
Contributor

@dc2917 dc2917 commented Oct 31, 2024

Description

This PR adds the ability to display the current mirror angle on the GUI, as determined by the stepper motor controller.

The GUI should display the angle moved to (broadcast by the device once it has finished moving), or the name of the corresponding preset.

We'd like to also have the GUI indicate that the mirror is moving, however I don't think this is possible to test with the dummy device.

Fixes #583

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (non-breaking change that adds or improves the documentation)
  • Optimisation (non-breaking, back-end change that speeds up the code)

Key checklist

  • Pre-commit hooks run successfully (pre-commit run -a)
  • All tests pass (pytest)
  • The documentation builds without warnings (mkdocs build -s)
  • Check the GUI still works (if relevant)
  • Check the code works with actual hardware (if relevant)
  • Check the pyinstaller-built executable works (if relevant)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests have been added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

@dc2917 dc2917 requested a review from alexdewar October 31, 2024 13:49
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.97%. Comparing base (5ddc193) to head (e04340a).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
+ Coverage   86.92%   86.97%   +0.05%     
==========================================
  Files          69       69              
  Lines        3556     3570      +14     
==========================================
+ Hits         3091     3105      +14     
  Misses        465      465              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

I've made some small suggestions. Besides that there are a couple of bugs:

  1. The display shows the previous angle, not the current one (see comment)
  2. The display will never show a "moving" state, because _update_mirror_position_display is only ever called when the motor has finished moving. To figure out when it has started moving you need to listen for the move.begin message instead. My PR might help with working on this: Add dummy stepper motor delay in default configuration #681

finesse/gui/stepper_motor_view.py Outdated Show resolved Hide resolved
finesse/gui/stepper_motor_view.py Outdated Show resolved Hide resolved
finesse/gui/stepper_motor_view.py Outdated Show resolved Hide resolved
finesse/hardware/plugins/stepper_motor/st10_controller.py Outdated Show resolved Hide resolved
@alexdewar
Copy link
Collaborator

FYI: I'm just merging #694 and I've updated the required status checks for the Python 3.12 changes, so this branch will need updating before the checks pass now. You should just be able to do a simple merge/rebase.

@dc2917 dc2917 requested a review from alexdewar November 7, 2024 12:02
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

Almost there I think! But it still won't display "moving" when the mirror is moving. See comment.

otherwise, display angle or its associated name.
"""
if moved_to is None:
self.mirror_position_display.setText("Moving...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still something that needs tweaking if we want it to display "moving" when the mirror is moving. You need to have a separate handler which sets the text to moving when you get a move.begin message.

Comment on lines 81 to 85
preset = next((k for k, v in ANGLE_PRESETS.items() if v == moved_to), None)
if preset:
self.mirror_position_display.setText(preset.upper())
else:
self.mirror_position_display.setText(f"{moved_to}°")
Copy link
Collaborator

Choose a reason for hiding this comment

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

On reflection, I'm thinking it might be useful to see the angle for presets too. How about:

Suggested change
preset = next((k for k, v in ANGLE_PRESETS.items() if v == moved_to), None)
if preset:
self.mirror_position_display.setText(preset.upper())
else:
self.mirror_position_display.setText(f"{moved_to}°")
text = f"{moved_to}°"
if preset := next((k for k, v in ANGLE_PRESETS.items() if v == moved_to), None):
text += f" ({preset})"
self.mirror_position_display.setText(text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does display the names as, e.g. "cold_bb", rather than the text on the labels ("COLD BB"), but it's still obvious which each corresponds to haha

@dc2917 dc2917 force-pushed the display_mirror_angle branch from be429e4 to 4744088 Compare November 11, 2024 11:33
@dc2917 dc2917 requested a review from alexdewar November 11, 2024 11:38
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

LGTM! Just need to resolve that merge conflict then this is ready to go 😄

tests/gui/test_stepper_motor_view.py Outdated Show resolved Hide resolved
@dc2917 dc2917 force-pushed the display_mirror_angle branch from 548da22 to 7b1a7f1 Compare November 14, 2024 10:41
@dc2917 dc2917 merged commit a26a765 into main Nov 14, 2024
7 checks passed
@dc2917 dc2917 deleted the display_mirror_angle branch November 14, 2024 19:42
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.

Display current mirror angle (esp. when running script)
2 participants