openhab / org.openhab.binding.zwave

openHAB binding for Z-Wave
Eclipse Public License 2.0
170 stars 202 forks source link

Extra 'ghost' option appearing when parameters have only a list of possible values / options in OH 3.2.0M5 #1701

Closed fpap72 closed 2 years ago

fpap72 commented 2 years ago

I'm moving here the bug issue I submitted under openhab / openhab-webui because apparently it is only related to ZWave devices:

https://github.com/openhab/openhab-webui/issues/1221

When device parameter values are a list of possible values (i.e. not any value, but only an enumerated list of values or options), there can be a 'ghost' value/option apparing before all the others. I've seen that, if a device parameter has only few options (max 3 or 4), these values appear inline immediately after the parameter and this works as expected (see image below):

image

Instead, when the options are more, a separate sub-window opens with the list of possible options plus an extra ghost option:

image

You can see that, before 'Normally closed alarm input', there is a bullet with an empty description, which does not exist in the device.

This 'ghost' option can be selected, and it is not clear what happens in this case:

image

After discussing with Yannick, it seems that only ZWave devices are affected by this problem.

It seems that the options are correctly defined in opensmarthouse.org (this device is the Fibaro Smart Implant) so there should be some problem between the ZWave database and the UI:

image

cdjackson commented 2 years ago

Sorry - I’m not sure I follow this. Please can you highlight on the image what you think is wrong. Please can you tell me exactly what the device is so we can check the database.

fpap72 commented 2 years ago

The extra bullet at the beginning of the list of options (yellow arrow) should not be there. The device is the Fibaro Smart Implant (parameters 20 or 21).

image

cdjackson commented 2 years ago

The extra bullet at the beginning of the list of options (yellow arrow) should not be there.

I'm a little confused. Why shouldn't it be there? If it needs to be removed, then it just needs to be removed in the database, but as far as I can tell, it is correct - there should be a normally open and normally closed option according to the manual.

fpap72 commented 2 years ago

In the manual you have 6 values for parameters 20 and 21 (values from 0 to 5 included), instead here you have 7 bullets, that is, the 6 bullets for the 6 allowed values plus an extra bullet without any description appearing at the beginning of the list. Below a snapshot where this extra bullet is selected (not sure what happens when this empty bullet is selected and what value is passed to the parameter in this case):

image

cdjackson commented 2 years ago

Ah - in the above image you pointed at the first line and it was not blank like in this line.

Anyway, this doesn't seem to be an issue with the binding as the binding only defines "real" values as can be seen below -:

image

The binding doesn't read these files - this is read by the core and provided directly to the UI. Alternatively, this might just be a problem with your version of the binding - I'm not sure what you are using as you've not mentioned this - maybe it was a problem that has already been fixed.

fpap72 commented 2 years ago

As written in the title, I was using version 3.2.0M5 when I discovered this issue. But today I upgraded to version 3.2.0.RC1 and the problem is still there.

I don't know where the problem comes from, if from the binding, the UI or somewhere in between. In this post:

https://github.com/openhab/openhab-webui/issues/1221

@ghys says 'The empty option appears when a parameter is not required so that it can be unset.' so maybe the binding is not telling the UI that the parameter is required and cannot be unset (as far as I know, ZWave device parameters are always required and must always be assigned a value). Maybe this property ('the parameter is required') is false by default, and the binding must explicitly set it to true. I'm just guessing of course, since I don't know the details of the implementation.

cdjackson commented 2 years ago

'The empty option appears when a parameter is not required so that it can be unset.'

Well, I don't know who adds that - it's not the database, and it's not the binding. The binding doesn't (and can't) manipulate these configurations - it's done by the core. It sounds like the core is adding this to allow the configuration to be removed and I've no idea why you would want to remove this. The parameter is always there in the device - so removing it in the UI, or in OH's view of the world is NOT a good idea.

@ghys - do you know what is adding this and why? It should not be possible to remove the parameter so this really doesn't make sense to me.

ghys commented 2 years ago

The UI adds the empty option because the parameter doesn't have the required flag set to true, therefore even if limitToOptions is set, you should be able not to set any value. If both required and limitToOptions are set, then the empty option won't be added.

cdjackson commented 2 years ago

Ok, so this problem is a UI issue. I don't agree with the statement that you should be able to set this to "not any value" (by which I think you mean you should be able to remove the setting?). From the device perspective, it is impossible to remove the setting - it MUST be set to something so there shouldn't be an option to remove it IMHO.

If both required and limitToOptions are set, then the empty option won't be added.

This causes other problems though if we set required. Since this configuration is a device configuration, users who define their things with text files will need to set every parameter - even though it doesn't do anything (since it's not possible to set zwave device configuration in the things files).

Personally, I think that the UI should not add extra options (blank or otherwise). The definition that is shown in the UI should come from the configuration definition.

I'd suggest to move this to the webui issue tracker since this is clearly not an issue in the zwave binding.

ghys commented 2 years ago

Ok so if the parameter is not actually required, then the UI adding a way to unset it is not in the wrong either, and there's no problem at all.

fpap72 commented 2 years ago

Ok so if the parameter is not actually required, then the UI adding a way to unset it is not in the wrong either, and there's no problem at all.

ZWave parameters are always required. All parameters must be assigned a value.

IMHO, if the UI requires this required flag to be explicitly set to true, it is sufficient for the ZWave binding to add this flag to all the parameters. Or am I missing something?

cdjackson commented 2 years ago

Well, I think the issue is the interpretation of "required". Here - "required" means that it must be provided by the user before the think will be initialised by the core. These configs are not really "required" since they are available in the device and therefore can't be removed, so the UI should not provide a way to remove them. If however I set them to "required", then user will need to define these or the system doesn't work at all and that will be a problem.

So, IMHO the problem is we are trying to use the required flag for two different things.

cdjackson commented 2 years ago

ZWave parameters are always required. All parameters must be assigned a value.

If we do this, it will be a MAJOR breaking change as anyone who uses thing file to define their configuration will NEED to add all required configuration. if that is "all" configuration, then this will be a big big problem as many devices have 25 or more parameters.

Or am I missing something?

IMHO, yes. I really think we are trying to use this required flag for two different purposes. If the consensus is that I must change the binding, then I will do so, but I really think personally it is wrong.

ghys commented 2 years ago

My view of "required" is, if it's not required, then it must be able to be unset - because either it's not applicable in this context, or if it is, there is a default behavior in place. If I understand right, your view is "either it's not applicable but if it is, it must be set".

The problem is that those config parameters are used all over openHAB, so if I remove the ability to unset a non-required parameter, you can be sure I will get another issue for that.

cdjackson commented 2 years ago

I can see your point - don't get me wrong. But the problem is we are using one parameter (ie the required flag) for two purposes. Since the thing will not be initialised if a user doesn't provide this.

So what is your suggestion - that I MUST set the required flag for all ZWave parameters as stated above? If so, I'll do this and we just have to live with the consequences.

ghys commented 2 years ago

So what is your suggestion - that I MUST set the required flag for all ZWave parameters as stated above? If so, I'll do this and we just have to live with the consequences.

I don't believe you should make that change as it appears clearly to have adverse side effects.

Since the thing will not be initialised if a user doesn't provide this.

In those cases, maybe the thing could stay in OFFLINE status with a CONFIGURATION_ERROR reason (and a sufficiently clear description message "The parameter XX must be set" or similar)?

fpap72 commented 2 years ago

So what is your suggestion - that I MUST set the required flag for all ZWave parameters as stated above? If so, I'll do this and we just have to live with the consequences.

There is something I don't understand... As per my understanding, 'required' means that a parameter must be assigned a value. This value can also be its default value. When you include a new device in the ZWave network, it will be included with all parameters set to their default values, and this should cause no problems. I mean, from the user point of view there should be no consequences: you include your device and modify only the parameters that need to be modified, the others will retain their default value. (Here with 'user' I mean the one that is using and configuring the ZWave device)

cdjackson commented 2 years ago

In those cases, maybe the thing could stay in OFFLINE status

Currently it will be uninitialised. I'm not sure exactly - it might already be something like CONFIGURATION_PENDING. The issue though is that if I set all ZWave parameters to "required" as is stated above, and many have dozens of parameters, then this becomes a huge amount of work for people who use thing file to configure their system as they then need to define everything.

I'm personally not sure why you would ever want to remove ANY configuration to be honest. If there is a configuration value, then it MUST have a default (I forget if the default is required in the config definition, but if not there, the binding will make an assumed default). So removing hte configuration is exactly the same as setting the configuration to the default - so why do we need the ability to remove it at all?

cdjackson commented 2 years ago

As per my understanding, 'required' means that a parameter must be assigned a value. This value can also be its default value.

Sure - I don't argue about this. However if a config parameter is defined as requiredthen is MUST be set to a value BEFORE the core will create the thing. So the binding doesn't even get a thing handler created until this is done. This means that the user must define all this configuration BEFORE their system will even start.

This might be ok for UI users, but for users who define things with a text file, this will be a lot of work.

When you include a new device in the ZWave network, it will be included with all parameters set to their default values,

Well, now you are mixing things up. The device will have default values - yes - but these will not necessarily be defined in the OH configuration. It is the OH view of the configuration that we are discussing here - the configuration that is stored in the "thing" definition within OH.

I mean, from the user point of view there should be no consequences

So, I repeat - if the configuration is not defined in the OH thing config, then OH will not create a thing. At least this is how it used to work - maybe it has changed - do you know if this is the case?

fpap72 commented 2 years ago

Thank you @cdjackson, I was not aware of these details...

I rethought everything from the beginning and have a couple of more questions. These questions are mainly directed to @ghys because they are related to how the UI deals with the 'unrequired' parameters:

  1. The blank option appears only when there are 'many' options (in this case, a frame or sub-window opens, as shown in the previous pictures, and the blank option appears). When the options are just 2 or 3, these options are listed immediately after the parameter and in this case the blank option does not appear:

image

Why? Parameter 24 is defined exacly like parameter 20 (the only difference is the smaller number of options), so the blank option should appear also in this case. It seems that there is an asymmetric behavior of the UI when dealing with the 'unrequired' parameters.

  1. Why the description of this extra option is blank? I would have expected something like 'Do not use' or 'Unapplicable', instead there is absolutely nothing, and this is misleading.
ghys commented 2 years ago

Why? Parameter 24 is defined exacly like parameter 20 (the only difference is the smaller number of options), so the blank option should appear also in this case.

It does normally appear, as a X icon like this:

image

However I suspect there's a bug there - when it's set to "0" (either manually or via the default) it believes the option is not set and therefore doesn't display the "unset" button.

Why the description of this extra option is blank? I would have expected something like 'Do not use' or 'Unapplicable', instead there is absolutely nothing, and this is misleading.

Because it's a "blank" option, it makes perfect sense outside this specific Z-Wave use case, maybe it could be "(none)" or "(unset)", but certainly not "do not use".

cdjackson commented 2 years ago

it makes perfect sense outside this specific Z-Wave use case,

I don't believe this is just a zwave specific issue. I think this is an issue whenever device configuration is used - and really I think it is an issue always. Certainly, Zigbee will be exactly the same - there is absolutely no reason to remove configuration in my mind.

I come back to the point I made above that no-one commented on -:

If there is a configuration value, then it MUST have a default (I forget if the default is required in the config definition, but if not there, the binding will make an assumed default). So removing hte configuration is exactly the same as setting the configuration to the default - so why do we need the ability to remove it at all?

So why is there a way to remove configuration? The configuration option still exists - right. We didn't remove it from the binding, and presumably by "removing it" we have really just set it back to the default. As I commented above - if there's a configuration, then I would assume the binding makes an assumption on a value if it's not set (so in my mind, that is the default). By "removing" the configuration - you're just removing it from the data store - but the config is still there - as the default value.

I think "removing" configuration is incorrect and I can't see a use case personally (although I'm sure someone will find one :) ). @ghys what do you think - I just think the concept here is wrong and it should really be set to default - no a blank configuration option.

ghys commented 2 years ago

I'm not sure I agree with this.

So removing hte configuration is exactly the same as setting the configuration to the default - so why do we need the ability to remove it at all?

From a functional point of view, in many cases, that would be true, default equals not set; technically though, it's not. Whether it makes a difference depends on the implementation of the component using the configuration, some might react differently. So there too, it depends on the definition of "default" by the developer of that component, which may vary, we can't really be sure. Some may consider the default to be a suggestion only when it makes sense to configure the parameter in the first place. Add to that the fact that most parameters have no default (it's not required to specify one).

So all in all, I am uncomfortable not giving a way to remove a parameter with required: false (which to me means "optional") from the configuration, especially if it has no default. Parameters that are both optional and also have a default are strange things actually... that might mean they are not "really" optional after all!

When you're presented with the parameter in the UI and no initial configuration, then the parameter will only be set if you act on the control - even if it has a default: that value is just there to display the initial state. If you never touch the control, nothing will be added to the configuration. If you do make a choice, configure the parameter, and later change your mind and want to go back to "never been configured" for this parameter, then IMHO you should be able to do so - unless of course the parameter is required: true.

cdjackson commented 2 years ago

Ok, I guess I will not convince you here, but for me, there MUST always be a default. I've tried to explain this, and this is exactly the situation we have here with ZWave devices - the device has a default when it is included into the network - you can't remove this.

A developper specifies the defaults - they also specify the default in the implementation (ie in the Java handler) and also in the XML - I don't understand why this should be any difference.

So, I guess we have to live with the impacts of your implementation here and there seems to be no solution, so we might as well close this.

cdjackson commented 2 years ago

@ghys I also don't think this was the way it worked in OH2 - so I think this is a new concept that you have added to the UI for OH3 (and I'm not sure if it was 3.0 or newer?). As above I think it is not the right approach - defaults must apply somewhere and the developer has always defined these.

Anyway, I'll close this now as can't fix.

ghys commented 2 years ago

Go to Settings > Regional Settings, all the parameters there are optional and don't have a default. If I ever decide that I don't want to specify my country or my time zone or my location anymore, I'm glad that I have a way to remove them from the config. This is just the equivalent of commenting out or removing a line in a configuration file, it makes no sense to try to prevent that.

So, I guess we have to live with the impacts of your implementation here and there seems to be no solution

Well, I was trying to think of a satisfactory compromise solution (like no empty/remove option IF the parameter has a default even if it's not required) as I believe both POV are valid, but I must say I feel less inclined after reading this... This was not necessary. Anyways, I suppose that's the kind of reasons you created Open Smart House.

cdjackson commented 2 years ago

Appologies if I caused offence - that was not the intention and maybe was lost in translation. My point was simply that if there is a design decision, then we have to live with the implications or downsides (as is always the case).

I'm certainly happy if there's a solution - that would be appreciated.

I suppose that's the kind of reasons you created Open Smart House.

The main reason was driven by customers who had requirements that couldn't be met with OH. We needed to reduce the footprint - something that was not possible with OH, and we needed some functionality that was not possible in OH. It still follows OH in many way so I don't see the issue, and I put in a lot of hours on OH bindings so again, I don't think it's a problem is it?

cdjackson commented 2 years ago

I'm certainly happy if there's a solution - that would be appreciated.

I suppose that's the kind of reasons you created Open Smart House.

The main reason was driven by customers who had requirements that couldn't be met with OH. We needed to reduce the footprint - something that was not possible with OH, and we needed some functionality that was not possible in OH. It still follows OH in many way so I don't see the issue, and I put in a lot of hours on OH bindings so again, I don't think it's a problem is it?

cdjackson commented 2 years ago

Go to Settings > Regional Settings, all the parameters there are optional and don't have a default. If I ever decide that I don't want to specify my country or my time zone or my location anymore, I'm glad that I have a way to remove them from the config.

Not wanting to dwell on this too much, but these parameters must have a default - somewhere. Settings like language or time zone don't go away when you remove them - they get set to the system default (I guess?). Maybe the solution is if there's no default, then add your blank option to allow removal, or call it "system default", but if there is a default value defined, allow the user to select this.