openhab-webui icon indicating copy to clipboard operation
openhab-webui copied to clipboard

[BasicUI] HSB Colorpicker sets 50% brightness when color is “picked”

Open Trickx opened this issue 5 years ago • 4 comments

Setup:

  • OH 2.5.9
  • Tasmota HSB driven LED bulb connected via MQTT

Issue I have an issue with a colorpicker (HSB). When I open the colorpicker, it shows correct color and brightness information. My Tasmota bulb sends JSON-formatted result strings via MQTT in case of an update. While JSONpath parsing of a switch works fine, the colorpicker jumps back to 50% brightness after setting a color in most cases (but not always). The command is send correctly.

It seems that the status update is not processed correctly. JSONpath parsing works fine when triggered externally as well as via the colorpicker:

2020-10-21 15:17:01.278 [ome.event.ItemCommandEvent] - Item ‘BTL_bulb_1_color’ received command 238,92,100
2020-10-21 15:17:01.327 [vent.ItemStateChangedEvent] - BTL_bulb_1_color changed from 359,100,100 to 238,92,100
2020-10-21 15:17:05.348 [ome.event.ItemCommandEvent] - Item ‘BTL_bulb_1_color’ received command 116,6,100
2020-10-21 15:17:05.427 [vent.ItemStateChangedEvent] - BTL_bulb_1_color changed from 238,92,100 to 116,6,100
2020-10-21 15:17:08.384 [ome.event.ItemCommandEvent] - Item ‘BTL_bulb_1_color’ received command 46,98,100
2020-10-21 15:17:08.424 [vent.ItemStateChangedEvent] - BTL_bulb_1_color changed from 116,6,100 to 46,98,100
2020-10-21 15:17:09.315 [ome.event.ItemCommandEvent] - Item ‘BTL_bulb_1_color’ received command 303,82,100
2020-10-21 15:17:09.391 [vent.ItemStateChangedEvent] - BTL_bulb_1_color changed from 46,98,100 to 303,82,100

As you can see, brightness was always set to 100, but the colorpicker shows 50%.

Additionally, when I change the color externally and re-open the colorpicker, a white color and brightness of 50% are shown. My interpretation is that the communication between the bulb item and the colorpicker is somehow buggy.

The setup works fine via the iOS app, so openHAB config should be ok. This issue has been discussed also at openHAB's community forum.

Trickx avatar Oct 21 '20 18:10 Trickx

From the logs that you have provided, I'd think you are right that this is a bug in the colorpicker. @lolodomo As you have helped a lot in the past on the Basic UI, might you be interested in looking into this?

kaikreuzer avatar Oct 21 '20 20:10 kaikreuzer

Yes, the color picker looks a little buggy. My current feeling is that it could be a HSB)RGB convention problem.

lolodomo avatar Aug 13 '21 07:08 lolodomo

Any news on this? is it there any way to use sliders to choose colors in sitemap in the meanwhile?

Shituation avatar Oct 13 '22 17:10 Shituation

Noone proposed a contribution for that. And this is probably out of my main skills. HSB sliders like in MainUI would be better and more reliable. I am almost convinced that the source of the problem is the HSB <-> RGB conversion done with the current color picker. .

lolodomo avatar Oct 14 '22 11:10 lolodomo

Hi @lolodomo I came across this issue during my work on the Philips Hue CLIP 2 PR and I am hoping you may be able to give me some advice concerning the 'color' channel for Hue CLIP 2.

In Hue CLIP 2 the lamp state parameters are represented by four JSON elements as follows..

image

.. where ..

  • The 'OnOff' element is a boolean.
  • The 'Dimmer' element is the brightness 0..100
  • The 'ColorTemperature' element is the colour temperature in 'Mirek' (reciprocal Kelvin) units.
  • The 'ColorXy' element is the colour hue as an XY pair on a colour coordinate plane.

My question specifically concerns the Hue full colour lights.

As you know, in OH we use the following channel types, state classes, and UI controls as follows..

  • Channel type 'Color' <=> state class 'HSBType' <=> UI control 'Colorpicker'
  • Channel type 'Dimmer' <=> state class 'Number:Dimensionless' <=> UI control 'Slider'

This does not match at all well with the Hue CLIP 2 JSON model, since the OH channel type 'Color' (HSBType) is a hybrid that combines the JSON 'Dimmer' and 'ColorXy' elements into a single state. And furthermore, it makes sense from a user perspective, to map the 'Dimmer' JSON element (also/separately) to an OH channel type 'Dimmer'; so we risk to have two OH channels 'cross-interfering' with the state of the lamps; i.e. OH 'Color' reads/writes both JSON 'Dimmer' and JSON 'ColorXy' elements, and/or OH 'Dimmer` reads/writes JSON 'Dimmer' element only..

=> WDYT ?

PS not to mention the further complication that, as this issue states, the BasicUI Colorpicker widget is apparently broken.

andrewfg avatar Nov 14 '22 14:11 andrewfg

If I am not wrong, we define only one channel "Color" which can then be used to adjust color and brigthness through items linked to that unique channel. HSB is I believe the reference color system we use in openHAB. So I assume you will have to implement conversions. Your problem is certainly not specific to Basic UI, I assume you will encounter it in MainUI too. This is certainly something to discuss in your new hue PR.

lolodomo avatar Nov 14 '22 23:11 lolodomo

@lolodomo many thanks for the feedback.

For the Hue CLIP 2 implementation I am inclined to support TWO channels - namely Color and Dimmer. The reason is that one could then combine the dimmer aspects of multiple colour, ambient, and monochrome lights into a single OH group Item so that all lamps in the group can be dimmed with one command. One would then (also) have a Color control for the subset of lights with colour support for changing the hue.

Anyway I will play around with it on my operative system for a while, and let you know the outcome.

Note: I will probably also have to push a fix for this Basic UI issue, since my operative system is based on BasicUI sitemaps...

andrewfg avatar Nov 15 '22 16:11 andrewfg

^ I looked at the UI code, and I think the web stuff is way beyond my capabilities. @lolodomo you mentioned that your suspicion is due to the conversion RGB <=> HSB presumably in a java file (??); so as I can definitely do java, could you please point me towards what files I should look at? And I will do the rest..

andrewfg avatar Nov 15 '22 19:11 andrewfg

No, it is in the JavaScript file.

lolodomo avatar Nov 15 '22 20:11 lolodomo

in the JavaScript file.

What module is it in?

andrewfg avatar Nov 15 '22 20:11 andrewfg

What module is it in?

There is only one: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/web-src/smarthome.js

In case the color widget used by MainUI works well, one idea would be to switch to it in Basic UI.

lolodomo avatar Nov 16 '22 08:11 lolodomo

@lolodomo many thanks for the link. I made the following findings..

  1. I tested the RGB <=> HSB conversion code in the JavaScript and it produces the same results as the equivalent code in our 'HSBType' java class (allowing for integer rounding effects). So I think that eliminates your suspicion about the cause of the bug.
  2. I ran the JavaScript in the Chrome browser debugger and I can see that the problem occurs in the 'onSliderFinish()' function in the java script, and specifically the call in line 1227 (link below) to 'debounceProxy().finish()'. The code comments say "some browsers fire onchange while the slider handle is being moved. This is incorrect, according to the specs, but it's impossible to detect, so DebounceProxy is used"

https://github.com/openhab/openhab-webui/blob/587994b7ff52eac2d351a3bccd2aa0a42257f833/bundles/org.openhab.ui.basic/web-src/smarthome.js#L1227

Potential Bug Fix: perhaps modern browsers are no longer among the aforementioned 'some browsers' (??) so one potential fix for the OH bug could simply be to remove the lines marked REMOVE below that reference the 'debounceProxy' ..

=> WDYT ??

		// Some browsers fire onchange while the slider handle is being moved.
		// This is incorrect, according to the specs, but it's impossible to detect,
		// so DebounceProxy is used
		function onSliderChange() {
			_t.hsvValue.v = _t.slider.value / 100;
			_t.debounceProxy.call(); <== REMOVE
		}

		function onSliderFinish() {
			_t.hsvValue.v = _t.slider.value / 100;

			_t.debounceProxy.call(); <== REMOVE
			_t.debounceProxy.finish(); <== REMOVE
		}

andrewfg avatar Nov 16 '22 16:11 andrewfg

^ @lolodomo I did some deeper debugging in Chrome, and actually I think my hypothesis above is wrong; and yours was/is indeed correct. My new hypothesis is that some parts of the HSB / RGB code takes and produces floats, whereas other parts are based on short integer. And the java script was never able to handle that..

andrewfg avatar Nov 16 '22 19:11 andrewfg

Maybe the color widget used in MainUI is not doing any conversion to/from RGB ?

lolodomo avatar Nov 16 '22 20:11 lolodomo

It doesn't. The color picker in Framework7 accepts HSB values (among others) in several formats and performs conversions automatically. https://v5.framework7.io/docs/color-picker#color-picker-value

It could be worthwhile to study how that works (it probably uses its own util functions): https://v5.framework7.io/docs/utils#colorhextorgb

ghys avatar Nov 16 '22 20:11 ghys

Okay .. after about one day of debugging .. I think I may have figured out the cause of the problem..

When the color picker control refreshes itself, the JavaScript method extractValueFromLabel() gets called to extract the item's current state from the state presentation part of the item label. And that value is then passed to the setValue() method which assumes that the state presentation value is a comma separated HSB triplet like [11, 22, 33] and tries to create an HSB from that triplet. But the fact is that the item label does NOT contain an triplet (e.g. "Kitchen Bay Light Colour [66]"), which means that setValue() creates an invalid HSB. Normally the refresh code should then modify that HSB with the new brightness slider position value, and apply that new value to the color picker control state. However since the HSB is already invalid, it cannot be modified, and this forces the brightness slider to go to its default 50% position.

So I think in fact that the problem is not in the Basic UI JavaScript at all. But rather somewhere in the OH Core code relating to the formatting of the state presentation part of the item label for items of type color. => Can somebody please kindly check this for me?

andrewfg avatar Nov 17 '22 15:11 andrewfg

The initial value is set there: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/src/main/java/org/openhab/ui/basic/internal/render/ColorpickerRenderer.java#L69

lolodomo avatar Nov 17 '22 16:11 lolodomo

^ Hmm.

It seems that the bug only occurs if the item is created with a 'hard' state presentation part in its label..

  1. label "Kitchen Bay Light Colour" - the bug does NOT occur
  2. label "Kitchen Bay Light Colour [%s]" - the bug occurs

The reason is that this.updateWidget() (see below) calls extractValueFromLabel() which can return either null (case 1.) or some other (bad) value (case 2.) and that in turn causes widget.setValue() to use either the real proper state value or the other bad value, and in the latter case the bad value creates a bad HSB which makes the slider fail to its default 50% position.

this.updateWidget = function(widget, update) {
    var
        value = this.extractValueFromLabel(update.label), <= IF LABEL HAS A STATE PRESENTATION PART
..
    if (value === null) {
        value = update.state;
    }
    widget.setValue(smarthome.UI.escapeHtml(value), update.state, update.visibility); <= THEN SET_VALUE FAILS !!

https://github.com/openhab/openhab-webui/blob/f1db954f76743328ca790e8586b25747497d4cfe/bundles/org.openhab.ui.basic/web-src/smarthome.js#L1950

I am not sure how to fix this.


EDIT: but I think that at least the OH core should ensure that if the label is actually something like "Kitchen Bay Light Colour [%s]" then the %s should be properly populated with the item's actual HSB value e.g. [11,22,33] (or something like that) rather than merely a single number. Or ??

andrewfg avatar Nov 17 '22 19:11 andrewfg

Did you see #1140 ?

What value is shown by the sitemap when you use %s? Does BasicUI even shows a value?

lolodomo avatar Nov 18 '22 19:11 lolodomo

Did you see https://github.com/openhab/openhab-webui/issues/1140 ?

Not. Yet .. :)

What value is shown by the sitemap when you use %s ?

When I set the label with [%s] in the sitemap (code as follows) it displays as in the image underneath. It looks exactly the same when I set the label with [%s] in the Item definition.

Frame label="Kitchen" icon="light" {
    Colorpicker item=Kitchen_Bay_Light_Colour label="Aardvark [%s]"
    Text item=Kitchen_Bay_Light_Colour label="Alligator [%s]"
}

image

I would expect "hue,saturation,brightness".

Indeed that is what I expected. But as you can see the Colorpicker shows nothing; although the Text does show the brightness value. I think this could be because public class ColorItem extends DimmerItem .. but not very well ???

This would be, I think, a different issue.

I am not sure if it is a different issue, or in fact the ultimate cause of the original issue? It is certainly wrong that the state only shows "brightness" rather than "hue,saturation,brightness". And it is certainly true that if you create an HSB from "brightness" in the JavaScript, this creates a NaN HSB which causes the brightness slider to snap to 50%. And certainly removing the [%s] from the label eliminates, at least for me, the problem of the color picker snapping to 50%..

andrewfg avatar Nov 18 '22 19:11 andrewfg

Looking at BasicUI code, that makes sense that no value is displayed, this is not a surprise at all. Nothing is done for that in the snippet.

lolodomo avatar Nov 18 '22 20:11 lolodomo

Ah ok, you are using a text sitemap element to display a value.

lolodomo avatar Nov 18 '22 20:11 lolodomo

^ I think the reason why %s returns "brightness" instead of "hue,saturation,brightness" is as follows..

  • HSBType extends (via PercentType) from DecimalType
  • the HSBType.brightness field is therefore in fact the root DecimalType.value field
  • the HSBType.hue and HSBType.saturation fields are by contrast added locally in HSBType
  • so when HSBType renders its state, it calls its parent DecimalType to render its state, which renders just the value (aka the brightness field)

andrewfg avatar Nov 18 '22 20:11 andrewfg

I am afraid you are mixing up different unrelated things. For your wish to set a pattern for color, it may be because there is no format method in HSBType overriding the one in DecimalType? Edit: I think I can add this method with a test unit.

lolodomo avatar Nov 18 '22 21:11 lolodomo

^ Sorry if I am mixing things. And I have no wishes. I have just been reporting my findings concerning this bug. My conclusion is that if the Item has %s in its label, the bug occurs. But if you remove the %s then the bug goes away (at least for me). That is all..

andrewfg avatar Nov 18 '22 22:11 andrewfg

@Trickx : are you using pattern %s on your item or sitemap widget for your color item?

lolodomo avatar Nov 18 '22 22:11 lolodomo

My conclusion is that if the Item has %s in its label, the bug occurs. But if you remove the %s then the bug goes away (at least for me). That is all..

Ok, we can add the support of pattern in label for color items. If it can help... But as I never used any pattern for colour, I can tell you that the colorpicker has a bug not related to that, see my other issue.

lolodomo avatar Nov 18 '22 22:11 lolodomo

@andrewfg : can you provide a scenario to reproduce your bug without involving MQTT? Then I could reproduce it.

lolodomo avatar Nov 18 '22 22:11 lolodomo

can you provide a scenario to reproduce your bug without involving MQTT?

I found it during testing of my PR for implementing CLIP 2 api support in the Hue binding. And therefore I am pretty sure that it is also applicable to the Hue CLIP 1 scenario. I am currently running the CLIP 2 on my operative system, but let me revert to CLIP 1 tomorrow, and I will post a test case for you based on that..

andrewfg avatar Nov 18 '22 22:11 andrewfg

When opening the page, the color state is not taken from the label. Maybe the problem is when there is an update while the page is already opened? In that case, it could be either the content of the sitemap SSE event or more probably the bad handling of this event by the JavaScript (which must consider the state and not the state in the label - both are provided in the SSE event). I can check that in the JavaScript code.

lolodomo avatar Nov 18 '22 23:11 lolodomo