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/extend support of fixed point values on input (modbus plugin) #7869

Merged
merged 5 commits into from
Jul 27, 2020

Conversation

sensor-freak
Copy link
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This is a fix/extension for #7317, and it presumably superseedes PR #7488 (except for range validation)

The change introduces new input type FIXED and UNFIXED, and deprecates usage of the type FIXED32 (in docs only)

@sensor-freak sensor-freak changed the title Fix/extend support of fixed point values on input Fix/extend support of fixed point values on input (modbus plugin) Jul 21, 2020
@ssoroka
Copy link
Contributor

ssoroka commented Jul 21, 2020

Two main thoughts:

  1. I'm a bit concerned that this would be a breaking change for anyone upgrading.
  2. Why is it that float32 needs to have a signed and unsigned version?

@sensor-freak
Copy link
Contributor Author

sensor-freak commented Jul 21, 2020

Two main thoughts:

  1. I'm a bit concerned that this would be a breaking change for anyone upgrading.

No, this change should not break anything for now. It just adds new types FIXED and UFIXED. The behaviour for all existing types, especially for FLOAT32 is (supposed to be) unchanged and therefore all existing configurations should work as they do currently.

  1. Why is it that float32 needs to have a signed and unsigned version?

The FLOAT32 reads an integer value from modbus registers, converts this integer value to a float value, applies the given scale (by multiplication of the float value with the configured scale value), and stores the result as a floating point type. When the sign bit of the input value from modbus registers is set, the result of the conversion from int to float will be different depending on the type of the input: for signed integers, the conversion must result in negative values, while for unsigned integers, the sign bit is the MSB of a positive value and thus the result must be a (large) positive value. Therefore we must be able to specify in configuration, which of these cases should be applied (it cannot be determined automatically, since there is no indication from modbus which one is the right one.)

Example:

  • Modbus register value 0xFFFF (16 Bit), scale 0.01
  • Interpretation with FIXED (signed):
    -- (FLOAT64)(INT16)0xFFFF = -1.0;
    -- -1.0 * scale = -0.01;
  • Interpretation with UFIXED (unsigned):
    -- (FLOAT64)(UINT12)0xFFFF = 65535.0;
    -- 65535.0 * scale = 655.35;

(Please note that this is something else than FLOAT32-IEEE, which directly reads the bits from modbus registers into a float32 variable, without interpreting the bits as integer in the first place.)

And the need for both convertions is reality. I'm currently working with a modbus device, which uses both variants (signed and unsigned) with different scaling values in many combinations:

30795 Netzstrom AC (A) [Iac] 2 U32 FIX3 RO
30803 Netzfrequenz (Hz) [Fac] 2 U32 FIX2 RO
30805 Blindleistung (var) [Qac] 2 S32 FIX2 RO
30813 Scheinleistung3 (VA) [Sac] 2 S32 FIX0 RO

Here S32/U32 specifies INT32 and UINT32 resp., and FIX3, FIX2,... specifies the number of decimal digits contained in the integer value.

@sensor-freak
Copy link
Contributor Author

I've just re-checked again, it seems that I made a mistake in handling of FLOAT32 data type. Please assume this PR on hold until I've verified and probably corrected this. (The current state might be a breaking change for other users.)

@sensor-freak
Copy link
Contributor Author

The last commit fixes the break that I've introduced. With this commit the type FLOAT32 is handled identical to UFIXED, which is how it is handled in the current main branch.

I have verified this by running the new unit tests with the code from the main branch.

(The confusion came from PR #7488, which tried to introduced a breaking change. And after that I didn't realize that I had incorporated the idea of that change into my brain.)

@sensor-freak
Copy link
Contributor Author

sensor-freak commented Jul 23, 2020

Failing checks are not related to modbus plugin, and (seem to) do not fail in main branch. Can I force a re-run of the checks somehow?

Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the typo it looks good to me.

plugins/inputs/modbus/README.md Outdated Show resolved Hide resolved
@sensor-freak sensor-freak requested a review from reimda July 24, 2020 06:04
Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ssoroka ssoroka merged commit d233b4c into influxdata:master Jul 27, 2020
@sensor-freak sensor-freak mentioned this pull request Aug 21, 2020
3 tasks
ssoroka pushed a commit that referenced this pull request Aug 29, 2020
@sjwang90 sjwang90 added this to the 1.15.3 milestone Sep 1, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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.

4 participants