-
-
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
FTP Upload binding #3137
FTP Upload binding #3137
Conversation
bd2281d
to
a743a18
Compare
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.
Thank you for your binding! It is indeed a nice hack to support such cameras in a generic way.
Do you have example vendors and model names of cameras that support this ftp upload functionality once they detect movement?
Two major comments:
- I think you only need one channel, the image one, the others are redundant.
- Your thing should go ONLINE/OFFLINE if the device is reachable/not reachable.
Please also have a look at the comments inline in the code.
<label>User Name</label> | ||
<description>Username</description> | ||
</parameter> | ||
<parameter name="password" type="text" required="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.
Please add a <context>password</context>
for this parameter.
@@ -0,0 +1,21 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
We are using DS annotations now. Can you please remove this file and put a ".gitignore" fle in the OSGI-INF directory?
Inside your HandlerFactory
you can use the @Component
annotation which will create this xml description for you. You can find an example in the astro binding
@@ -0,0 +1,83 @@ | |||
# Network Camera Binding | |||
|
|||
This binding integrates large number of network cameras which can send image to FTP server when motion or sound is detected. Binding acts as a FTP server. Images stored to FTP server are not saved to the file system, therefore binding shouldn't cause any problems on flash based openHAB installations. |
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 a new line after each sentence. This simplifies merging later.
...send images to a FTP server if motion...
Images stored on the FTP server...
therefore the binding...
| Channel Type ID | Item Type | Description | | ||
|-----------------|--------------|----------------------------------------------------------------------------------------| | ||
| image | Image | Image received from network camera. | | ||
| motion | Switch | Motion detection sensor state. Updated to ON state when image is received from camera. | |
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.
What is the difference to the trigger channel? I guess you only need the image channel, don't you?
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.
Binding is actually done, when trigger channel was pretty new concept, so that's why there is also direct motion channel.
| motion-trigger | MOTION_DETECTED | Triggered when image received from network camera. | | ||
|
||
|
||
## Full Example |
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.
Can you please also provide a .sitemap
file? You can at least show the current image, right?
byte[] d = DatatypeConverter.parseHexBinary(data.toString()); | ||
logger.debug("File len: {}", d.length); | ||
return d; | ||
} catch (Exception 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.
See above
} | ||
} | ||
|
||
private class MyFTPLet extends DefaultFtplet { |
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.
Usually one or two small private classes fit nicely into a class, but I think you should factor out yours into extra files because they are too many which makes it hard to read.
try { | ||
server.start(); | ||
} catch (FtpException e) { | ||
e.printStackTrace(); |
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 use the logger.
|
||
| Channel Type ID | Options | Description | | ||
|-----------------|------------------------|----------------------------------------------------| | ||
| motion-trigger | MOTION_DETECTED | Triggered when image received from network camera. | |
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.
see above: I a rule you can also check if the image channel has received and update, so that one channel should be sufficient.
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.
Motion trigger channel is useful, if someone don't want to have image channel e.g. for private reaseon.
|
||
ftpServer.addEventListener(this); | ||
ftpServer.addAuthenticationCredentials(configuration.userName, configuration.password); | ||
updateStatus(ThingStatus.ONLINE); |
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 should be a mechanism to set the thing to ONLINE/OFFLINE if its reachable/not reachable. How about implementing a scheduled job that pings the device every x seconds to see if the camera is still there?
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.
Thank you for your very quick response!
I left some more comments in the code.
One larger point which I would like to discuss with you is if this binding should rather be named something like "generic ftp image binding" or so. Because it doesn't matter where the image is uploaded from, it does not necessarily be a network camera, right?
And if this binding will be this "generic image ftp binding" (we have to find a good name for it), I think it would make sense to not only offer ONE single image channel, but make the thing-type
extensible with more image channels of the same type. Your image channel would then get a configuration parameter "path" which is the ftp path where the image is uploaded to, so you can have multiple images per thing. WDYT?
@@ -0,0 +1,10 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<classpath> | |||
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.7"/> |
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.
JavaSE-1.8
|
||
<channels> | ||
<channel id="image" typeId="image-channel" /> | ||
<channel id="motion" typeId="motion-channel" /> |
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 thought you had removed this channel?
<channels> | ||
<channel id="image" typeId="image-channel" /> | ||
<channel id="motion" typeId="motion-channel" /> | ||
<channel id="motion-trigger" typeId="motion-trigger" /> |
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 think this channel is also kind of redundant, because you can obtain this information in rules by creating a condition whether the state of the item on the image channel has been changed, right?
<description>Image from network camera</description> | ||
<state readOnly="true"></state> | ||
</channel-type> | ||
<channel-type id="motion-channel"> |
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 remove
<description>Motion detected</description> | ||
<state readOnly="true"></state> | ||
</channel-type> | ||
<channel-type id="motion-trigger"> |
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.
See above, this can be removed too, don't you think?
if (configuration.userName.equals(userName)) { | ||
updateStatus(ThingStatus.ONLINE); | ||
updateState(IMAGE, new RawType(data, guessMimeTypeFromData(data))); | ||
triggerChannel(MOTION_TRIGGER, "MOTION_DETECTED"); |
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.
probably not needed anymore.
String strIdleTimeout = (String) componentContext.getProperties().get("idleTimeout"); | ||
|
||
if (StringUtils.isNotEmpty(strPort)) { | ||
port = Integer.valueOf(strPort); |
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.
What if you get a NumberFormatException
here? I think you should at least catch and log it and then use the default values.
} | ||
|
||
if (StringUtils.isNotEmpty(strPort)) { | ||
idleTimeout = Integer.valueOf(strIdleTimeout); |
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.
What if you get a NumberFormatException
here? I think you should at least catch and log it and then use the default values.
iterator.next().fileReceived(userName, data); | ||
} | ||
|
||
} catch (Exception 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.
Please do not catch Exception
, catch the specific ones.
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.
Callback could throw any kind of execution.
idleTimeout = Integer.valueOf(strIdleTimeout); | ||
} | ||
|
||
ftpServer.startServer(port, idleTimeout); |
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 method should throw an exception so you can catch it here. In the catch block you should notify all created handlers so they can set their things to OFFLINE with configuration_error.
Thanks @triller-telekom about the review. I think, most of the findings should be fixed now (I have not tested if binding still works). About the generic ftp image binding, do you think that there are many use cases for that? Images could be send to item via rest interface as well, right? Btw, static code analysis give me following errors:
Any idea? |
try { | ||
ftpServer.startServer(port, idleTimeout); | ||
} catch (FtpException e) { | ||
logger.warn("FTP server starting failed, reason: {}", 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.
As I mentioned before you should store all you created handlers in something like a Set
and notify them that the ftpserver is gone if this happens. So they can set their things to offline.
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 e.g. FTP server port can't be bind, FTP server throws IOException. As it's not catch, bundle activation fails and things stays in uninitialised state. There is no mechanism that ftp server could notify that server is suddenly shutdown. I don't know when ftp server could throw FtpException in start, but if I remove FtpException catching, things should stay Offline state if server throws that exception during startup.
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 think you should catch this Exception and not make the bundle initialization fail. This is an error case which can be treated by handling exception, so we should do it. Note that your modified
method is called if you change the binding configuration, so you start the server not only on the initial bundle start but also because of a changed configuration.
Regarding the static code analysis error: Can you please rebase your branch onto the latest master branch? |
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
@kaikreuzer, I quickly tested the custom channels.
When file is upload to FTP server, binding tries to find first image channel which match the filename. If any custom channel is not found, then binding updates the default image channel. Same for the triggers. Is that what you proposed? |
Yes, exactly! Isn't it nice and wasn't it simple? ;-) As mentioned above, you might want to allow regular expressions and not only static strings for the filename parameter. And note that you can define in the thing type that it can be extended by "image-channel"s (note the |
Signed-off-by: Pauli Anttila <[email protected]>
@kaikreuzer, yes, that was simple :) My fear was more related to FTP directory structures if that should be implemented. About the regular expression syntax, does thing file configuration syntax or Paper UI support some kind of auto escaping or how that should be implemented? By regex syntax, user can easily broke thing file. |
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
I didn't yet test what issues might come up with regexs as config parameter strings. But in general, both Paper UI and DSL should support any kind of string values for those parameters. For the DSL, only the quotes will need to be escaped, but besides that I would expect it to work. And the most simple regex is anyhow a static filename, so that case should be easily covered - for any other issues, you can happily create bug reports in ESH then ;-) |
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.
Except fro a few minor typos this binding looks good to me now. Please have a look at my comments. Since @kaikreuzer is also satisfied with the custom channels, I guess we are very close to get this binding merged :)
<config-description> | ||
<parameter name="filename" type="text" required="true"> | ||
<label>Filename</label> | ||
<description>Filename to match received files. Support regular expression patterns.</description> |
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.
Supports regular...
<config-description> | ||
<parameter name="filename" type="text" required="true"> | ||
<label>Filename</label> | ||
<description>Filename to match received files. Support regular expression patterns.</description> |
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.
Supports regular...
@@ -25,26 +25,54 @@ This binding currently supports following channels: | |||
|-----------------|--------------|----------------------------------------------------------------------------------------| | |||
| image | Image | Image received from network camera. | | |||
|
|||
Binding also support custom Image channels, where matching filename can be configured. |
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.
supports... where a matching filename can...
@@ -25,26 +25,54 @@ This binding currently supports following channels: | |||
|-----------------|--------------|----------------------------------------------------------------------------------------| | |||
| image | Image | Image received from network camera. | | |||
|
|||
Binding also support custom Image channels, where matching filename can be configured. | |||
When image file is uploaded to FTP server, binding tries to find channel which filename match to upload image filename. |
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.
When an image file.. server, the binding... find the channel which whose filename matches to the uploaded...
Binding also support custom Image channels, where matching filename can be configured. | ||
When image file is uploaded to FTP server, binding tries to find channel which filename match to upload image filename. | ||
If any direct match isn't found, the default image channel is updated. | ||
filename parameter support regular expression patterns. |
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 filename parameter supports...
ChannelUID channel = findCustomChannelByFilename(filename); | ||
updateState(channel != null ? channel.getId() : IMAGE, new RawType(data, guessMimeTypeFromData(data))); | ||
channel = findCustomTriggerByFilename(filename); | ||
triggerChannel(channel != null ? channel.getId() : MOTION_TRIGGER, "MOTION_DETECTED"); |
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.
Maybe your want to extract the string "MOTION_DETECTED"
into a constant.
import java.util.regex.Pattern; | ||
import java.util.regex.PatternSyntaxException; | ||
|
||
import org.eclipse.jdt.annotation.NonNull; |
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 do not use @NonNull
, see https://www.eclipse.org/smarthome/documentation/development/conventions.html
@@ -8,6 +8,8 @@ | |||
*/ | |||
package org.openhab.binding.networkcamera.internal.ftp; | |||
|
|||
import org.eclipse.jdt.annotation.NonNull; |
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.
See above.
<state readOnly="true"></state> | ||
</channel-type> | ||
<channel-type id="motion-trigger"> | ||
<channel-type id="image-received"> |
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.
Does this channel really check if the received file is an image? I see in your later commit you renamed the thing
to imagereceiver
, so you probably want to check in your implementation that the received file is an image file.
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.
Yes, I renamed thing to imagereceiver as only supported channel is a image channel. I also thought trigger channel name and in the beginning it was "file-received", but then I ended up to image-received because of thing name.
## Supported Things | ||
|
||
This binding supports ```motiondetection``` Thing. Every camera is identified by FTP user name. Therefore, every camera should use unique user name to login FTP server. | ||
This binding supports ```ftpupload``` Thing. Every thing is identified by FTP user name. Therefore, every thing should use unique user name to login FTP server. |
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 a line break after each sentence.
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
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.
Thank you for your changes. LGTM now!
@triller-telekom, hopefully all findings are now fixed. |
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 @paulianttila, please find some final comments attached.
# FTP Upload Binding | ||
|
||
This binding can be used to receive image files from FTP clients. | ||
Binding acts as a FTP server. |
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 binding...
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.
Done
## Supported Things | ||
|
||
This binding supports ```ftpupload``` Thing. | ||
Every thing is identified by FTP user name. |
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.
Every Thing...
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.
Done
|
||
## Supported Things | ||
|
||
This binding supports ```ftpupload``` Thing. |
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 binding supports Things of type ftpupload
.
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.
Done
did not receive this Content directly from the openHAB community, the following is provided | ||
for informational purposes only, and you should look to the Redistributor's license for | ||
terms and conditions of use.</p> | ||
<p><em> |
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 include 3 jars, so it would be good to have also 3 entries here - to me it is not clear, where ftpled comes from (FTP Server or from MINA?).
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.
Done
|
||
## Binding Configuration | ||
|
||
Bindings FTP server listening 2121 TCP port by default, but port can be configured. |
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 a table like this one so that the parameter names are clearly documented.
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.
Done
} | ||
|
||
private void initServer() throws FtpException { | ||
logger.info("Starting FTP server, port={}, idleTimeout={}", port, idleTimeout); |
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.
dito
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.
Done
|
||
serverFactory.addListener("default", listener); | ||
|
||
Map<String, Ftplet> ftplets = new LinkedHashMap<String, Ftplet>(); |
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.
use diamond operator
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.
Done
if (!e.getMessage().isEmpty()) { | ||
ftpStartUpErrorReason += ": " + e.getMessage(); | ||
} | ||
throw (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.
Doesn't throw e
do?
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.
Done
* Procedure for receive raw data from FTP server. | ||
* | ||
* @param userName | ||
* User name. |
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.
put the description on the same line as the parameter, please (all three)
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.
Done
@@ -38,6 +38,7 @@ | |||
<module>org.openhab.binding.folding</module> | |||
<module>org.openhab.binding.freebox</module> | |||
<module>org.openhab.binding.fronius</module> | |||
<module>org.openhab.binding.ftpupload</module> |
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 also add the binding to https://github.com/openhab/openhab2-addons/blob/master/features/openhab-addons/src/main/feature/feature.xml, otherwise it won't turn up in the distro.
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.
Done
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
@kaikreuzer, hopefully all findings are now fixed. |
Thanks @paulianttila! All fine, just please check the formatting of the XML (I wonder, why it does not apply tabs in your IDE...?) |
If you struggle with it, I can run it through my formatter, just let me know. |
Signed-off-by: Pauli Anttila <[email protected]>
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!
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
Signed-off-by: Pauli Anttila <[email protected]>
This binding integrates large number of network cameras which can send image to FTP server when motion or sound is detected. Binding acts as a FTP server.
Signed-off-by: Pauli Anttila [email protected]