Fix float32 negative value/ add validation for range limit #7488
Conversation
garciaolais
changed the title
| { name = "TankLevel", byte_order = "AB", data_type = "INT16", scale=1.0, address = [0]}, | ||
| { name = "TankPH", byte_order = "AB", data_type = "INT16", scale=1.0, address = [1]}, | ||
| { name = "Pump1-Speed", byte_order = "ABCD", data_type = "INT32", scale=1.0, address = [3,4]}, | ||
| ] |
There was a problem hiding this comment.
Make sure to preserve 2-space indention of this text. I think we should actually update the README to match the sample configuration, since the sample config is using the preferred Telegraf style for field names.
| @@ -513,7 +522,7 @@ func convertDataType(t fieldContainer, bytes []byte) interface{} { | |||
| return scaleFloat32(t.Scale, f32) | |||
| case "FLOAT32": | |||
| if len(bytes) == 2 { | |||
| e16 := convertEndianness16(t.ByteOrder, bytes) | |||
| e16 := int16(convertEndianness16(t.ByteOrder, bytes)) | |||
There was a problem hiding this comment.
I realize that I'm not sure what the FLOAT32 data_type is supposed to be. It appears that before this PR it was a 2, 4, or 8 byte unsigned integer that is converted to a float64 and scaled. After this pull request it appears that it is two's complement signed integer.
If I'm understanding correctly, the issue I see is that the value could be either of these encodings and there is no way as a user to select which it is. If we apply this change, large positive values will begin being reported as negative values.
Let me know if my understanding is accurate, and we can try to brainstorm a solution.
|
The proposed change modifies the conversion of 16 bit values only, However, for 32 bit and 64 bit values the behaviour remains the opposite. This seems to me unexpected behaviour. It would be better if all cases are handled equally. |
|
@garciaolais: |
|
Hi @srebhan I need to verify the new terms, maybe this change its not valid anymore. |
|
@garciaolais: Sure. Just compared your code to what is in master and it looks very similar. Just let me know on what you think! |
|
@srebhan: in a few days I Will have free time, I hope check this issue very soon 👍 |
|
@garciaolais any chance you find some time to look at this? |
|
Hi @srebhan , Tomorrow I have a free weekend, I will working in this |
Required for all PRs:
Signed CLA.
Associated README.md updated.
Has appropriate unit tests.
Fix 7317
Add test for negative float32 values
Updated Readme inside code