FinianLandes / SpotifyEsp32

This is a library to connect to and control spotify from an esp
GNU Affero General Public License v3.0
9 stars 4 forks source link

Simplify integration into existing projects #4

Closed T-vK closed 3 months ago

T-vK commented 4 months ago

Hello and thanks for your great work on this library! :) I would like to use it in WLED (another open source project).

But I'm having a few issues:

  1. Fixed Redirect URI. In WLED http://{IP}/ is already reserved for the Web UI, so it can't be the Redirect URI. It would be nice to be able to specify a custom URL.
  2. Can't reuse existing HTTP server/client. WLED already has an HTTP server/client. It would make sense to be able to pass those to the SpotifyEsp32 constructor to save resources and avoid running servers on multiple ports.
  3. Dependent on Serial WLED is usually operated without a serial connection and is configured entirely via Wifi. Thus it would be nice if there was a way to retrieve the Refresh Token and Redirect URI programmatically instead of reading from serial. This way I could store the token across reboots and provide the user with the Redirect URI through the web UI.

What are your thoughts on that? :)

FinianLandes commented 4 months ago
  1. You can set the port of the webserver as parameter in the constructor so put some port over 8000 that should solve this problem.
  2. Will look into that for the next update. You are talking about the server getting create with the WebServer library right?
  3. Im already working on something like that so that will be included in the next update aswell. That update will probably be released sometime next week. Thank you for the feedback
T-vK commented 4 months ago

Thanks for the quick reply! That sounds great! :)

One more thing I would like to see, is support for /get-audio-analysis. The responses can be like 400kB in size though so it could easily be too much for an ESP. Do you think we could add support for ArduinoJson's filter? That combined with using the response streams instead of strings, would allow us to only use minimal amounts of memory. For example if I just need meta.timestamp, beats and bars from the response, it would be awesome if I could pass a filter like:

JsonDocument filter;
filter["meta"]["timestamp"] = true;
filter["beats"] = true;
filter["bars"] = true;

String trackId = "11dFghVXANMlKmJXsNCbNl";

response data = sp.get_audio_analysis(trackId, DeserializationOption::Filter(filter));

It seems that currently the String interface is used:

DeserializationError deserializeJson(JsonDocument& doc, const String& input);

String means that we load the entire response body into memory before parsing it. Using the Stream interface would allow to parse the response while it is getting received, only storing the required properties in memory. Meaning much lower memory consumption when used with filter.

DeserializationError deserializeJson(JsonDocument& doc, Stream& input);
FinianLandes commented 4 months ago

Will have to look into that, but sure should be possible, and sensefull, potentially for all get endpoints, will look to get the other things into version 1.1.x and this will then be in 1.2.x(probably out in 1-2 weeks) if you need it fast feel free to make a pull request and implement it as you like ill then look over it and merge it

T-vK commented 4 months ago

I agree. It would probably make sense to allow a filter for all methods and pass it down to Spotify::RestApi where it can be passed to deserializeJson then. I'll try to implement it and keep you posted. If I succeed, I'll create a PR. Switching from String to Stream might however require a more in-depth refactoring.

FinianLandes commented 4 months ago

Perfect, otherwise ill try it then, one thing please keep the naming in style, so x_x etc.

T-vK commented 4 months ago

Of course! :)

FinianLandes commented 4 months ago

"Can't reuse existing HTTP server/client. WLED already has an HTTP server/client. It would make sense to be able to pass those to the SpotifyEsp32 constructor to save resources and avoid running servers on multiple ports." currently looking into that, what webserver does WLED use? the one from the WebServer.h lib i use too? As im not sure how i should handle the closing/the server.handleClient() command

T-vK commented 4 months ago

WLED mostly uses ESPAsyncWebServer for receiving requests as a server. I also found references to AsyncJson which from my understanding provides a way of sending JSON responses, but I think it's probably not relevant for the integration of SpotifyEsp32.

Here's an example of how it's used and here's another one. The server is globally defined here.

I'll check with the people from WLED if there is also a standard/recommended way of how they send HTTP requests as a client.

FinianLandes commented 4 months ago

Ok thanks, then ill leave that away for now as i use WebServer and the thing is if you save your tokens the webserver will only be used once and then not again, so its not that big big of a size issue i think.

FinianLandes commented 3 months ago

Were you able to get anything done with filters? As i now have released version 1.1 which got some new features on how to login, so i could go on to work on that...

T-vK commented 3 months ago

I have tried, but I haven't gotten far. It would probably take me another week or so to get it working. So if you have the time, go for it. I'm sure I'll find something else to contribute down the road. :)

FinianLandes commented 3 months ago

No problem, ill try to do it, will dind out how long itll take me to do it as ive never used these features befor but will find it out:)