[novafinedust] Optimizations on access to the serial port #10005

t2000 · 2021-01-31T15:27:51Z

retry logic if port does not yet exist on startup
do not write sleep command on shutdown if port has issues
no not register data listener on port but wait for data instead to be
compatible with RFC2217 serial over network implementation (see RFC2217 serial port over network: SerialPortEventListener does not work openhab-core#2170)
ignore all buffered data from device during initialization to get the
device into a defined state

Signed-off-by: Stefan Triller github@stefantriller.de

- retry logic if port does not yet exist on startup - do not write sleep command on shutdown if port has issues - no not register data listener on port but wait for data instead to be compatible with RFC2217 serial over network implementation - ignore all buffered data from device during initialization to get the device into a defined state Signed-off-by: Stefan Triller <github@stefantriller.de>

t2000 · 2021-01-31T15:30:15Z

This PR makes the usage of the binding MUCH more reliable, i.e. I do not fear restarting my openHAB installation where I had to pray whether or not the device is still working afterwards.
Also the RC2217 implementation (via ser2net) for the serial port works much better as the direct access to the port.

cpmeister · 2021-02-01T02:49:39Z

...c/main/java/org/openhab/binding/novafinedust/internal/sds011protocol/SDS011Communicator.java

@@ -78,33 +86,40 @@ public boolean initialize(WorkMode mode, Duration interval)
            throws PortInUseException, TooManyListenersException, IOException, UnsupportedCommOperationException {
        boolean initSuccessful = true;

+        logger.debug("Initializing with mode={}, interval={}", mode, interval);


I suggest changing the logging level of most of these to trace instead.

My general rule for logging levels is:

trace - used for normal expected execution paths

debug - used for unexepected execution paths, but well within normal operation

info - used for notable points in an execution path, like a milestone. (in openhab we try reserve this logging level for the core, so bindings should rarely ever call this.)

warn - used for notable unexpected execution paths that a regular user (not just a developer) should be notified of. Warnings should be used to indicate that something not-normal occurred and user intervention is required to resolve. Warnings do not indicate a failure to operate merely an abnormal condition of operation that can still be handled by the binding. Failures in binding operation should be indicated by changing the thing status to offline.

error - used to indicate catastrophic program failure. This should be used to indicate a catastrophic failure in openhab's ability to operate. A failure in a binding would never cause openhab as a whole to fail so a failure in a bindings should never log an error. Instead that failure should be indicated by changing the thing status.

cpmeister · 2021-02-01T02:51:05Z

...c/main/java/org/openhab/binding/novafinedust/internal/sds011protocol/SDS011Communicator.java

+        if (localScheduler != null) {
+            Future<?> sleepJob = null;
+            try {
+                sleepJob = localScheduler.submit(new Runnable() {


Please use a lambda here instead.

- moved most "normal" logging to TRACE level - used lambda function Signed-off-by: Stefan Triller <github@stefantriller.de>

t2000 · 2021-02-01T15:15:37Z

Thanks for your review. I have moved most logging to TRACE and changed the scheduling of the sleepjob to use a lambda.

I would appreciate if you can have another look. Thanks!

cpmeister

Only a few last changes I'd like.

cpmeister · 2021-02-04T06:40:53Z

...c/main/java/org/openhab/binding/novafinedust/internal/sds011protocol/SDS011Communicator.java

+        try {
+            write(commandData);
+        } catch (IOException ioex) {
+            logger.debug("Got an exception while writing a command, will not try to fetch a reply for it.");


You should include the exception message in the log so that it is easier to know what went wrong.

cpmeister · 2021-02-04T06:45:15Z

...ding.novafinedust/src/main/java/org/openhab/binding/novafinedust/internal/SDS011Handler.java

+                        updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR,
+                                "Cannot query data from device");


You should include the exception message in the status detail so that users have a better idea of what went wrong.

Signed-off-by: Stefan Triller <github@stefantriller.de>

t2000 · 2021-02-04T07:34:53Z

@cpmeister Thank you very much, I have integrated your changes.

) * [novafinedust] Test for optimizations on access to the serial port - retry logic if port does not yet exist on startup - do not write sleep command on shutdown if port has issues - no not register data listener on port but wait for data instead to be compatible with RFC2217 serial over network implementation - ignore all buffered data from device during initialization to get the device into a defined state * Adress review comments - moved most "normal" logging to TRACE level - used lambda function * Improve error messages as requested in the review Signed-off-by: Stefan Triller <github@stefantriller.de> Signed-off-by: John Marshall <john.marshall.au@gmail.com>

) * [novafinedust] Test for optimizations on access to the serial port - retry logic if port does not yet exist on startup - do not write sleep command on shutdown if port has issues - no not register data listener on port but wait for data instead to be compatible with RFC2217 serial over network implementation - ignore all buffered data from device during initialization to get the device into a defined state * Adress review comments - moved most "normal" logging to TRACE level - used lambda function * Improve error messages as requested in the review Signed-off-by: Stefan Triller <github@stefantriller.de>

t2000 changed the title ~~[novafinedust] Test for optimizations on access to the serial port~~ [novafinedust] Optimizations on access to the serial port Jan 31, 2021

cpmeister requested changes Feb 1, 2021

View reviewed changes

Adress review comments

7aff494

- moved most "normal" logging to TRACE level - used lambda function Signed-off-by: Stefan Triller <github@stefantriller.de>

cpmeister requested changes Feb 4, 2021

View reviewed changes

Improve error messages as requested in the review

2f4b5fd

Signed-off-by: Stefan Triller <github@stefantriller.de>

cpmeister approved these changes Feb 4, 2021

View reviewed changes

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Feb 4, 2021

cpmeister merged commit 00d2aab into openhab:main Feb 4, 2021

cpmeister added this to the 3.1 milestone Feb 4, 2021

t2000 deleted the fineDustPortOptimizePR branch February 5, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[novafinedust] Optimizations on access to the serial port #10005

[novafinedust] Optimizations on access to the serial port #10005

t2000 commented Jan 31, 2021

t2000 commented Jan 31, 2021

cpmeister Feb 1, 2021

cpmeister Feb 1, 2021

t2000 commented Feb 1, 2021

cpmeister left a comment

cpmeister Feb 4, 2021

cpmeister Feb 4, 2021

t2000 commented Feb 4, 2021

		updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR,
		"Cannot query data from device");

[novafinedust] Optimizations on access to the serial port #10005

[novafinedust] Optimizations on access to the serial port #10005

Conversation

t2000 commented Jan 31, 2021

t2000 commented Jan 31, 2021

cpmeister Feb 1, 2021

Choose a reason for hiding this comment

cpmeister Feb 1, 2021

Choose a reason for hiding this comment

t2000 commented Feb 1, 2021

cpmeister left a comment

Choose a reason for hiding this comment

cpmeister Feb 4, 2021

Choose a reason for hiding this comment

cpmeister Feb 4, 2021

Choose a reason for hiding this comment

t2000 commented Feb 4, 2021