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

Switch interface rework #623

Merged
merged 64 commits into from
Dec 11, 2020
Merged

Switch interface rework #623

merged 64 commits into from
Dec 11, 2020

Conversation

kay-jahnke
Copy link
Member

@kay-jahnke kay-jahnke commented Oct 22, 2020

The whole tool chains for simple switches has been reworked. The hardware controlled by this can simply be switched between any two or more different states without the need for any timing (software timed).

Attention: This is a change to an interface, so it is a breaking change for old switch hardware wrappers not included in qudi that use the SwitchInterface!

Description

The old tool chain was quite old and did not conform to the qudi guidelines by using camel-case, old connector types and not making use of signal. The new logic now conforms to the guidelines and make excessive use of pythonic properties and setters.

  • A change of the switch state in the logic in now reflected in the gui by transmitting signals
  • Names can be given to states, switches and the hardware itself
  • Switches are referenced by their given name
  • States are be referenced their name

The GUI has changed to Radio Buttons to reflect the option to set more than 2 states:
Coloured radio buttons are optimized for better readability when wearing laser protection goggles in the lab:
image

How Has This Been Tested?

Tested on Linux 5.8.11-1-MANJARO with dummy config.
Please additionally test with the hardware when you have it available.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have documented my changes in the changelog (documentation/changelog.md)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added/updated for the module the config example in the docstring of the class accordingly.
  • I have checked that the change does not contain obvious errors (syntax, indentation, mutable default values).
  • I have tested my changes using 'Load all modules' on the default dummy configuration with my changes included.
  • All changed Jupyter notebooks have been stripped of their output cells.

@kay-jahnke
Copy link
Member Author

@alrik-durand I think the Thorlabs ows12 module was originally from you. As I don't have any of these, could you please test it with your hardware.

Also it would be perfect if you adapted PR #620 to the new interface, once this PR is through.

@alrik-durand
Copy link
Contributor

Hi Kay, thank you for this rework, this is a big improvement !

I will test the OSW12 asap ! Also my PR #603 will need rework, I'll rework both PR as soon as I can.

Can I make a suggestion about switch logic ? Some devices have a physical button to change the state of the switch. For them the value can change without the logic being aware of. What do you think about having an optional (via config option) loop that check periodically (also via config) the state of the hardware ?

@Neverhorst
Copy link
Member

My GUI tweaks were still missing but are now merged into this branch. Testing can commence...

@alrik-durand
Copy link
Contributor

@kay-jahnke Thank you for the consideration, indeed I think this is an improvement.
I would go even further into module organization but maybe an example of what I'm proposing is better than long explanation. I've pushed on my repo a WIP I've done starting from your branch. It is not finished yet (I have to pause for today) but it gives an idea.
In a few word I'm proposing that the logic translate the names used by the hardware (which are strings specific to the hardware, but not configurable) to configurable names used by the logic and gui specified by config.
To the GUI or scripts there is no difference, but for hardware modules things are much simpler.

I'll try to finish what I've started in the next few days.

@Neverhorst
Copy link
Member

@alrik-durand Well, I really do not share the opinion that the names should be defined by the logic. In Python it makes no difference (in most cases) if you address something by name or by index. We just chose "by name" because it makes it more verbose and easy to understand/read (which is a pillar of qudi and Python in general). Also you do not need to keep both the name and index information (lesser complexity).
Additionally all modules using a specific hardware module share the same information by default if the hardware is defining the names, which is the more direct and less confusing approach compared to multiple modules defining different names for the same hardware switch.
The argument about code duplication in activation is now invalidated since it is all wrapped in an interface method.
Also nobody is forcing you to make the names a ConfigOption in the hardware modules; you could just make them hard coded for simple hardware.
All in all the hardware modules are really lean now and just define the basics.

…y way inferior to the ToggleSwitch regarding the SwitchGui.
# Conflicts:
#	documentation/changelog.md
@kay-jahnke
Copy link
Member Author

@Neverhorst
it seems the yaml representer has some problems with the new Enums you defined in the GUI:

Failed to save status variables of module gui.switches:
OrderedDict([('pos_x', 632), ('pos_y', 249), ('switch_style', <SwitchStyle.TOGGLE_SWITCH: 0>), ('colorscheme', <ColorScheme.DEFAULT: 0>), ('alt_toggle_switch_style', False)])
Traceback (most recent call last):
  File "C:\Software\qudi\core\manager.py", line 1292, in saveStatusVariables
    config.save(filename, variables)
  File "C:\Software\qudi\core\config.py", line 250, in save
    ordered_dump(data, stream=f, Dumper=yaml.SafeDumper, default_flow_style=False)
  File "C:\Software\qudi\core\config.py", line 227, in ordered_dump
    return yaml.dump(data, stream, OrderedDumper, **kwds)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\main.py", line 849, in dump
    version=version, tags=tags, block_seq_indent=block_seq_indent)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\main.py", line 810, in dump_all
    dumper._representer.represent(data)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\representer.py", line 73, in represent
    node = self.represent_data(data)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\representer.py", line 101, in represent_data
    node = self.yaml_representers[data_types[0]](self, data)
  File "C:\Software\qudi\core\config.py", line 165, in represent_ordereddict
    dict_data.items())
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\representer.py", line 206, in represent_mapping
    node_value = self.represent_data(item_value)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\representer.py", line 111, in represent_data
    node = self.yaml_representers[None](self, data)
  File "C:\Software\Anaconda3\envs\qudi\lib\site-packages\ruamel\yaml\representer.py", line 375, in represent_undefined
    raise RepresenterError("cannot represent an object: %s" % data)
ruamel.yaml.representer.RepresenterError: cannot represent an object: SwitchStyle.TOGGLE_SWITCH

@Neverhorst
Copy link
Member

Neverhorst commented Nov 23, 2020

it seems the yaml representer has some problems with the new Enums you defined in the GUI:

Ok, that's weird. IntEnum is a subclass of int which is why I expected the representer to identify it as such. Seems like the representer is checking type using type(). This is why you should (almost) always use isinstance().
I will look into it.

The issue of saving enums will be generally addressed in the new qudi core.
@Neverhorst
Copy link
Member

@kay-jahnke
Introduced a quick workaround for yaml. Also fixed a bug with saving and restoring switch states.

@kay-jahnke
Copy link
Member Author

@Neverhorst thanks for the fix. I tested and it works for me now.

@alrik-durand
Copy link
Contributor

@Neverhorst As I've proposed on my demo, the hardware should use names as indeed it is more explicit than index.
Any real hardware has a convention anyway on what the states are : 1/2, High/Low, A/B, etc. It seems natural to let the hardware module keep this convention as it help understanding the mapping. If we want to have a more explicit naming, the user is creating a second convention and that's ok - we just need to define the mapping by specifying the config. I think this is complex not complicated.
I agree code duplication can be partially resolved by using a trick with the interface, but this violate the division into hardware, logic, and interface a lot. If some logic checks is to be done to every hardware module, it should be done by logic.

With that said, I let you decide what is best.

@GRexBayer
Copy link

GRexBayer commented Nov 23, 2020

Edit: I restarted QuDi and now it works, sorry!

I tried running the SwitchGUI from the "qudi: Manager" on my office Computer (Win 10) with the default config.
It shows me the following error message:

Error during activation
AssertionError: Invalid switch state: "eins"
Traceback (most recent call last):

File "C:\Program Files\Anaconda3\Lib\site-packages\qudi\core\module.py", line 98, in wrap_event
base_event(*args, **kwargs)

File "C:\Program Files\Anaconda3\envs\qudi\lib\site-packages\fysom_init_.py", line 311, in fn
self.transition()

File "C:\Program Files\Anaconda3\envs\qudi\lib\site-packages\fysom_init_.py", line 306, in _tran
self._after_event(e)

File "C:\Program Files\Anaconda3\envs\qudi\lib\site-packages\fysom_init_.py", line 338, in _after_event
return getattr(self, fnname)(e)

File "C:\Program Files\Anaconda3\envs\qudi\lib\site-packages\fysom_init_.py", line 95, in _callback
return func(obj, *args, **kwargs)

File "C:\Program Files\Anaconda3\Lib\site-packages\qudi\core\module.py", line 223, in __load_status_vars_activate
self.on_activate()

File "C:\Program Files\Anaconda3\Lib\site-packages\qudi\gui\switch\switch_gui.py", line 149, in on_activate
self._switches_updated(self.switchlogic().states)

File "C:\Program Files\Anaconda3\Lib\site-packages\qudi\gui\switch\switch_gui.py", line 237, in _switches_updated
self._widgets[switch][1].set_state(state)

File "C:\Program Files\Anaconda3\Lib\site-packages\qudi\gui\switch\switch_state_widgets.py", line 73, in set_state
assert state in self.switch_states, f'Invalid switch state: "{state}"'

Perhaps I messed up somewhere?

@GRexBayer
Copy link

I think this is just a minor detail, but upon reopening, the module remembers all the view settings for the Toggle bars (whether one had toggle bars or not).
But it does not remember the color scheme (red-green)

…based on the current Qt palette of the switch main window.
@Neverhorst
Copy link
Member

I think this is just a minor detail, but upon reopening, the module remembers all the view settings for the Toggle bars (whether one had toggle bars or not).
But it does not remember the color scheme (red-green)

This should be fixed now.

@GRexBayer
Copy link

Checked the GUI again on Windows 10 OS with dummy hardware.
Under the possible operations I could think of, the program runs smooth. Unless I clean the Module status file, it remembers the preferences for the button style. Reloading the config file with the module deactivated or activated seems to result in no error.

…ing process (moved _switching_time to the query comand instead of time sleep afterwards)
@Yawghmoth
Copy link
Member

Tested flip-mirror (radiant dyes box) which is now working after small changes on the flip-mirror hardware file

Copy link
Member

@Yawghmoth Yawghmoth left a comment

Choose a reason for hiding this comment

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

Tested with an Radiant dyes flippmirror, which worked so far

Copy link
Member

@Yawghmoth Yawghmoth left a comment

Choose a reason for hiding this comment

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

changed ask to querry (not add)

Copy link
Contributor

@jochen-scheuer jochen-scheuer left a comment

Choose a reason for hiding this comment

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

Tested it on two different setups and works as intended.

@kay-jahnke kay-jahnke merged commit 895a56a into master Dec 11, 2020
@kay-jahnke kay-jahnke deleted the switch_interface_rework branch December 11, 2020 08:12
@Neverhorst Neverhorst mentioned this pull request Jan 26, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants