search

vapourismo / knx-go

KNX clients and protocol implementation in Go
MIT License
91 stars 59 forks source link

Add support for dpt 8.xxx (attempt 2) #93

Closed pakerfeldt closed 2 months ago

pakerfeldt commented 3 months ago

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:

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 of 98.56. This is what ETS showed: image

And this is what my application showed (using this version of knx-go): image

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 as 98.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.

pakerfeldt commented 3 months ago

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.

pakerfeldt commented 3 months ago

Could you separate the logic such that the following holds?

formats.go only handles conversion between V16 wire format and int64 types_8.go handles interpretation/production of int64

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.

pakerfeldt commented 3 months ago

Confirmed working as ETS5.

vapourismo commented 3 months ago

I'll give this another review the coming days.