ModbusManagerInterface refactored to have separate result and failure callbacks

[mrbig]

Hello,

sorry for the delay, I was pretty much offline at the weekend. I've tested your code, and also read through the PR. I liked the proposal by @fwolter, mostly because it will allow to change most of the callback methods and parameters to be @nonnull.

However I understood that you were hesitating to start the refactor again, so I just started to look into the code to see how much would break... and after a while I showed to be long but relatively self understand set of changes.

So here in this PR you'll see how I accomplished those changes.

However there are a few things left I was considering also:

• I've found out that ModbusDataThingHandler in org.openhab.binding.modbus has a lot of references to ModbusReadRequests, however a read task is never executed. I could not decide if it's an error caused by some refactor, or just some leftover dead code. I've added TODO's to the places I believe are never used now.

• I've changed the result callback for ModbusCommunicationInterface::registerRegularPoll and ModbusCommunicationInterface::submitOneTimePoll to @nonnull. However I would like to go a step further and change every parameter @nonnull. This would make several internal classes to be nonnull as well, and I don't really see a need to call those methods with a null callback. If it's really needed by some use case we could still override them with fewer parameters and internally pass along a VoidCallback. What do you think about this?

[mrbig]

I've updated the SmokeTest as well, can you take a look at it that I had no mistake?

[mrbig]

I've found out that ModbusDataThingHandler in org.openhab.binding.modbus has a lot of references to ModbusReadRequests, however a read task is never executed. I could not decide if it's an error caused by some refactor, or just some leftover dead code. I've added TODO's to the places I believe are never used now.

I've found out that I've missed the tests package, and through those I've found how they were used in ModbusPollerThingHandler.

I've tried to fix the tests as much as possible, but it's not complete yet: the failures are not delegated to the children. This part might needs some rethinking. However I keep the PR open if you wish to look into the code.

[ssalonen]

Thanks @mrbig, huge effort! I glanced through and generally looks good. Will check with more detail

Did you look into the integration tests (under itest) and how much work it would take to fix those? There's a quite significant test set there.

[mrbig]

Hello,
more on less I could fix the itests too. I could not find out how to execute them locally, but the travis run now only had one failed test.

I also know it's cause, and that was the point where I decided to take a sleep before continuing.

The problem is that the PollerThing handler collects the DataThing handlers as ModbusReadCallback (private volatile List<ModbusReadCallback> childCallbacks). But to propagate the failures I should call the failure handlers as well, but they are not exposed now.

I believe the root cause here is that ModbusReadCallback/ModbusWriteCallback interfaces are used for a different purpose than originally designed for. Even though they provide more on less the required functionality, they do not fit perfectly.

So I would propose the add a different interface for the PollerThing->DataThing relation, for example an interface ModbusPollerChild that could provide the necessary methods to forward events from the PollerThing to the DataThing.

Also that would mean that the ModbusDataThing handler would implement only this new ModbusPollerChild interface.

What do you think about this?

[ssalonen]

Hmm I see yes... The poller caching is also broken now for errors. Probably needs to be reverted to state similar to 2.5.x:

https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusPollerThingHandlerImpl.java#L75-L77

I think the ModbusPoller should be actually package-internal, and it is OK to assume that you can have only data things as children. I'm not sure if separate interfaces are needed?

One thing I also noted is that due to Java generic type erasure, a class cannot implement both ModbusFailureCallback and ModbusFailureCallback. Typing them separately (similar to) ModbusReadCallback and ModbusWriteCallback workarounds the issue.

We would then have 4 callbacks:

• ModbusReadCallback (ok results)
• ModbusReadFailureCallback (read failures)
• ModbusWriteCallback (ok results)
• ModbusWriteFailureCallback (write failures)

[ssalonen]

Should we return Optional<ModbusRegisterArray> instead?

[mrbig]

I like that, it would be a cleaner approach.

[ssalonen]

Implemented in mrbig#1

[ssalonen]

Change in behaviour: errors are not cached -- we should cache them as well.

[mrbig]

Yes, I'm aware of this, and this is the main todo at the moment

[ssalonen]

I proposed to take back changes from here https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusPollerThingHandlerImpl.java#L75-L77

However, perhaps even simpler would be to

1. Restore AtomicStampedKeyValue.java from https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/AtomicStampedKeyValue.java where key would be the request and value would be the response/error
2. Use single AtomicStampedKeyValue in poller, but erasing the types with Objects and casting where necessary when calling callbacks

[mrbig]

I have fixed this by adding a small private class to store the result+failure together. This way I could put back the original caching.

[ssalonen]

Looks good! I added toString implementation to PollResult in mrbig#1 since it is used in logging

[ssalonen]

Gives me an error. When using matchers (notNull), all arguments must be matchers.
I propose:

        verify(comms, never()).submitOneTimePoll(eq(request), notNull(), notNull());

[mrbig]

One thing I also noted is that due to Java generic type erasure, a class cannot implement both ModbusFailureCallback and ModbusFailureCallback. Typing them separately (similar to) ModbusReadCallback and ModbusWriteCallback workarounds the issue.
We would then have 4 callbacks:

I would take a different approach:

The DataThingHandler could expose methods to get notified about the result/failure, like onParentResult and onParentFailure. Then there will be no need to implement the callback interfaces on the class itself, instead it could use convinient lambda methods:

registerRegularPoll(task, config.getRefresh(), 0, this::doSomething, this::handleFailure);

or using the ReadCallbackDelegator it's also not needed to have both read and write handlers implemented at the same time.

Note that I was also considering to have both a ModbusReadFailureCallback and ModbusWriteFailureCallback but I then abandoned it, because it doesn't really look like it would ease anything, but we have more and more classes for the same task.

[ssalonen]

Is this a correct change? I get test failure:

java.lang.ClassCastException: org.mockito.codegen.ModbusWriteRequestBlueprint$MockitoMock$1796262632 cannot be cast to org.openhab.io.transport.modbus.ModbusReadRequestBlueprint
	at org.mockito.codegen.PollTask$MockitoMock$823535341.getRequest(Unknown Source)
	at org.mockito.codegen.PollTask$MockitoMock$823535341.getRequest(Unknown Source)
	at org.openhab.binding.modbus.tests.ModbusDataHandlerTest.createPollerMock(ModbusDataHandlerTest.java:145)
	at org.openhab.binding.modbus.tests.ModbusDataHandlerTest.testWriteHandlingGeneric(ModbusDataHandlerTest.java:457)
	at org.openhab.binding.modbus.tests.ModbusDataHandlerTest.testWriteRealTransformation(ModbusDataHandlerTest.java:591)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.mockito.internal.runners.DefaultInternalRunner$1$1.evaluate(DefaultInternalRunner.java:44)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.mockito.internal.runners.DefaultInternalRunner$1.run(DefaultInternalRunner.java:74)
	at org.mockito.internal.runners.DefaultInternalRunner.run(DefaultInternalRunner.java:80)
	at org.mockito.internal.runners.StrictRunner.run(StrictRunner.java:39)
	at org.mockito.junit.MockitoJUnitRunner.run(MockitoJUnitRunner.java:163)
	at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:38)
	at junit.framework.TestSuite.runTest(TestSuite.java:252)
	at junit.framework.TestSuite.run(TestSuite.java:247)
	at aQute.junit.Activator.test(Activator.java:326)
	at aQute.junit.Activator.automatic(Activator.java:220)
	at aQute.junit.Activator.run(Activator.java:176)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at aQute.launcher.Launcher.launch(Launcher.java:385)
	at aQute.launcher.Launcher.run(Launcher.java:143)
	at aQute.launcher.Launcher.main(Launcher.java:121)
	at aQute.launcher.pre.EmbeddedLauncher.executeWithRunPath(EmbeddedLauncher.java:154)
	at aQute.launcher.pre.EmbeddedLauncher.findAndExecute(EmbeddedLauncher.java:119)
	at aQute.launcher.pre.EmbeddedLauncher.main(EmbeddedLauncher.java:51)

I think the issue is that in createPollerMock, we have

ModbusPollerThingHandler mockHandler = Mockito.mock(ModbusPollerThingHandler.class);
doReturn(task.getRequest()).when(mockHandler).getRequest();

This is of course wrong -- PollerThingHandler should not return Write requests...

Should we instead have two types of requests, one for the mocked poller, one corresponding to the data thing (?

Similar errors with testWriteRealTransformation2, testWriteRealTransformation3, and testWriteRealTransformation4

[mrbig]

The name confused me, but now I understand that it sets up a poller to check the results of the write task. I have reset these to the original code

[ssalonen]

Regarding integration tests, you can find the instructions to run them at https://www.openhab.org/docs/developer/tests.html#integration-tests

It should be possible to run them in eclipse as well, right-click itest.bndrun and Run As -> Bnd OSGi Test Launcher (JUnit).

[ssalonen]

One thing I also noted is that due to Java generic type erasure, a class cannot implement both ModbusFailureCallback and ModbusFailureCallback. Typing them separately (similar to) ModbusReadCallback and ModbusWriteCallback workarounds the issue.
We would then have 4 callbacks:

I would take a different approach:

The DataThingHandler could expose methods to get notified about the result/failure, like onParentResult and onParentFailure. Then there will be no need to implement the callback interfaces on the class itself, instead it could use convinient lambda methods:

registerRegularPoll(task, config.getRefresh(), 0, this::doSomething, this::handleFailure);

or using the ReadCallbackDelegator it's also not needed to have both read and write handlers implemented at the same time.

Note that I was also considering to have both a ModbusReadFailureCallback and ModbusWriteFailureCallback but I then abandoned it, because it doesn't really look like it would ease anything, but we have more and more classes for the same task.

I see your point, makes sense. I also agree that there's really no need to allow implementing both callbacks in one class -- one can always use one-liner lambdas and delegate to whatever methods are applicable.

What if we would move to BiConsumers in the API? Would remove those needless interfaces...

[ssalonen]

I've changed the result callback for ModbusCommunicationInterface::registerRegularPoll and ModbusCommunicationInterface::submitOneTimePoll to @nonnull. However I would like to go a step further and change every parameter @nonnull. This would make several internal classes to be nonnull as well, and I don't really see a need to call those methods with a null callback. If it's really needed by some use case we could still override them with fewer parameters and internally pass along a VoidCallback. What do you think about this?

I agree, we can require non-null callbacks from the API consumer. As you said, they can always provide no-op callback.

[TravisBuddy]

Travis tests have failed

Hey @mrbig,
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.

[mrbig]

Thanks for the help on how to run the integration tests, unfortunately I had no luck with them. Eclipse gives me a NoClassDefFoundError: org/osgi/framework/ServiceListener error, and if I try to run them through maven as described here then I get some osgi errors about missing dependencies. For so long I use the travis ci to run the tests, then I'll see if it's enough.

[TravisBuddy]

Travis tests have failed

Hey @mrbig,
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.

[mrbig]

@ssalonen could you check out the latest version? Now it passes the Travis unit tests as well, unfortunately on the last push the unrelated mqtt plugin failed in one of the two tests.

[ssalonen]

I've changed the result callback for ModbusCommunicationInterface::registerRegularPoll and ModbusCommunicationInterface::submitOneTimePoll to @nonnull. However I would like to go a step further and change every parameter @nonnull. This would make several internal classes to be nonnull as well, and I don't really see a need to call those methods with a null callback. If it's really needed by some use case we could still override them with fewer parameters and internally pass along a VoidCallback. What do you think about this?

I agree, we can require non-null callbacks from the API consumer. As you said, they can always provide no-op callback.

I've introduced this in mrbig#1 in commit mrbig@fae05a2 & mrbig@4ce384b. Is this what you meant?

[ssalonen]

@mrbig I think it looks good now! I added some more changes we discussed in mrbig#1

Merge that and I think we are good to proceed in openhab#7994?

[mrbig]

Thanks, I've merged in your commits. Unfortunately now travis fails also with the integrations tests, it's basically the same what i got at my computer. I'm not good resolving these osgi dependency issues, do you have any idea what's happening there?

[TravisBuddy]

Travis tests were successful

Hey @mrbig,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @mrbig,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[ssalonen]

I'm not sure... I will try forcing a rebuild

[TravisBuddy]

Travis tests were successful

Hey @mrbig,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.