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

Fix unsigned floats #130

Merged
merged 4 commits into from
Jan 5, 2023
Merged

Conversation

tomkooij
Copy link
Contributor

@tomkooij tomkooij commented Dec 30, 2022

Fixes #123

This is a bit of a hack, as it implements a "is_signed" flag for "floats" but leaves ints and uints in place.
In reality all "itho raw bytes" are either signed/unsigned and have some divider. Ints are just floats with divider 1.

This should be refactored with IthoDeviceStatus::type (int/uints/float) removed and only the is_signed flag in place.

But this works:

Cast "signed raw bytes" to signed integer first, before casting to float and applying a divider.

Example: datatype = 0x92 (1-0-010-010)
datatype & 0x80 (bit 8) => signed value
datatype & 0x38 (bits 654) => length => 2 bytes
datatype & 0x07 (bits 321) => divider => 10^2

datatype 0x92 value 0xFC18
datatype signed, length 2, divider 100.
signed int value -1000: value -10.00

Cast "signed raw bytes" to signed integer first, before casting to float and applying a divider.

Example: datatype = 0x92 (1-0-010-010)
datatype & 0x80 (bit 8) => signed value
datatype & 0x38 (bits 654) => length => 2 bytes
datatype & 0x07 (bits 321) => divider => 2

datatype 0x92 value 0xFC18
datatype signed, length 2, divider 2.
signed int value -1000: value -10.00
@tomkooij
Copy link
Contributor Author

I don't think this PR caused the CodeFactor failures.

@tomkooij tomkooij mentioned this pull request Dec 31, 2022
7 tasks
Itho datatype (single byte)

7-654-3210
bit 7: signed (1), unsigned (0)
654: length of raw data
3210: (4 LSBs): divider

Exception 0x5B: two byte unsigned int.
@tomkooij
Copy link
Contributor Author

tomkooij commented Jan 1, 2023

I refactorted the Itho datatype handling to reflect the behaviour of the latest version of the Itho servicetool. (Maybe document the datatype format on a wiki page?)

The logic is somewhat different from the old source: https://github.com/rustyx/itho-esp/blob/main/Specs.md#message-a4-00-query-status-format. The special cases 0x0f, 0x0c and 0x6c are no longer special cases and handled correctly by this code.
(This new code also handles some other datatypes that would have been translated incorrectly, but aren't encountered (yet) in the wild).

Special case 0x5B remains. This seems to be a real "legacy" itho datatype. The itho servicetool also treats this as a special case.

@arjenhiemstra
Copy link
Owner

Is there a way around having the divider as float datatype? Comparing floats is tricky (ie. when divider = 1.0 the comparison if(divider == 1.0) is not guaranteed to result true)

@tomkooij
Copy link
Contributor Author

tomkooij commented Jan 4, 2023

Is there a way around having the divider as float datatype? Comparing floats is tricky (ie. when divider = 1.0 the comparison if(divider == 1.0) is not guaranteed to result true)

I agree that comparing floats is scary and a certain source of bugs in the future.

From my observations of the Itho servicetool the dividers 0.1 and 0.01 are not implemented for several pieces of the servicetool (The servicetool has several different pieces of code that converts datatypes, I do not understand why it does not simply uses a single library...). The dividers 2 and 256 are implemented everywhere.

We can use an int32_t divider if we just ignore these two odd float dividers 0.1 and 0.01. The current code does not handle them anyway.

I'll push a commit (but not today).

@arjenhiemstra
Copy link
Owner

That sounds like a workable solution as it is not implemented now indeed.

Another take might be to change it to a 2 step approach?
get_divider_from_datatype(i2cbuf[6 + i]);
to something like
get_divider_index_from_datatype(i2cbuf[6 + i]); and store that in ithoStatus.divider (although ithoStatus.dividerindex might be a better name then)

and use that index to retrieve the divider float value from an array using the index at the time it is needed.
Something like:

get_divider_by_index(ithoStatus.divider);

Dividers index 9 and 10 should 0.1 and 0.01
These seem to be never used. We set them to 1 so the divider
can be integer.
@tomkooij
Copy link
Contributor Author

tomkooij commented Jan 5, 2023

Another take might be to change it to a 2 step approach? get_divider_from_datatype(i2cbuf[6 + i]); to something like get_divider_index_from_datatype(i2cbuf[6 + i]); and store that in ithoStatus.divider (although ithoStatus.dividerindex might be a better name then)

and use that index to retrieve the divider float value from an array using the index at the time it is needed. Something like:

get_divider_by_index(ithoStatus.divider);

I was even thinking of implementing a class IthoDataType that takes the raw bytes and spits out desired datatypes/values.
But let's KISS.

I tested 1a540a3 on my WPU and it still seems to work.
I'll refactor the IthoSettings PR #133 to use the same functions.
This seems done.

@arjenhiemstra arjenhiemstra merged commit a47ceaa into arjenhiemstra:master Jan 5, 2023
arjenhiemstra added a commit that referenced this pull request Jan 5, 2023
For testing purposes only!

Changes since 2.4.4-beta3:

- feat: add ability to enable/disable various i2c commands
- fix: check if i2c returned buffer is reply of requested command
- fix: mqtt web interface issues
- fix: change syslog queue from FreeRTOS queue to stl deque
- fix: Merge pull request #130 from tomkooij/fix_signed_floats
- fix: missing settings link for WPU version 9 and make code in these cases more resilient
- fix: update arduino build info
- fix: buffer size mismatch
- fix: stop i2c bus only if i2c debug function is active
- Update supporting libs

Firmware binary (CVE HW rev.2 and NON-CVE):
https://github.com/arjenhiemstra/ithowifi/raw/master/compiled_firmware_files/hardware_rev_2/nrgitho-hw2-v2.4.4-beta4.bin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative values problem for WPU and HRU300
2 participants