Spotify usermod #4
Conversation
…hat dependency to build
WalkthroughThe pull request introduces a new Spotify usermod for the WLED project, enhancing the system's capabilities by integrating Spotify functionality. The changes include adding a new library dependency, creating a Spotify usermod class, registering the usermod, and updating various configuration files. The modifications involve adjusting library inclusion methods, adding compiler attributes to functions, and setting up the infrastructure to support Spotify integration within the existing WLED framework. Changes
Sequence DiagramsequenceDiagram
participant User
participant WLED
participant SpotifyUsermod
participant SpotifyAPI
User->>WLED: Configure Spotify settings
WLED->>SpotifyUsermod: Initialize
SpotifyUsermod->>SpotifyAPI: Authenticate
SpotifyAPI-->>SpotifyUsermod: Access Token
loop Every Second
SpotifyUsermod->>SpotifyAPI: Get Currently Playing
SpotifyAPI-->>SpotifyUsermod: Track Information
SpotifyUsermod->>WLED: Update State
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
usermods/Spotify/usermod_spotify.h (3)
17-104: Reduce verbosity inprintCurrentlyPlayingToSerial.
While it provides thorough debug output, consider wrapping debug prints in a toggleable debug flag to avoid spamming the serial console in production and to improve performance.
175-205: Check for potential WiFi reconnection.
During calls tospotify.getCurrentlyPlaying(...), if WiFi drops mid-request, consider retry or an error-handling flow to avoid indefinite waiting or repeated failures.
380-391: Ensure robust MQTT error handling.
This method silently returns if MQTT is unavailable. Consider logging in debug mode or toggling an internal flag to indicate that publishing is skipped.wled00/usermods_list.cpp (1)
421-424: Validate default usermod enabling.
The usermod is currently added with the parameterfalse, disabling it by default. If you intend Spotify to be active out-of-the-box, set this totrue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platformio.ini(4 hunks)usermods/Spotify/usermod_spotify.h(1 hunks)wled00/const.h(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/src/dependencies/json/AsyncJson-v6.h(1 hunks)wled00/usermods_list.cpp(2 hunks)wled00/wled.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wled00/wled.h
🔇 Additional comments (9)
wled00/src/dependencies/json/AsyncJson-v6.h (1)
15-15: LGTM! Include statement update follows best practices.The change from quotes to angle brackets for the ArduinoJson include is appropriate, as it correctly indicates this is an external library dependency rather than a local header.
wled00/fcn_declare.h (2)
143-143: LGTM! Include statement update follows best practices.The change from a relative path to angle brackets for ArduinoJson include is appropriate and consistent with other files.
Line range hint
58-58: Excellent optimization! Compiler attributes improve performance.The addition of
__attribute__((const))and__attribute__((pure))helps the compiler optimize these frequently called functions:
constfor pure mathematical functions (color_blend, color_add, approximateKelvinFromRGB)purefor functions that may read global memory (gamma8, gamma32, crc16)These attributes enable compiler optimizations like:
- Common subexpression elimination
- Loop optimization
- Constant folding
Also applies to: 59-59, 60-60, 75-75, 76-76, 77-77, 391-391
usermods/Spotify/usermod_spotify.h (2)
140-144: Validate initialization result.
Currently,setup()just sets the refresh token and doesn’t confirm or log if any required fields (e.g.clientId,clientSecret) are missing. Consider adding validation steps or logging errors if config values are empty.
320-326: Confirm the redirect URI and scope usage.
InappendConfigData(), building the Spotify login link is crucial. Verify the scope parameter and callback URI meet Spotify’s latest security and permissions requirements, preventing potential invalid redirect issues.wled00/usermods_list.cpp (1)
212-214: Optional compilation check.
Includingusermod_spotify.hwithin#ifdef USERMOD_SPOTIFYis correct. It might be useful to add compile-time messages to confirm Spotify usermod is enabled, helping troubleshoot any missing build flags.wled00/const.h (1)
146-146: New usermod ID looks good.
Registering a new ID for Spotify usermod helps avoid conflicts with existing usermods. No issues spotted here.platformio.ini (2)
1085-1087: LGTM! Spotify integration looks good.The Spotify library dependency is properly pinned to a specific commit hash (872e5c3) which ensures build reproducibility. The build flag
USERMOD_SPOTIFYis appropriately defined to enable the Spotify usermod.
237-237: Consider the implications of changing lib_ldf_mode.The change to
lib_ldf_mode = chain+may affect how library dependencies are resolved. While this can help with complex dependency trees, it might increase build times.Run this script to check if any library conflicts arise with the new mode:
✅ Verification successful
The lib_ldf_mode = chain+ setting is appropriate for WLED
This mode is well-suited for WLED's modular architecture and helps properly resolve hierarchical dependencies between core components and usermods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential library conflicts with chain+ mode # Look for multiple versions of the same library or circular dependencies # Search for library include statements rg -A 2 '#include' | grep -i 'spotify\|asynctcp\|espasyncwebserver' # List all library dependencies pio pkg listLength of output: 1254
| WiFiClientSecure client; | ||
| SpotifyArduino spotify(client, "", "", ""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate or privatize these global client instances.
Declaring a global WiFiClientSecure and SpotifyArduino instance at lines 14–15 can complicate testing and concurrency management. Consider moving them inside the SpotifyUsermod class to scope their usage properly:
| void connected() { | ||
| if (!enabled) return; | ||
| USER_PRINTLN("Connected to WiFi!"); | ||
| client.setCACert(spotify_server_cert); | ||
| initDone = true; | ||
| const char *refreshToken = NULL; | ||
| String callbackURI = "http://" + WiFi.localIP().toString() + "/settings/um?um=Spotify"; | ||
| if(false) { | ||
| // if(strcmp(refreshToken.c_str(), "") == 0) { | ||
| USER_PRINTLN("Fetch refresh token"); | ||
| refreshToken = spotify.requestAccessTokens("AQBhcQ4OCmawSPENF8ZWizBRjE9vawQvrOnpsZzzOy3NdsK37WrqyZSSBRK2toNCABH1AIPcXZSVK3H-ya77wS_bEcd85nX1XDYPFVKEEzIzy4klRqfg8RtCyR14a2xTY1ya7I_mmpJBvOb9ZEL0tfT97QNQu3_K8-XaBAPMxEfkAMyJYE_HTVn37I6Q5FIFDL5MmHHZvj3nV7C_u9aqd9dWASGLyv3tWhwi", callbackURI.c_str()); | ||
| spotify.setRefreshToken(refreshToken); | ||
| } | ||
|
|
||
| USER_PRINTLN("Refreshing Access Tokens"); | ||
| if (!spotify.refreshAccessToken()) { | ||
| USER_PRINTLN("Failed to get access tokens"); | ||
| } | ||
| USER_PRINTLN("SpotifyUsermod::connected completed"); | ||
| } |
There was a problem hiding this comment.
Harden token handling logic against null or invalid tokens.
If the refreshToken is missing or invalid, the code tries to request a new one. Ensure that the code doesn’t reveal sensitive tokens in logs, and gracefully handle refresh failures (e.g., temporarily disable the usermod).
Summary by CodeRabbit
New Features
Configuration Changes
Technical Improvements