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] fix defaults for tcp and serial things and some other minor cleanup #10147

Merged
merged 5 commits into from
Feb 17, 2021

Conversation

ssalonen
Copy link
Contributor

@ssalonen ssalonen commented Feb 14, 2021

Small improvements and fixes:

  • Resolve modbus itests to remove dependency to apache commons (was removed in [modbus] Replace apache commons lang usage with plain java #10002)
  • AbstractModbusEndpointThingHandler poolConfiguration is never null in practice with properly configured tcp/serial endpoints. Therefore marking with @NonNullByDefault({})
  • One of the itests did used mocked ModbusManager, leading to no-op test. Fixed now (test still passes)

More significant finding:

  • Now utilizing the defaults documented for tcp and serial configurations. Presumably wrong values were used if one did not explicitly specify the values, e.g. slave id of zero was used instead of 1, or default delay of zero was used between transactions (timeBetweenTransactionsMillis) instead something more robust

Signed-off-by: Sami Salonen ssalonen@gmail.com

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen closed this Feb 14, 2021
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen changed the title [modbus] More strict nullness. Remove apache.commons.lang from itests [modbus] fix defaults for endpoints and other fixes Feb 14, 2021
@ssalonen
Copy link
Contributor Author

Fyi @Rossko57 on the incorrect defaults

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@ssalonen ssalonen changed the title [modbus] fix defaults for endpoints and other fixes [modbus] fix defaults for tcp and serial things and some other minor fixes Feb 14, 2021
@ssalonen ssalonen changed the title [modbus] fix defaults for tcp and serial things and some other minor fixes [modbus] fix defaults for tcp and serial things and some other minor cleanup Feb 14, 2021
@ssalonen
Copy link
Contributor Author

Regression test added in fbdeb73

I can confirm that the test indeed fails with the main branch:

git checkout origin/main
git checkout -b roska
git cherry-pick fbdeb739acb6421a940b552f90999d5fd21f762c
mvn install -DwithResolver -pl itests/org.openhab.binding.modbus.tests

result:

TEST org.openhab.binding.modbus.tests.ModbusTcpThingHandlerTest#testTwoDifferentEndpointWithDifferentParameters() <<< ERROR: expected: <EndpointPoolConfiguration [interTransactionDelayMillis=1, interConnectDelayMillis=0, connectMaxTries=1, reconnectAfterMillis=0, connectTimeoutMillis=10000]> but was: <EndpointPoolConfiguration [interTransactionDelayMillis=60, interConnectDelayMillis=0, connectMaxTries=3, reconnectAfterMillis=0, connectTimeoutMillis=0]>
org.opentest4j.AssertionFailedError: expected: <EndpointPoolConfiguration [interTransactionDelayMillis=1, interConnectDelayMillis=0, connectMaxTries=1, reconnectAfterMillis=0, connectTimeoutMillis=10000]> but was: <EndpointPoolConfiguration [interTransactionDelayMillis=60, interConnectDelayMillis=0, connectMaxTries=3, reconnectAfterMillis=0, connectTimeoutMillis=0]>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1124)
	at org.openhab.binding.modbus.tests.ModbusTcpThingHandlerTest.testTwoDifferentEndpointWithDifferentParameters(ModbusTcpThingHandlerTest.java:108)

@Hilbrand Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Feb 17, 2021
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@Hilbrand Hilbrand merged commit fbf55d5 into openhab:main Feb 17, 2021
K4ntir4n pushed a commit to K4ntir4n/openhab-addons that referenced this pull request Feb 17, 2021
…cleanup (openhab#10147)

* [modbus] More strict nullness. Remove apache.commons.lang from itests
* [modbus] Defaults for tcp and serial things according to docs
* [modbus] further explicit defaults
* [modbus] document default encoding for serial.
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.
* [modbus] verify defaults are used for undefined configuration parameters

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
@wborn wborn added this to the 3.1 milestone Mar 21, 2021
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
…cleanup (openhab#10147)

* [modbus] More strict nullness. Remove apache.commons.lang from itests
* [modbus] Defaults for tcp and serial things according to docs
* [modbus] further explicit defaults
* [modbus] document default encoding for serial.
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.
* [modbus] verify defaults are used for undefined configuration parameters

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
…cleanup (openhab#10147)

* [modbus] More strict nullness. Remove apache.commons.lang from itests
* [modbus] Defaults for tcp and serial things according to docs
* [modbus] further explicit defaults
* [modbus] document default encoding for serial.
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.
* [modbus] verify defaults are used for undefined configuration parameters

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
…cleanup (openhab#10147)

* [modbus] More strict nullness. Remove apache.commons.lang from itests
* [modbus] Defaults for tcp and serial things according to docs
* [modbus] further explicit defaults
* [modbus] document default encoding for serial.
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.
* [modbus] verify defaults are used for undefined configuration parameters

Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants