-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[novafinedust] Nova Fine Dust binding for SDS011 sensors #7528
Conversation
I have the binding attached for testing, please rename to ".jar" before putting it into the addons folder. |
Travis tests have failedHey @t2000, |
082d9ea
to
17ab9e4
Compare
Travis tests were successfulHey @t2000, |
|
||
| parameter name | mandatory | description | | ||
|-------------------|-----------|---------------------------------------------------------------------------------------| | ||
| port | yes | the port the sensor is connected to, i.e. /detv/ttyUSB0. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| port | yes | the port the sensor is connected to, i.e. /detv/ttyUSB0. | | |
| port | yes | the port the sensor is connected to, i.e. /dev/ttyUSB0. | |
|
||
// initialize timeBetweenDataShouldArrive with a large number | ||
private Duration timeBetweenDataShouldArrive = Duration.ofDays(1); | ||
private final Duration dataCanBeLateTollerance = Duration.ofSeconds(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "Tolerance"
try { | ||
communicator.requestSensorData(); | ||
} catch (IOException e) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Thing is not set as online again, if the connection succeeds in one of the upcoming cycles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be set to ONLINE again once there is valid data received, see method updateChannels
.
private boolean validateConfiguration() { | ||
if (config.port == null || config.port.isEmpty()) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.CONFIGURATION_ERROR, "Port must be set!"); | ||
return false; | ||
} | ||
|
||
if (config.reporting) { | ||
if (config.reportingInterval < 0 || config.reportingInterval > 30) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.CONFIGURATION_ERROR, | ||
"Reporting interval has to be between 0 and 30 minutes"); | ||
return false; | ||
} | ||
} else { | ||
if (config.pollingInterval < 3 || config.pollingInterval > 3600) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.CONFIGURATION_ERROR, | ||
"Polling interval has to be between 3 and 3600 seconds"); | ||
return false; | ||
} | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to check the config again, if you configured min/max etc. in the XML files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, so openHAB now validates these values before passing them to the handler? That's cool, I didn't know that this was implemented. Thanks for letting me know. I have removed the checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I have to retract my words. The config is only validated in Paper UI, but not when configuring via a things file. @cpmeister should bindings double check their config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always better if the binding double checks the configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I have reverted the removal of the check, so it is back in place.
private void verifyIfStillConnected() { | ||
ZonedDateTime now = ZonedDateTime.now(); | ||
Temporal lastData = lastCommunication.plus(timeBetweenDataShouldArrive).plus(dataCanBeLateTollerance); | ||
if (now.isAfter((ChronoZonedDateTime<?>) lastData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use ZonedDateTime
instead of Temporal
, you could remove this cast.
reply = readReply(); | ||
logger.debug("Got data from sensor: {}", reply); | ||
} catch (IOException e) { | ||
logger.error("Could not read available data from the serial port", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bindings should only log as error, when a bug in the code is detected. You could use warn here.
IOUtils.closeQuietly(inputStream); | ||
IOUtils.closeQuietly(outputStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are going to remove the Apache Commons dependencies, could you replace the lines with standard Java code? See #7722
Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
Signed-off-by: Stefan Triller <[email protected]>
Thanks for the review. I have addressed your comments, except for one where I replied here on github. I have also rebased the code against the latest 2.5.x branch. |
Travis tests were successfulHey @t2000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except the single log message.
@cpmeister The dev was very responsive. Can you take a look?
@@ -253,7 +252,7 @@ public void serialEvent(SerialPortEvent event) { | |||
reply = readReply(); | |||
logger.debug("Got data from sensor: {}", reply); | |||
} catch (IOException e) { | |||
logger.error("Could not read available data from the serial port", e); | |||
logger.warn("Could not read available data from the serial port", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to log the stacktrace here, but you could log the exception message:
logger.warn("Could not read available data from the serial port", e); | |
logger.warn("Could not read available data from the serial port: {}", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this makes sense to only log the message. I have changed it, thanks!
Signed-off-by: Stefan Triller <[email protected]>
Travis tests were successfulHey @t2000, |
* @param bytes the byte array to be converted | ||
* @return a String describing the byte array in hexadecimal values | ||
*/ | ||
public static String toHexString(byte[] bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an implementation of this in HexUtils
from openhab core.
// we do not support refreshing as values are either reported by the device or polled from the device in fixed | ||
// intervals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could cache the values so that a Refresh command would be able to get the most recently reported ones.
} | ||
|
||
if (communicator != null) { | ||
scheduler.schedule(() -> communicator.dispose(), 0, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this disposed asynchronously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I think initialize
and dispose
from a ThingHandler
should return quickly, i.e. within 5 seconds. Inside the dispose
I send the sensor to sleep by communicating with it. This is important to enhance its lifetime.
So in order to not block the disposing call, I do it asynchronously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens if the handler is restarted (disposed then initialized again)? Would the handler be stuck in a configuration COMMUNICATION_ERROR because the port is still in use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a VERY interesting point! :-)
There are similar problems in the zigbee binding, because there it also takes a while to startup the stack and if this gets restarted through dispose/initialize calls, everything gets messed up too. Synchronization might help there but is a pain in the ass to do that for every binding.
And I am afraid to say that with the 5 second rule for initialize/dispose, bindings are forced to spawn a separate task, which will ALWAYS cause these troubles if dispose/initialize is called within a short time frame (i.e. by a disable/enable of the thing).
If you prefer to have my dispose here running in sync with the framework, I am fine to replace the task with a direct call. In my case this usually only takes about 1-2 seconds anyway, but I wanted to play safe since I am communicating with an external device and this might timeout, etc.
But the overall problem for all bindings will stay, especially with the use case of enabling and disabling of a thing.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I can see of dealing with delayed initialize/dispose is to make your handler and communicator objects have different lifecycles.
You would need to create a your own communicator factory class that handlers would request communicators for a given port.
public interface CommunicatorFactory{
SDS011Communicator getCommunicator(SerialPortIdentifier portId);
}
The factory itself would share the same lifecycle as an osgi component so either you could make your handler factory a communicator factory or implement it in a new osgi component.
The key benefit here is that you would be able to reuse SDS011Communicator instances across handler restarts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really solve the problem, I could still have the communicator instance running if i dispose the handler and then the port is blocked.
Also its not worth the effort as I said before the communication takes max 1-2 seconds.
I have changed it to be synchronous now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really solve the problem, I could still have the communicator instance running if i dispose the handler and then the port is blocked.
Not really since you can prevent a communicator from opening the port if it is already open. Likewise a communicator can stop its disposal process if a new handler requests to use it.
I have changed it to be synchronous now.
I don't think would work, a new handler means a new communicator object. Synchronization only works on the same object instance.
Nvm, just saw what you committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really since you can prevent a communicator from opening the port if it is already open. Likewise a communicator can stop its disposal process if a new handler requests to use it.
The problem is not having a different thing, let's say "thing2" wanting the very same port. This is already handled, because the port in use exception is dealt with. And this is always a user problem, since he has configured thing1 and thing2 with the same port.
What I am talking about is that the ThingHandler from thing1 is disposed and shortly afterwards initialized again. This can happen if one disabled and enables a thing, see https://github.com/openhab/openhab-core/blob/5b325aa3d210c2dfa490c676520fe755b0f3fe1c/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java#L1167-L1216
Here it doesnt matter if a communicator object or the thinghandler itself want to open the port on initialize. If it is currently in use by a thinghandler that gets disposed at the moment, then there is no way for a starting thinghandler to open the port and he will run into the port in use exception.
This applies to ALL binding which are operating on a shared resource like a port btw...
So with the change I did yesterday evening, the dispose is always called and finished before an initialize, so for now all is good within this binding.
However, one should think about the general problem of shared resources and scenarios where an initialize or dispose might take longer and one has no other choice than putting this in an external task because of this 5 second rule from openHAB.
|
||
private static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections.singleton(THING_TYPE_SDS011); | ||
|
||
private @NonNullByDefault({}) SerialPortManager serialPortManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest populating this through the NovaFineDustHandlerFactory constructor.
private @NonNullByDefault({}) SerialPortManager serialPortManager; | |
private final SerialPortManager serialPortManager; | |
@Activate | |
public NovaFineDustHandlerFactory(@Reference SerialPortManager serialPortManager){ | |
this.serialPortManager = serialPortManager; | |
} |
Then you can remove the setSerialPortManager
and unsetSerialPortManager
methods.
|
||
@Override | ||
public void dispose() { | ||
if (pollingJob != null && !pollingJob.isCancelled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm in cancelling something that is already cancelled.
if (pollingJob != null && !pollingJob.isCancelled()) { | |
if (pollingJob != null) { |
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
* @author Stefan Triller - Initial contribution | ||
* | ||
*/ | ||
public enum WorkMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add @NonNullByDefault
Signed-off-by: Stefan Triller <[email protected]>
@cpmeister Thanks for your review. Again I learned something about OSGi (reference thingy in the constructor) and that openHAB has some units on the thing-types.xmls :) I have addressed all your comments and replied to your questions. Please have a look. |
Travis tests have failedHey @t2000, |
Signed-off-by: Stefan Triller <[email protected]>
Travis tests were successfulHey @t2000, |
Signed-off-by: Stefan Triller <[email protected]>
Hey @t2000, TravisCI finished with status TravisBuddy Request Identifier: aecad270-9f71-11ea-a34f-dd9ab62511aa |
...main/java/org/openhab/binding/novafinedust/internal/sds011protocol/messages/SensorReply.java
Outdated
Show resolved
Hide resolved
...g/openhab/binding/novafinedust/internal/sds011protocol/messages/SensorMeasuredDataReply.java
Outdated
Show resolved
Hide resolved
...n/java/org/openhab/binding/novafinedust/internal/sds011protocol/messages/CommandMessage.java
Outdated
Show resolved
Hide resolved
if (serialPort != null) { | ||
try { | ||
// send the device to sleep to preserve power and extend the lifetime of the sensor | ||
sendSleep(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you remove the event listener before you call this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this before, but somehow the underlieing library does something with the port if I de-register my listener and then my command doesn't go through.
This is just a guess from my observation. That is why I did not de-register the listener before that call here.
Also in the callback method for the listener I check for valid data, so it doesn't matter if the callback receives the reply to the sleep on shutdown.
...c/main/java/org/openhab/binding/novafinedust/internal/sds011protocol/SDS011Communicator.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/openhab/binding/novafinedust/internal/sds011protocol/SDS011Communicator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Stefan Triller <[email protected]>
Travis tests were successfulHey @t2000, |
Thanks for the review. I have addressed your changes. |
Travis tests were successfulHey @t2000, |
scheduler.schedule(() -> initializeCommunicator(WorkMode.REPORTING, timeBetweenDataShouldArrive), 0, | ||
TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scheduler.schedule(() -> initializeCommunicator(WorkMode.REPORTING, timeBetweenDataShouldArrive), 0, | |
TimeUnit.SECONDS); | |
scheduler.submit(() -> initializeCommunicator(WorkMode.REPORTING, timeBetweenDataShouldArrive)); |
scheduler.schedule(() -> initializeCommunicator(WorkMode.POLLING, timeBetweenDataShouldArrive), 0, | ||
TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scheduler.schedule(() -> initializeCommunicator(WorkMode.POLLING, timeBetweenDataShouldArrive), 0, | |
TimeUnit.SECONDS); | |
scheduler.submit(() -> initializeCommunicator(WorkMode.POLLING, timeBetweenDataShouldArrive)); |
connectionMonitorStartDelay.getSeconds(), timeBetweenDataShouldArrive.getSeconds(), TimeUnit.SECONDS); | ||
} | ||
|
||
private void initializeCommunicator(WorkMode mode, Duration interval) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be safe, I would make sure that initialize
, dispose
, and initializeCommunicator
are synchronized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should I do that? This won't work if I write synchronized
on those methods.
The reason is that this will synchronize of the ´ThingHandlerobject and this is never the same is a
disposeand an
initializeare called. The
ThingManagerwill always create a new
ThingHandlerinstance before calling
initialize, so the call to
dispose` will happen on a different (old) object.
This is what I wanted to state earlier: The openHAB framework currently does NOT ensure that dispose
and initialize
run into each other, if one is forced to schedule a task (that runs in a different thread) from initialize
if one needs more than 5 seconds.
So either: I do NOT schedule this task and risk the warning "ThingHandler initialize takes longer than 5 seconds", or I take the (in my case pretty unlikely) risk that dispose
and initialize
collide eventually.
So which option should I pick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@J-N-K Could you please weigh in here? I can't tell if I'm being overly concerned or not with regards to thread safety here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can chime in here. We never have synchronized
keywords on such methods. As @t2000 mentions, the work is done in a separate thread and it is actually intended that the framework is able to dispose the handler, while such jobs might still be running. They should NOT block a call of dispose
, so synchronizing it would be counter-productive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaikreuzer I'm just concerned that initializeCommunicator
would be called after dispose
since initializeCommunicator
is called asynchronously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds pretty unlikely and even if it does, the code should be written in a way that it shouldn't harm much.
If dispose would set the communicator to null and initializeCommunicator would not try to set the Thing status upon a null-communicator, it should almost be safe ;-)
Signed-off-by: Stefan Triller <[email protected]>
Travis tests were successfulHey @t2000, |
Thank you for the reviews and the approvals. is there anything else that needs to be done from my side before merging this? If so, please ping me. |
No, you did fine. All you have to do is to wait for another maintainer to approve your PR. That's why the CRE label has been added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @t2000 - please find some final review comments below.
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.novafinedust/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Travis tests were successfulHey @t2000, |
Thanks Kai for your suggestions, I have accepted them all and will take care of the few others in an additional commit. Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Stefan Triller <[email protected]>
Signed-off-by: Stefan Triller <[email protected]>
Thanks, for the final review @kaikreuzer. Please have a look if its OK or I missed something. Thank you! |
Travis tests were successfulHey @t2000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick updates, lgtm now!
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]> Signed-off-by: CSchlipp <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
* Nova Fine Dust binding for SDS011 sensors Closes openhab#7527 Signed-off-by: Stefan Triller <[email protected]>
Closes #7527
Signed-off-by: Stefan Triller [email protected]