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

Indicate Value min/max/step values in the UI #3789

Closed
kpine opened this issue Jun 30, 2024 · 10 comments
Closed

Indicate Value min/max/step values in the UI #3789

kpine opened this issue Jun 30, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@kpine
Copy link
Contributor

kpine commented Jun 30, 2024

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 kpine added the enhancement New feature or request label Jun 30, 2024
@robertsLando
Copy link
Member

robertsLando commented Jul 2, 2024

@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

@AlCalzone
Copy link
Member

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.

@robertsLando
Copy link
Member

robertsLando commented Jul 3, 2024

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:

:min="value.min != value.max ? value.min : null"
:step="1"
persistent-hint
:max="value.min != value.max ? value.max : null"

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
Copy link
Member

Fixed by 1107718

@kpine
Copy link
Contributor Author

kpine commented Mar 17, 2025

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
    },

@robertsLando robertsLando reopened this Mar 18, 2025
@robertsLando
Copy link
Member

robertsLando commented Mar 18, 2025

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

1107718#diff-eb14af88120cbacc8ef66cd5b2b6e2144cded6195f07987d9e8185cf24f634fcR55-R59

1107718#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?

@kpine
Copy link
Contributor Author

kpine commented Mar 18, 2025

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).

@robertsLando
Copy link
Member

robertsLando commented Mar 27, 2025

@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

@kpine
Copy link
Contributor Author

kpine commented Mar 27, 2025

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

@AlCalzone
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants