Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

TCke83
Copy link
Contributor

@TCke83 TCke83 commented Feb 22, 2023

@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.

@jimirocks
Copy link
Contributor

Hi, thanks for another contribution, will get into that in day or two - hope you are not in a hurry ;-)

@TCke83
Copy link
Contributor Author

TCke83 commented Feb 27, 2023

@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.

@jimirocks
Copy link
Contributor

@TCke83 Hi Tom, sorry for beeing late. Here are my thoughts on design (and I will also add few notes into the code):

  1. Because there is no way how to pair file request with file response on otherwise async websocket communication, there should be single point for getting APP (calling it APP since there is also FTP interface with lot of other files available) files from Miniserver. Therefore let's rename LoxoneIconRegistry.getIcon to LoxoneAppFiles.getFile
  2. I think this LoxoneAppFiles should be available straight through Loxone class
    • add Loxone.appFiles() method
    • the LoxoneAppFiles can take LoxoneWebSocket as constructor parameter => there will be no need for CommandRequestResponseListener (which feels weird anyway)
  3. Separate the image/file cache from the LoxoneAppFiles - similarly to TokenRepository introduce simple FileRepository interface and provide it's in memory implemntation for the beginning

Would you agree on this proposal?

Copy link
Contributor

@jimirocks jimirocks left a 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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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");
Copy link
Contributor

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)

Copy link
Contributor Author

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?

Copy link
Contributor

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"));
Copy link
Contributor

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 imagereference? If yes it could make sense to explicilty set this information in Command and behave according to that

Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

@jimirocks jimirocks Mar 14, 2023

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

Copy link
Contributor Author

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");
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

@jimirocks
Copy link
Contributor

Hi, sorry for not being able to progress on this due to other priorities. Is your proposal still relevant?

@jimirocks
Copy link
Contributor

Closing due to inactivity

@jimirocks jimirocks closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants