search

dresden-elektronik / deconz-rest-plugin

deCONZ REST-API plugin to control ZigBee devices
BSD 3-Clause "New" or "Revised" License
1.9k stars 502 forks source link

remove superfluous eval conditions #7984

Open bluemoehre opened 1 week ago

bluemoehre commented 1 week ago

Signed s16 integer supports values from -32768 to 32767, so Attr.val != 32768 is superfluous. Typically the lowest value, -32768 is used to indicate an invalid value.

github-actions[bot] commented 1 week ago

Hey @bluemoehre, thanks for your pull request!

[!TIP] Modified bundles can be downloaded here. Relative expire date

DDB changes

Modified

Validation

[!TIP] Everything is fine !

:clock430: Updated for commit ffdb8f39abc7b05bc7a53eb3f1b6cfe3c2c7fc83

bluemoehre commented 1 week ago

@ebaauw FYI

SwoopX commented 1 week ago

While this is technically correct, there's devices out there depending on this check to discard blips. Given the fact that this check doesn't hurt and there is a necessity, we should keep this untouched.

bluemoehre commented 1 week ago

While this is technically correct, there's devices out there depending on this check to discard blips.

Do we know which ones?

Given the fact that this check doesn't hurt and there is a necessity, we should keep this untouched.

That's bad practice. Like @ebaauw asked me to change this (at least for the generic one), and I found the same "error" in other files, it may happen again that people reading the code want to fix that, because it looks like a bug. If only a few people have that hidden knowledge, it's a risk. \ I would recommend to make the generic one correct and then take care about the the blippy bloppy devices. We should also leave a comment in the code or add a test, so everyone can understand why it's like that and prevent accidentally breaking something.\ What do you think?

SwoopX commented 1 week ago

I'm afraid I cannot follow you to a certain extend. Where is the bad practice? We're discussing a valid boundaries check here being there for a reason. This is neither a bug (which is by definition a programatical misbehavior), nor an error (as nothing is working falsely) so there is nothing to fix in my view.

Leaving the code as is has 0 risk, changing the code as proposed introduces an unneccessary risk of breaking functionality of an unknown number of devices, hence I prefer avoiding that risk.

Regarding your suggesting to add a respective comment in the code, I'd fully buy in. ~Although this possibility is not given yet (I find the "description" key not really suitable for this purpose and placement is restricted), we should introduce a "comment" key to allow for some (important) documentation within a DDF at the place specifically required.~ "comment" key is available and usable, however, not displayed in DDF editor.

ebaauw commented 1 week ago

It's technically not possible for an s16 to report a value >= 32768; it simply doesn't fit the 16 bits. Any device depending on this check would have to use a different data type. That alone is reason enough to override the default item definition in the device's DDF.

bluemoehre commented 1 week ago

Where is the bad practice?

"Given the fact that this check doesn't hurt" [...] "we should keep this untouched"

It's bad practice to keep a workaround in a generic file just because it doesn't hurt. Especially if only a few devices are to be fixed with it. The generic files should kept clean all the time, since the amount of devices will continuously keep growing. (Imagine having in there all workarounds for all types of devices - that won't work out.)

Also we should not keep this untouched, since the workaround is not documented at all.

Undocumented workarounds will lead to copies pasted somewhere. If you need to clean this up in the future, it will take a lot of effort.

We're discussing a valid boundaries check

It is not, as @ebaauw already described. 1 Bit is used for the sign, 15 Bits remaining for the magnitude: 0111 1111 1111 1111 = 32,767

According to the specification the value always must also be an s16: \ ZigBee Cluster Library

I checked all the devices that were changed by this PR and their commits to see if there was any information about special treatment required. If there really was a problem, it was unfortunately not documented. But you may check yourself:

That's all I can do at this point to convince everyone that we are doing the best possible here. \ Please let me know how we continue with this PR, because it's affecting the device request #7959.