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

[bug] UI for a dimming duration shows "[object Object]" #185

Closed
1 of 3 tasks
LordMike opened this issue Jan 9, 2021 · 17 comments · Fixed by #230
Closed
1 of 3 tasks

[bug] UI for a dimming duration shows "[object Object]" #185

LordMike opened this issue Jan 9, 2021 · 17 comments · Fixed by #230
Assignees
Labels
bug Something isn't working

Comments

@LordMike
Copy link
Contributor

LordMike commented Jan 9, 2021

Version

Build/Run method

  • Docker
  • PKG
  • Manually built (git clone - npm install - npm run build )

zwavejs2mqtt version: v1.0.0-alpha.2

Describe the bug
A wall switch with built in dimmer (ZDB5100 Matrix) has a multilevel switch section in ZJS2M. In the UI, the duration parameter shows up as [object Object]. In the getNodes json, I see that the value of this field is:

image

The UI probably doesn't handle the value+unit combo very well. :)

To Reproduce
N/A

Expected behavior
A dropdown or selector (maybe?) for value+unit... I expect only the value is settable, so maybe just display the unit next to the input box?

Additional context
Screenshot of the UI with the JS object outputted:

image

@LordMike LordMike added the bug Something isn't working label Jan 9, 2021
@robertsLando
Copy link
Member

@LordMike Uh never saw this! I think it comes from last updates? I have durations values too but them were not showed in that way on ui. @AlCalzone Breaking change from 6.0.0?

@AlCalzone
Copy link
Member

Nothing changed in that regard. Duration is not a primitive value, but you can use toJSON() or work with its properties directly: https://github.com/zwave-js/node-zwave-js/blob/b1df89733dc088ae6084b6a97516db4736a5641b/packages/core/src/values/Duration.ts#L73-L80

@AlCalzone
Copy link
Member

Basically you need to select/edit both the value (0-127) and the unit (minutes/seconds)

@robertsLando
Copy link
Member

Humm I think I need to parse it in a different way. When I set it should I pass a Duration on the setValue too?

@AlCalzone
Copy link
Member

AlCalzone commented Jan 11, 2021

Probably, but you cannot set the duration yet. zwave-js/node-zwave-js#1321
So there's room to make this more compatible (accept objects for example)

@robertsLando
Copy link
Member

@AlCalzone Just to knnow but wouldn't a type: Duration be more appropiated instead of any?

@AlCalzone
Copy link
Member

good point. I've treated "any" more like a: You cannot be sure what comes here, better check before assuming.
For non-primitive values it could make sense to add new keys.

@robertsLando
Copy link
Member

@LordMike So keep this open until the support is available on zwave-js

@LordMike
Copy link
Contributor Author

My 0.02$ would be not to make the available types more complex than primitives. So I wouldn't add a "duration-with-minute-and-seconds" type, that supports "minutes" and "seconds", only to add one that also supports "hours" later when a device comes along supporting that.

I would do one of two things:

  • Work with a concept of "timespans", like "5s" or "10m". When you get "5 seconds" from the device, you emit "5s" (as a string). Likewise, setting it to "1h" submits "1 hour" to the device.
    • This gets even better for usability, if the device f.ex. doesn't support hours. You can convert "1h" into "60m" or "3600s" before sending it.
  • Expose this as two separate values, one as an integer and one as a list of units. It sucks due to the extra set of messages though.

@AlCalzone
Copy link
Member

AlCalzone commented Jan 11, 2021

How would you configure a value of 127 seconds with that approach?
Not trying to argue against it, just discussing the dirty details of zwave. Likewise, 128 seconds does not exist, so you have to use 2 (or three) minutes.

@LordMike
Copy link
Contributor Author

"127s" or "2m7s" .. both are identical. The tricky part (for you) is distilling that down to a value that z-wave can work with, and abiding by any limitations by the device.

You obviously can't send that as a z-wave value, in anything but seconds. If you were to send it as minutes, the best you could do was "120s" (2 minutes).. I don't imagine the message supports floats :)

@AlCalzone
Copy link
Member

I'm wondering if that is intuitive that you can set 2m7s but not 2m8s (which cannot be represented and AFAIK all durations have the same encoding).

@LordMike
Copy link
Contributor Author

If they're all in the low end or represented by seconds, why not just skip the unit and go with a "number" ?

I imagine validation comes naturally for numbers, with the min/max values.

Ooh, the 128..255 values are then minutes?

If so, the seconds can still work, but validation will be trickier. You could simply log an error and not send the message, if a user puts in 179 seconds, instead of a valid value of 180 seconds.

@AlCalzone
Copy link
Member

Ooh, the 128..255 values are then minutes?

Yep that is the tricky part. When setting a duration:
grafik

and when decoding a report:
grafik

So just using a number won't work (well it will, but it is suuuuper unintuitive). I kinda like going with a hybrid approach:

  • Keep Duration objects, but give library users a way to work with them (Use type: 'duration' for Duration valueIds node-zwave-js#1348 as a first step)
  • Add a method to serialize them as either a timespan string like you proposed or the current object format
  • Add a method to deserialize them from a timespan string or the current object format. Trying to use an invalid combination will throw.

Devs can then decide whether they need to work with timespan strings, value+unit objects or the original Duration object.

@LordMike
Copy link
Contributor Author

Will all Durations be like that?... Like all ? I would be worried that suddenly a device comes along, which operates in increments of 10s.

So one third way is simply to expose the 0..255 value. :) Additional metadata / functions could then help users in translating between timespans, Durations, or something else entirely.

Anyhoo, if it's always minutes and seconds, it's probably fine. Let's hope a Duration2 doesn't appear :D

@AlCalzone
Copy link
Member

So one third way is simply to expose the 0..255 value

I don't like exposing values which need additional interpretation. The aim of a library is to abstract away the ugly details (or at least make them intuitive to work with). I think we're on a good way with the additions to Duration.

@robertsLando
Copy link
Member

I think that Duration value is good, I just think that the type should be Duration and not any

robertsLando added a commit that referenced this issue Jan 13, 2021
robertsLando added a commit that referenced this issue Jan 18, 2021
* fix: duration type handling

Fixes #185

* fix: handle writes

* feat: allow to choose custom duration

* fix: lint errors

* fix: merge issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants