-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add support for dpt 8.xxx (attempt 2) #93
Conversation
knx/dpt/formats.go
Outdated
} | ||
|
||
func packV32AsV16(i int32, resolution int32) []byte { | ||
return packV16(int16(i / resolution)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no checks in place here to ensure the value provided (i
) is within valid bounds. I.e. within math.MinInt16 * resolution ... math.MaxInt16 * resolution
.
However, the pack functions does not offer an error
as return value, and changing this would break the interface and cause ripple effects.
Side note: I have verified that the existing 7.003 and 7.004 (which are structured similarly) are indeed broken and showing the wrong values. I'm happy to fix those (and if there are more) in a later PR if we can agree this change is the way to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you separate the logic such that the following holds?
formats.go
only handles conversion between V16 wire format andint64
types_8.go
handles interpretation/production ofint64
I hope I got what you meant. I'd like to run a final test through ETS5 but won't be with my PC for another couple of hours. |
Confirmed working as ETS5. |
I'll give this another review the coming days. |
Creating a new PR (previous #91) to keep the conversation clean since I've done some research now.
There were some confusion on how to represent fields with a different resolution than 1 which is the case for 8.003, 8.004 and 8.010. My conclusion is that the unit presented in the KNX specification is indeed what ETS uses to present the values when listening on the bus.
This means that:
ms
as value of int16 times their respective resolution.%
as value of int16 times its resolution.If we want to stay accurate with ETS this means 8.003 and 8.004 needs to be stored in int32 and 8.010 in float32.
I did some test writes from ETS5. For 8.003 and 8.004 I was sending raw value of
5891
. For 8.010 I was sending raw value of98.56
. This is what ETS showed:And this is what my application showed (using this version of knx-go):
Note how 8.003 truncates 1 digit (due to resolution of 10) and 8.004 truncates 2 digits (due to resolution of 100). This is the same behaviour in this branch.
ETS seemingly present 8.010 after first converting it to a floating point number, so even if I write
98.56
it prints as98.5599977970123 %
. Similarly, this PR does the same but due to a difference in precision it's slightly different. However, when rounding to its resolution (in this case 2 digits) they will always be the same.