ControlSystemStudio / cs-studio

Control System Studio is an Eclipse-based collections of tools to monitor and operate large scale control systems, such as the ones in the accelerator community.
https://controlsystemstudio.org/
Eclipse Public License 1.0
113 stars 96 forks source link

Alarm pulsing in BOY #661

Closed berryma4 closed 10 years ago

berryma4 commented 10 years ago

@kasemir @shroffk @utzeln

@tonlyttt is proposing the following changes: pull request #653

At DLS we use the red colour to indicate OFF, as well as to indicate an alarm state. This is confusing, so we have a requirement to make alarms pulse. This also makes the panels more friendly for colour blind people.

To implement this, a new boolean property "alarm_pulsing" (with default value false) has been added to the IPVWidgetModel, and an implementation in PVWidgetEditpartDelegate pulses the alarm colour of any alarm sensitive part of the widget around the midpoint of the normal colour and the alarm colour. Period is 1.5s for MAJOR, 3s for MINOR, and no pulse for DISCONNECTED.

kasemir commented 10 years ago

Eric, thanks for linking that pull request to a ticket.

We currently have alarm sensitive (border) colors, as in EDM/MEDM/.. There have been several comments on that, mostly to support the color blind. One request was to add decorators. Now we have blinking colors. I remember mention of beeping.

I don't think we want to support all of that and some some, especially not while we lack an overall maintainer for BOY, and we have some real problems in very basic areas like the performance of the rudimentary PV -> display chain.

Tom's specific justification, using red to indicate OFF, doesn't convince me at all. When you deal with alarms, a specific color, usually it is in fact red, should indicate a severe alarm. "off" is just a state, and often it is a good state. Simply displaying off=red, on=green is in fact one of the first examples in the Hollifield/Habibi "Alarm Management" book on what not to do.

Still, on the upside Tom's specific implementation seems benign enough, and I can see how even I may want to display some PVs with a blinking color in addition to the default alarm-sensitive border. If the blink periods are globally configurable, this could work for several sites. Would be good to check/assert that it supports both draw-2d and native widgets, and some guess for the performance impact. Especially on Linux commonly used in the control room, the display updates aren't super efficient, so adding blinking redraws when there's really no change in the value concern me a little.

thomascobb commented 10 years ago

Agreed, red should be alarm state. Unfortunately we have thousands of panels where it cannot be programmatically determined whether red means off or alarm state, and even if I got agreement to change them, it's a massive task to change them all, so we're stuck with needing two kinds of red.

My implementation does the following:

As for performance, a screen with 15 alarm sensitive and pulsing buttons on it takes:

This will not at the moment pulse the alarm border. I tried to use the same method as recalculating alarm colours, but it turns out that replacing the border of a widget at 10Hz is very expensive, 140% CPU for the same screen. I wonder if caching the borders and swapping them out might be better...

It doesn't support the native button as you can't set its background colour. I have hidden the option wherever alarm sensitive background is hidden, so it shouldn't cause confusion. It works fine for the combo box.

I will have a look at adding MINOR and MAJOR pulse period to the eclipse preferences. I should probably add framerate to there too (fixed at 10Hz at the moment). I will update the pull request when I'm finished.

thomascobb commented 10 years ago

I also share your concerns about the performance of BOY, and we will be looking at that before rolling CS-studio out, it is just easier to get the design of some screens down with all the features that we would like before focussing on how to make them faster.

kasemir commented 10 years ago

Thanks for checking the performance. No-change-when-not-used is very good.

As for making it faster later, I worry that some key elements of BOY could benefit from a re-do.

If we keep adding, we'll later face more code that we need to re-do the right&fast way, once we find the right&fast way. I think we have a very good operational prototype, and should look for the right&fast way ASAP, instead of continuing to tweak GUI aspects. Since you're now familiar with the workings of widgets, maybe you can be part of that?

thomascobb commented 10 years ago

I will leave it to @willrogers to comment on how we will be contributing long term, I imagine DLS as a whole will be putting some effort into doing things the right way, although it's unlikely that I will be the one doing the actual coding...

For my part, I only required these two changes to demonstrate a prototype of what beamline screens could look like in BOY, so I can't see myself adding any more features to existing BOY widgets anytime soon.

willrogers commented 10 years ago

I'd certainly be interested in taking some part in the BOY work.

Incidentally, by 'operational prototype', do you mean BOY as it is now?

kasemir commented 10 years ago

@willrogers Yes, BOY as now is the operational prototype. @tonlyttt Thank you very much for reconsidering the bold-button-font in #660. "you've convinced me" is a first, I really appreciate that!

The color-blind aspect of blinking alarm colors is important. So is the "tab" support that Eric has mentioned before, or generally mouse-less operation. Should be able to "tab" to a button, push enter or space to activate; or "tab" to a slider, use mouse keys to operate it, without depending on the mouse.

But we should do that by deciding on one good way to indicate alarms, and then every widget does it. I'd prefer for the widgets to just have an "alarm_sensitive: yes/no" property, with a color-blind-compatible representation. Not an alarm-sensitive border property used by one site, blinking border for another site, alarm-sensitive background color for a third site, alarm-dependend decorator icons in the upper left corner of the widget for a fourth site, where native widgets implement some of those options, Draw2D widgets another subset of the options.

If the blinking border is indeed necessary to make the alarm-sensitive border color-blind-proof, maybe that's just it: Update the alarm-sensitive border to always blink?

utzeln commented 10 years ago

For ITER, the HMI guidelines from human factors and ergonomics study which apply both on alarm table and BOY are the following:

  1. Colour:
  2. The background colour of the alarm message [alarm table]/symbol/text update [BOY] shall reflect the priority of the alarm
  3. The colour of the alarm priority symbol shall reflect the priority of the alarm

so for ITER, the background alarm sensitive shall be used

BTW, it could be could to have common colour settings - both for the alarm and BOY. For now, there are set via different preferences and so a MAJOR alarm colour could be different in BOY and in the alarm table

  1. Alarm priority symbol in the alarm table:
  2. in addition to colour coding, a symbol shall indicate the priority of the alarm (stop, warning icons)

new requirement for the alarm GUI - the icon should probably reflect the current alarm state this should answer the concern about colour-blind for at least the alarm table

  1. Use of flashing:
  2. The alarm priority symbol/icon in the alarm table shall flash at 2Hz until it is acknowledged.

not sure that we need that as the un-acknowledged alarms are in the active alarm table, separated from the acknowledged ones

  • On a VDU workstation the flash timing of symbols must be synchronised for all flashing states.
  • The alarm message must never flash.
  • The background of a widget in BOY shall flash at 2Hz until acknowledgement

I would propose to open another thread in git issues to discuss this point

Example of alarm state representation for symbols:

image

  1. Interaction:
  2. Users shall be able to individually acknowledge alarms from mimics, by double-clicking on the widget

would be part of the other discussion thread

  • Once acknowledged the widget shall no longer flash
kasemir commented 10 years ago

Sounds good to have one configuration for MINOR=yellow, MAJOR=red, .. that is used wherever anything is colored based on an alarm severity.0

Problem with generally using the background : You can do that in the alarm table, where the default background is white, so you pick a yellow and red that shows up OK. But if you make every BOY widget's background use those same colors, trouble is that the default background is not white. Instead, you have a site-specific preference of white, black, pale green, anything. Plus widgets that just 'update' may have one background, while widgets where people can 'enter' something tend to have another background. ==> You would have to pick alarm-sensitive background colors that work with any possible default background color.

We went through that with EDM, and decided on using the border instead of the background for that reason: Our overall display background tends to be light, and the alarm colors in the border show up against it. For the widget background, which is gray for text updates, blue for text entries, .. there was no one alarm color that worked for all. In ITER examples, I see text updates with gray background and black text -> A yellow alarm background would work OK. But in the same display I see a control widget with blue background and white text -> With yellow alarm background, you could no longer read the white-on-yellow text. So you'd have an alarm that you cannot read, quite bad!

screen shot 2014-08-27 at 10 08 24 am

This adds to my general leaning that we should allow less configuration rather than more. If we had never allowed anybody to configure any color, we'd be fine. Just having a "Text Update" with "PV" and "alarm_sensitive=Yes" would allow you to display that as Black text on White background when all is fine, as Black text on Yellow background for minor, and as White text on Red background for major. A "Text Entry" with "PV" and "alarm_sensitive=Yes" would allow us to use White-on-Blue for normal, Black-on-Yellow for minor alarm etc.

But we allowed people to pick a text and background color, so now you can't just change the background color based on an alarm, since it may result in unreadable text depending on their text color.

utzeln commented 10 years ago

Now ITER guidelines for text input and text update widgets are the following:

image

The ergonomics guys make them compatible with the background alarm sensitive colour but they do not make it clear if this alarm information is shown on any type of widgets, as they mainly focus on symbols (valve, pumps...).

kasemir commented 10 years ago

Good, that would be compatible with alarm-sensitive backgrounds. I actually like that color scheme. Can everybody agree to it, so we remove alarm-sensitive border, and "alarm_sensitive" for now means background?

Still doesn't help with color-blindness. For that, decorators like a triangle /!\ warning, a stop-sign type alarm, a (?) disconnect/invalid indicator is required that can be distinguished regardless of color. The technical issue with that is that you cannot draw outside the widget's border. The alarm sensitive border already shows some of that: Since it needs to be inside the overall widget outline, a widget "shrinks" a little when you enable the alarm-sens. border, just to make room for it. So the widget size needs to grow to accommodate the decorator, or the icon need to turn into (temporary) extra widgets.

thomascobb commented 10 years ago

Sounds like a reasonable idea, although I'm not sure how well removing an existing feature would go down with some people. At DLS we will be using CSS in two modes. For the machine we just want it to look like EDM, complete with any idiosyncrasies required to make the screens legible. For beamlines we will be mostly starting from scratch, although low level screens will probably be converted from EDM.

For colour blindness, I think you could still get away without decorators if you pulse the background alarm colour at different frequencies depending on the severity of the alarm. That was my rationale behind making the alarm colour pulse rather than flash, it's easier on the eyes, and a low frequency pulse is more recognisable than a low frequency pulse.

@utzeln : are the ITER display guidelines publicly available somewhere? Could I have a copy please?

kasemir commented 10 years ago

Does pulse vs. flash refer to a gradual change between two colors vs. a simple color1, color2 switch? For the webopi that might be very expensive. Every redraw that is not absolute essential to show the current value is best avoided in the web display. By basing the pulse period and the number of steps on preferences, the webopi could be configured to have just 1 step, i.e. a basic flash, at a low rate to make it practical for the web display.

That was my rationale behind making the alarm colour pulse rather than flash

thomascobb commented 10 years ago

Yes, pulse refers to a gradual change. I tied the framerate of the animation into the GUI refresh period, so there's one number to change if you want slower refreshing.

kasemir commented 10 years ago

tied the framerate of the animation into the GUI refresh period

That's good, thanks!

thomascobb commented 10 years ago

At the end of this thread I have the impression that there are no objections left to the pulsing alarm option. If this is the case, could someone merge the changes in please?