-
Notifications
You must be signed in to change notification settings - Fork 10
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
First draft for image download from miniserver #197
Conversation
Hi, thanks for another contribution, will get into that in day or two - hope you are not in a hurry ;-) |
@jimirocks not in a hurry from my point of view. Love the time/effort you guys invested in this library so happy to contribute to make it even more complete. Give me your thought about the implementation. |
@TCke83 Hi Tom, sorry for beeing late. Here are my thoughts on design (and I will also add few notes into the code):
Would you agree on this proposal? |
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.
Thx for contribution - things need to be adjusted and tried out.
@@ -82,8 +82,16 @@ public void onOpen(final ServerHandshake handshakedata) { | |||
public void onMessage(final String message) { | |||
LOG.trace("Incoming message " + message); | |||
final MessageHeader msgHeader = msgHeaderRef.getAndSet(null); | |||
if (msgHeader != null && msgHeader.getKind() != MessageKind.TEXT) { | |||
LOG.warn("Got text message but " + msgHeader.getKind() + " has been expected"); | |||
if (msgHeader != null) { |
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 am kind of surprised that binary content comes as text? Are you sure this shouldn't be in onMessage(ByteBuffer)
method? Or is it like SVG icons come as TEXT and PNG icons as ByteBuffer and both onMessage
methods need to be adjusted? Also it's a question whether we should work with possibly TEXT content as binary? Should the registry somehow support both or more possibilities?
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.
Your assumption is correct, when it is a text message it is an SVG file, when it is a png file it is a binary message. Will try with my miniserver to add a png file and test it out to be sure.
ws.processFile(message); | ||
return; | ||
} else { | ||
LOG.warn("Got of wrong type " + msgHeader.getKind() + " has been expected"); |
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.
before your change it tried to process even unexpected kind of incoming message, please do it the same way (the explicit return
won't be needed 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.
It seems to me it will still try to process it as a message event if it is not FILE or TEXT, what do you mean exactly?
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.
ahh you are right, but I would prefer to remove explicit returns and put the ws.processMessage
into else
branch
final Command<?> command = commands.remove(); | ||
if (!Void.class.equals(command.getResponseType())) { | ||
try { | ||
ByteBuffer byteBuffer = ByteBuffer.wrap(file.getBytes("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.
well it's a question why to wrap incoming String to ByteBuffer? It only brings encoding complexity in place
I guess it's possible to know whether it's text (SVG) or binary (PNG) from the image
reference? If yes it could make sense to explicilty set this information in Command and behave according to that
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.
Also the decision could be up to listener (registry) - making Command based on String
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've looked to the documentation and it seems you can download all svg images also as png image, will try to simulate that also on my miniserver.
iconLatch = new CountDownLatch(1); | ||
loxoneWebSocket.sendCommand(ImageCommand.genericControlCommand(iconUuid)); | ||
try { | ||
final int timeout = loxoneWebSocket.getAuthTimeoutSeconds() * loxoneWebSocket.getRetries() + 1; |
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.
do not use auth timeout, also no need for any retries?? I would recommend to use very short timeout for 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.
What timeout do you suggest? 1 seconde? Also I see no possibility at the moment to send a command without retry, should I add this to the LoxoneWebSocket class?
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.
well sorry, I wasn't very clear. No need to tweak retries.
Let's introduce possibly configurable separate timeout?
Or we can keep it like this for now - the more I am thinking about it I feel it would be better to add some blocking request-response method to LoxoneWebSocket instead... but I need more time to try out....
See #200
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.
Added a timeout for file retrieval with 1 second default timeout
if (iconLatch.await(timeout, TimeUnit.SECONDS)) { | ||
return iconMap.get(iconUuid); | ||
} else { | ||
LOG.error("Loxone icon wasn't fetched within timeout"); |
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 bad thing here is that if no file is received the command is pending in LoxoneWebsocket creating incosistency there and possibly breaking all the communication - this is not your issue, rather the design issue of the whole library.
Could you please find out what happens when somebody request a file which does not exist? Does miniserver answer anything or just nothing?
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've tried this and found that the miniserver responds with following message:
{"LL": { "control": "IconsFilled/unknown.svg", "value": "Bad request", "Code": "400"}}
This is a message with TEXT format, so picked up by the processMessage(final String message)
where it fails to convert the message to the ByteBuffer format specified by the command. I've tried to use LoxoneMessage as response type, but that causes the LoxoneAuth command listener to process the command. This class thinks the current token is invalid and discards this.
Not entirely clear to me if the response type of the command shoud be LoxoneMessage, this is only applicable if the command results in an error. When the file is found it answers with a FILE type command.
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.
thx for trying, I will look closer on that
Hi, sorry for not being able to progress on this due to other priorities. Is your proposal still relevant? |
Closing due to inactivity |
@jimirocks i've quicky made a first draft for downloading (and caching) images from the miniserver, could you take a look and give me your comments?
The caching part maybe needs to be more permanent (together with the App object, if the last modified timestamp doesn't change), but that will be another change i guess.