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

Add FLOAT64-IEEE support to inputs.modbus #8361

Closed
Nemecsek opened this issue Nov 4, 2020 · 9 comments
Closed

Add FLOAT64-IEEE support to inputs.modbus #8361

Nemecsek opened this issue Nov 4, 2020 · 9 comments
Labels
area/modbus feature request Requests for new plugin and for new features to existing plugins

Comments

@Nemecsek
Copy link

Nemecsek commented Nov 4, 2020

Feature Request

Proposal:

Add FLOAT64-IEEE Modbus registers support.
INT64, UINT64, FLOAT32-IEEE are already available.
FLOT64-IEEE is used natively at least in some Janitza devices or can be added to some of them through Jasic code.

Current behavior:

A 32-bit float has about 7 digits of precision and a 64-bit double has about 16 digits of precision.
Some counter registers are read and a delta is calculated after a period in time. If the counter is big enough, the lack of precision given by a float32 register is enough to lose changes.

Desired behavior:

Adding FLOAT64-IEEE support to inputs.modbus solves the problem.

Use case:

In a Janitza UMG604 an Energy counter is read every 15 minutes. Natively UMG604 exposes only float32 Modbus registers while internal calculations are float64.

When the counter is low (i.e. below 1.000.000) the number of meaningful digits is enough to detect unitary changes in the counter during this interval. As soon as the counter is bigger than maximum precision (about 7 digits), less significant digits are lost and little changes in the counter are no more detected.
UMG604 can be extended by using Jasic code: I exposed the internal float64 registers adding some new Modbus registers to the standard ones. Alas inputs.modbus plugin does not support FLOAT64-IEEE registers to read them back.

For this reason I implemented for personal use and tested this FLOAT64-IEEE extension in modbus.go
I can commit the change if somebody thinks it is worth. I am not a go developer but the change was very easy using the template provided by the other data formats.

It has been tested on the real devices and verified thoroughly.

@Nemecsek Nemecsek added the feature request Requests for new plugin and for new features to existing plugins label Nov 4, 2020
@ssoroka
Copy link
Contributor

ssoroka commented Nov 5, 2020

@Nemecsek please do feel free to make a PR with the changes

@Nemecsek
Copy link
Author

Nemecsek commented Nov 5, 2020

Thank you.
No way to push it. I never had any problem before.
I have been trying for hours, following a number of tutorials.
I always get ERROR: Permission to influxdata/telegraf.git denied to Nemecsek.
fatal: Could not read from remote repository.

ssh -vT [email protected] authenticate me correctly.

I will try again when I am not so pressed by other job related tasks...

@youcann
Copy link

youcann commented Nov 22, 2020

Also interested (For a Siemens PAC2200)

@Nemecsek
Copy link
Author

Nemecsek commented Nov 24, 2020

@youcann, sorry but I continuously get a permission denied to push the modified telegraf/plugins/inputs/modbus.go.
I leave you the file in attachment. I renamed it to .go.txt because .go files cannot be attached to this comment.
Please, if you can push it do it.

modbus.go.txt

@youcann
Copy link

youcann commented Nov 24, 2020

Thanks alot!
Regarding your permission error: I'm not sure what exactly you did, bur standard procedure is to open a pull request for the maintainer of the original repo to suggest him to merge your work.

@Nemecsek
Copy link
Author

@youcann, I cannot push my branch to github, that's the problem. So no PR.
I tried a number of solutions I found online but at no avail. I forked it and cloned it multiple times to fix this issue, even after reinstalling my Ubuntu machine.

Credentials have been reset:
git credential reject / git credential fill

HTTPS URL has been tried with both formats:

remote.origin.url=https://github.com/influxdata/telegraf.git
remote.origin.url=https://[email protected]/influxdata/telegraf.git

It should be straightforward, but actually it isn't.

I don't want to fill this thread with unrelated stuff. If you can push it using your account and PR, it would be great.

@soerman
Copy link
Contributor

soerman commented Nov 25, 2020

Created a PR for this: #8474

@srebhan
Copy link
Member

srebhan commented Nov 25, 2020

@Nemecsek you cannot push to Telegraf directly. You should fork the repository, push your changes there to a branch and then create a pull-request (PR). You can find information here. Pull-requests ensure that code gets reviewed to find issues and guaranteeing quality before code hits people (at least in theory ;-)).

Ok short step-by-step as I see you are still trying to push to telegraf directly (remote.origin.url=https://github.com/influxdata/telegraf.git):

  1. Sign in to github
  2. Click the fork button (top right) to fork telegraf into your personal repository e.g. Nemecsek/telegraf
  3. Clone Nemecsek/telegraf NOT influxdata/telegraf! locally
  4. Apply your changes locally
  5. Push your local changes to a branch in Nemecsek/telegraf again NOT influxdata/telegraf!
  6. Navigate to the branch in Nemecsek/telegraf on github and click Pull request

@ssoroka
Copy link
Contributor

ssoroka commented Nov 30, 2020

merged in #8474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modbus feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

No branches or pull requests

6 participants