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

Device icon #2

Open
mwittig opened this issue Jun 9, 2015 · 10 comments
Open

Device icon #2

mwittig opened this issue Jun 9, 2015 · 10 comments

Comments

@mwittig
Copy link

mwittig commented Jun 9, 2015

Hi

As part of test-upnp.js a device icon is used but I can't figure out where the icon file needs to be placed - or do I need to implement serving the file?

Thanks
Marcus

@louaybassbouss
Copy link
Contributor

@mwittig there are two options:

  1. Host your icon on any web server in the web or local and use the absolute url of the icon
  2. extends the server implementation in upnp-test.js to serve the icon e.g. from using images/icon.png. in this case you can use the relative url.

@mwittig
Copy link
Author

mwittig commented Jun 9, 2015

Thanks for the swift reply. I think I'll shoot for option 2

@mwittig mwittig closed this as completed Jun 9, 2015
mwittig added a commit to mwittig/peer-upnp that referenced this issue Jun 9, 2015
@mwittig
Copy link
Author

mwittig commented Jun 9, 2015

Louay, I have reopened the issue as I realized that option 2. requires an extension of the peer-upnp code. I have drafted an implementation for this which handles both relative paths (appended to peer.prefix+"/device) and absolute paths. See mwittig@26443f3

Unfortunately, my IDE did some silly substitutions like removing blanks from empty lines. There are only two code blocks I have changed/extended which are lines 70-76 and lines 627-630. The code is used as part of pimatic-upnp-root, see _serveIcon() method at upnp-root.coffee. I haven't created a PR yet as this is just a draft for now. For the time being I am using the fork as part of pimatic.

@mwittig mwittig reopened this Jun 9, 2015
@louaybassbouss
Copy link
Contributor

@mwittig serving icons and other static files are not part of peer-upnp implementation. By creating a device you can pass url to the icon hosted on the same server used for peer-upnp or on any web server (in this case you need absolute url). to serve the icon from your server you need to extent upnp-test.js as following.

var fs = require("fs");
server.on("request",function(req,rsp){
        // ignore all request starting with /upnp
        // since they will be handled in peer-upnp.js
        if(req.url.indexOf("/upnp") != 0){
                // check if it is a request to get icon
                if(req.url == "/images/icon.png"){
                        // serve your icon to the client 
                        // in this example icon.png is located in same folder as upnp-test.js
                       var icon = fs.readFileSync('./icon.png');
                       rsp.writeHead(200, {'Content-Type': 'image/png' });
                       rsp.end(img, 'binary');
                }
                // for all other cases send not found
                else {
                        rsp.statusCode = 404;
                        rsp.end("Not found");
                }
        }
});

@mwittig
Copy link
Author

mwittig commented Jun 10, 2015

@louaybassbouss I have tried this before, but it does not work as you end up with two, competing request handlers where both may generate conflicting responses. The other request handler is triggered registerHTTPHandler() and it will create a 404 response. Well, actually, it depends on the icon url. If the icon path is not relative to the device URL, say it is "/icon", your code does not create a response (which I considered as a bug - see my PR), If the path is relative to the device URL, say "/upnp/device/icon" a 404 response is created.

NB, UPnP Device Architecture 2.0 states, the icon URL "Shall be relative to the URL at which the
device description is located in accordance with clause 5 of RFC 3986."
. Following this, a solution for icon URLs as part of peer-upnp would be nice.

@mwittig
Copy link
Author

mwittig commented Jun 10, 2015

Having thought about this further I think it all comes down to a general design decision. As you say "serving icons and other static files are not part of peer-upnp implementation" and I think I can deal with this following your code example. May be a side note in the README or an example for a device with icons would be helpful to other users. The solution still has a glitch as it is not possible to have icon URL like "icons/icon.png" as part of the device XML as this will be rendered to "/upnp/device/icons/icon.png" (in line with clause 5 of RFC 3986). Anyhow, I will try your code snippets and let know.

Btw. Could you please publish a new version of the package following the change done in PR-1? This would be great.

@louaybassbouss
Copy link
Contributor

@mwittig you have the control on the server in your application if you wish to deliver the icon e.g. from /upnp/device/icons/icon.png as you mentioned to follow the standard you can still do it in this way (insert the following code in upnp-test.js at line 30).

var fs = require("fs");
server.on("request",function(req,rsp){
        if(req.url.indexOf("/upnp") == 0){
                // check if it is a request to get icon
                if(req.url == "/upnp/device/icons/icon.png"){
                        // serve your icon to the client 
                        // in this example icon.png is located in same folder as upnp-test.js
                       var icon = fs.readFileSync('./icon.png');
                       rsp.writeHead(200, {'Content-Type': 'image/png' });
                       rsp.end(img, 'binary');
                }
                // all other requests to "/upnp/*" will be handled in peer-upnp
        }
       // for all other requests send not found (or you can handle other requests 
       // if needed if you use the server for other purposes in your application)
       else {
                    rsp.statusCode = 404;
                    rsp.end("Not found");
       }
});

in your PR you send an error code if it is not a request to '/upnp/*'. In this case the server is unusable in your application.

@mwittig
Copy link
Author

mwittig commented Jun 10, 2015

@louaybassbouss thanks for your reply. As far as I understand the code, the request handler as part of registerHTTPHandler() will handle requests for "/upnp/*" pathnames. It will trigger a 404 response if there is no handler for the given pathname. It is the case where you end up with the two competing responses. Anyhow, for my needs "/icons/icon.png" is OK and I think this is also in line with the UPnP device architecture.

@louaybassbouss
Copy link
Contributor

@mwittig yes in the second example you can catch the request before it is sent to peer-upnp. This is why important to register the handler before creating the Peer. in Node.js you can register multiple handlers on HttpServer, they will be chained until the first handler sends a response. all following handlers will be then ignored since you already sent a response to the client. I saw you are very active in the IoT domain (pimatic) can you tell me more about the project? Do you need support? you can contact me by mail [email protected]

mwittig added a commit to mwittig/peer-upnp that referenced this issue Jun 10, 2015
mwittig added a commit to mwittig/peer-upnp that referenced this issue Jun 10, 2015
@mwittig
Copy link
Author

mwittig commented Jun 12, 2015

@louaybassbouss I did not manage to get the suggested code changes to work, yet. As I am currently busy with other duties I'll get back to this early next week. I'll also send you an e-mail with summary of what I am doing :)

mwittig added a commit to mwittig/peer-upnp that referenced this issue Oct 1, 2015
mwittig added a commit to mwittig/peer-upnp that referenced this issue Oct 1, 2015
mwittig added a commit to mwittig/peer-upnp that referenced this issue Oct 1, 2015
mwittig added a commit to mwittig/peer-upnp that referenced this issue Jan 12, 2018
mwittig added a commit to mwittig/peer-upnp that referenced this issue Jan 12, 2018
mwittig added a commit to mwittig/peer-upnp that referenced this issue Jan 12, 2018
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

No branches or pull requests

2 participants