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

[modbus] Modbus register array backed by bytes and other simplifications #8862

Conversation

ssalonen
Copy link
Contributor

@ssalonen ssalonen commented Oct 25, 2020

Inspired by #8172 I reworked the modbus transport to use direct byte arrays to represent the data.

This is purely refactoring with the aim of reducing boilerplate byte manipulation/copying code in bindings. With the changes we do get boosted performance: with 45000 invocations to extractStateFromRegisters with varying inputs, the self time is dropped to 55% of original, total time to 80%. Naturally these type of performance gains are dwarfed by network I/O and other real time overheads.

This has many benefits:

  • Unnecessary boxing / unboxing of ModbusRegister objects is removed all-together and one can operate directly with familiar byte[] using bitwise operations
  • Some modbus bindings constructed byte[] from ModbusRegister[] (essentially array of 16-bit data blocks, registers).

In addition I brought in more general methods to work with the data:

  • Methods such as extractXX, e.g. extractSInt8 (extract 8-bit signed integer) working directly with byte[] data. Return type is directly smallest suitable java primitive such as byte, short etc.
  • ValueBuffer interface that acts similar to java's ByteBuffer but having specialized methods to extract different types of value types that are encountered with modbus

Finally, simplifications & harmonization was made to how registers are converted to to numbers, and how commands are converted to bytes. I find the new conversion logic more readable.

The PR is splitted in two parts:

  1. changes in transport API. Mainly backwards compatible. Only exception is extractStringFromXX which now returns String, not StringType
  2. Updates and simplifications to individual bindings. Tests are added, if missing, to have regression tests for data type conversions

@ssalonen
Copy link
Contributor Author

ssalonen commented Oct 25, 2020

CC code owners to review at least the changes in the relevant binding

/bundles/org.openhab.binding.modbus.e3dc/ @weymann
/bundles/org.openhab.binding.modbus.helioseasycontrols/ @bern77
/bundles/org.openhab.binding.modbus.stiebeleltron/ @pail23
/bundles/org.openhab.binding.modbus.studer/ @giovannimirulla
/bundles/org.openhab.binding.modbus.sunspec/ @mrbig

Would be great if you could test with real devices as well

In addition, bring specialized value extraction methods and
ByteBuffer-like interface to work with data

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
...from transport

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Now extractStringFromRegisters returns String, not StringType

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen force-pushed the modbus-array-backed-by-bytes-compacted-2.5.x branch from c2dc374 to 8ebf298 Compare October 25, 2020 09:48
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@TravisBuddy
Copy link

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@ssalonen ssalonen changed the title [modbus] Modbus array backed by bytes compacted 2.5.x [modbus] Modbus register array backed by bytes and other simplifications Oct 25, 2020
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@fwolter
Copy link
Member

fwolter commented Oct 25, 2020

PRs must be filed against the main branch. See #8512. Can you create another PR and close this?

@ssalonen
Copy link
Contributor Author

Thanks @fwolter . Was there a bot to automate this?

@openhab-bot
Copy link
Collaborator

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/112/artifact/bundles/.

You can download the migrated code from there and create a new PR against the main branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.

@ssalonen
Copy link
Contributor Author

Closing in favor of #8865

@ssalonen ssalonen closed this Oct 25, 2020
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