zwave-js-ui icon indicating copy to clipboard operation
zwave-js-ui copied to clipboard

Indicate Value min/max/step values in the UI

Open kpine opened this issue 1 year ago • 3 comments

Is your feature request related to a problem? Please describe. The UI for the Values doesn't show min / max and step restrictiosn. If you set a value outside of the range, the command will report an error.

Describe the solution you'd like Indicate min / max and step in the UI next to the Value somewhere. Or validate the value client side before allowing it to be sent (but I might want to force an invalid value for some debug purpose?). Or do this for select Values, like wake up interval.

Describe alternatives you've considered N/A

Additional context I was setting the wake up interval for a battery device (ZSE41). I usually use 7 days (604,800 seconds), most of my devices all this. I tried with a Zooz and got an error flash in the UI. When I checked the value it was set to 86400 seconds instead.

I checked the node dump and it lists a min and max value, for this device, as well as a step value of 60 seconds. 7 days is out of range (so would non-multiples of 60). It would be preferable to know this prior to setting an invalid value.

    {
      "id": "60-132-0-wakeUpInterval",
      "nodeId": 60,
      "toUpdate": false,
      "commandClass": 132,
      "commandClassName": "Wake Up",
      "endpoint": 0,
      "property": "wakeUpInterval",
      "propertyName": "wakeUpInterval",
      "type": "number",
      "readable": false,
      "writeable": true,
      "label": "wakeUpInterval (property)",
      "default": 43200,
      "stateless": false,
      "commandClassVersion": 2,
      "min": 3600,
      "max": 86400,
      "step": 60,
      "list": false,
      "value": 86400,
      "lastUpdate": 1719774741980,
      "newValue": 604800
    },

kpine avatar Jun 30 '24 19:06 kpine

@kpine I was going to implement this but I noticed that some values already report this in the hint. It could be a bit redundant in such cases.

Suggestions? cc @AlCalzone

2024-07-02_18-02

robertsLando avatar Jul 02 '24 16:07 robertsLando

Those hints you mention exist only for configuration parameters, and they should only exist in cases that cannot be modeled with min/max/step, e.g. in the case here with one value outside the "normal" range. Respecting the min/max/step where they exist is a good idea in general.

AlCalzone avatar Jul 02 '24 16:07 AlCalzone

Respecting the min/max/step where they exist is a good idea in general.

I already respect them on UI the problem here is that actually I don't show them. My idea was to add it to the hint but then I saw some hints already have them so I was wondering what's the best choice here

Ref: https://github.com/zwave-js/zwave-js-ui/blob/40080a17ee730611bd3c89e04e10ad82bac60eec/src/components/ValueId.vue#L55-L58

What I only miss here is step that I wasn't aware it existed. I can add it but the question remains open for min/max value, should I show them? If yes where?

robertsLando avatar Jul 03 '24 08:07 robertsLando

Fixed by https://github.com/zwave-js/zwave-js-ui/commit/1107718f674e222aa1b4efdfe4a397b68c37341d

robertsLando avatar Nov 04 '24 09:11 robertsLando

I have finally upgraded to 9.33.0 from 9.26.0 and I am not seeing the min/max labels on the value for Wake Up Interval. It doesn't look like the referenced commit addressed that. I do see the step values working correctly though. Were you expecting the the min/max labels to be visible for Wake Up Interval?

Also, there's some inconsistencies with how the number controls work with the min/max.

This node does not respect either the min and max via the controls. I can use the number control to go below or beyond 4200.

Image

    {
      "id": "11-132-0-wakeUpInterval",
      "nodeId": 11,
      "toUpdate": false,
      "commandClass": 132,
      "commandClassName": "Wake Up",
      "endpoint": 0,
      "property": "wakeUpInterval",
      "propertyName": "wakeUpInterval",
      "type": "number",
      "readable": false,
      "writeable": true,
      "label": "wakeUpInterval (property)",
      "default": 4200,
      "stateless": false,
      "commandClassVersion": 2,
      "min": 4200,
      "max": 4200,
      "step": 0,
      "list": false,
      "value": 4200,
      "lastUpdate": 1740876856107,
      "newValue": 4200
    },

This node respects the max, but not min. I can not go past 86400 with the number control, but I can go down to 300 for the minimum.

Image

    {
      "id": "60-132-0-wakeUpInterval",
      "nodeId": 60,
      "toUpdate": false,
      "commandClass": 132,
      "commandClassName": "Wake Up",
      "endpoint": 0,
      "property": "wakeUpInterval",
      "propertyName": "wakeUpInterval",
      "type": "number",
      "readable": false,
      "writeable": true,
      "label": "wakeUpInterval (property)",
      "default": 43200,
      "stateless": false,
      "commandClassVersion": 2,
      "min": 3600,
      "max": 86400,
      "step": 60,
      "list": false,
      "value": 86400,
      "lastUpdate": 1742194134815,
      "newValue": 86400
    },

kpine avatar Mar 17 '25 21:03 kpine

@kpine when min = max I have a rule that disables them:

https://github.com/zwave-js/zwave-js-ui/commit/1107718f674e222aa1b4efdfe4a397b68c37341d#diff-eb14af88120cbacc8ef66cd5b2b6e2144cded6195f07987d9e8185cf24f634fcR55-R59

https://github.com/zwave-js/zwave-js-ui/commit/1107718f674e222aa1b4efdfe4a397b68c37341d#diff-eb14af88120cbacc8ef66cd5b2b6e2144cded6195f07987d9e8185cf24f634fcR58

In the second case I should check but it should work, tried also with a different browser? Could you try to inspect the element and see if the min/max are set correctly?

robertsLando avatar Mar 18 '25 09:03 robertsLando

Ok, you can ignore the second case, I copied the wrong Debug Info from a similar device, 300 is the correct value. Sorry about that.

I think there are still two issues:

  1. When min == max, you can use the number control to go out of range
  2. When min != max, you cannot use the number control to go out of range (correct), but you can input any value you want and ZUI and the Driver will be happy to set it (not sure if that's a problem or not)

Showing a hint about min and max would be useful here? Here's a mock:

Image

I don't know if it's the job of the UI to validate the value or not, but showing the min/max would be helpful.

Here's my use case. The battery sensor default value of 43,200 is 12 hours. I'd rather have a 1 week wake up interval. That means I need to input value 604800. I'm not going to use the up/down control to advance 40,000 seconds and then see it's the max, I'm just going to type in the value 6048000 exactly as-is. I don't see a min/max in the dialog so I think it's OK, or there's no validation of the value as I have typed it in. When I send the command nothing tells me it is invalid. When the device wakes up, it'll just reject the setting and eventually (could be 12 hours later) the value will revert without any indication (except for the "error flash" I mentioned originally). If I know enough I can expect the debug info and see the problem (as I did).

kpine avatar Mar 18 '25 15:03 kpine

@kpine I decided for now to add the min/max indications to the help text, will see if adding a more strict validation but I'm a bit worried about that as in case for some reason the config is wrong I would still allow users to set the value they prefer and let driver throw the error instead

robertsLando avatar Mar 27 '25 15:03 robertsLando

That's fine, textual indication was really all I was asking for, thanks!

kpine avatar Mar 27 '25 16:03 kpine

in case for some reason the config is wrong

This isn't only about config parameters though. The above example is for the Wakeup CC, where the device itself reports the supported steps.

AlCalzone avatar Mar 27 '25 19:03 AlCalzone