V2.0.0 Initial Release: Complete Spotify Controller for ESP32 with UI and Album Art Support #1
Conversation
|
Wow, this is an impressive PR. I would love to share your experience on the ThingPulse blog, can you contact me? And please give me some time to test the PR out |
|
@Electric-Diversions Very nicely done, thanks! I am looking forward to taking this for a test ride. There's just a small thing that caught my attention while scrolling through the changes. @squix78 This started off at https://support.thingpulse.com/1543/esp32-spotify-remote-where-is-logic-to-show-song-information |
|
I'll also add this line (or similar) to the README.md usage instructions. I accidentally left it out.
|
|
@Electric-Diversions thank you again for your work! I noticed a couple of things:
Thank you |
|
@squix78 . thanks! Please see my responses below:
|
|
(in addition to Dani's feedback) @Electric-Diversions thanks again for this enormous contribution! I spent some time with this application today. ThingPulse users are often hobbyists rather than professional engineers or developers. To cater for such a user group, hardware and software has to be as simple as possible (but not simpler 😄). Furthermore, the apps should ideally have a high degree of compatibility within a chip family. I see some room for improvement in both areas. There's artificial complexity the app doesn't really need IMO.
|
|
Thanks for the feedback. I'm sorry it's been difficult getting the project running. I only have the current kit to test with and, having started with the Weather app, I don't think I ever tested it with a completely erased filesystem. I was able to recreate what Dani described though and I've now added a check for whether the filesystem is initialized and display a screen explaining the error. I understand that many users are hobbyists and I am open to making the project more accessible to a broader audience. However, the project was developed to meet a specific functional need, and I have limited capacity to refactor it into more of a demonstration. The configuration changes mentioned were all made based on necessity. As the codebase grew, the available space became limited, putting at risk the ability to add planned features. The custom file partition was introduced so that the art cache could be a decent size and improve performance. The inclusion of C++14 allowed me to get past issues I had creating the view framework. It may feel over-engineered and artificially complex, but these solutions addressed real limitations and allowed the project to fully use the capabilities advertised by ThingPulse. Rather than see this as a break with older generations of products, I encourage you to look at this as an example project that demonstrates how to fully tap into the potential of the Color Kit Grande as sold today. Regarding the encrypted WiFi, I'll be cleaning this up as part of the changes Dani requested. My original project actually used encrypted credentials and I stripped that out for the PR. I believe it is good privacy practice to minimize the surface area that credentials are exposed, but I understand the feeling it might be heavy. I think I have an idea that will allow the original settings.h approach while also offering a more advanced option for those who want more protection in this project or others. Much of this startup logic dates back to when I first got the kit working and it didn't receive much attention afterward. Can you help me with the CI build issue? The paths look correct to me, and it builds fine locally. Regarding the commented code, I will take a more aggressive stance at removing the comments and adding context for the ones I keep. I tried to base it on the original project's approach and leave comments that would help me and potentially others iterate over the code. Part of me worries I might need those comments later, but can't really defend keeping them beyond that. Update: Found the path issue. The commit has the wrong case for the Font folder. It doesn't affect me locally and VSCode matches the code. I'll fix it. |
No worries, it's been an interesting experience. It wasn't immediately obvious to me what was going on and the crash dumps gave only vague clues.
My primary motivation behind the feedback is keeping support effort for Dani low. Making all code public, free for anyone to use (customer or not), also means dealing with user support for years to come. Removing as many potential obstacles as possible does help. Also, I don't see my feedback as a list of tasks for you to fix. The better we understand your motivation behind doing things the way you did, the easier it is to assess what we might want to modify after merging.
Thanks for the hint. I didn't notice any performance issues while testing on my device so far but I'll pay more attention to this now. |
|
Thanks for the additional context. My first prototype was sluggish and this implementation is heavily optimized to provide a responsive, quick, and reliable device (once it is up and running). I use the device extensively and it sits right next to my monitor. I also tried to lay a foundation I could use in future ESP32/Color Kit Grande efforts. I didn't know there were 4MB Wrover-B kits in circulation and didn't originally plan to release the code as open source. Once I get the next batch of updates out, we can better align on ThingPulse's hopes for the project. We can also walk through the design in more detail and I can provide feedback on ways to fit it into earlier generation kits. The existing codebase may still fit at the lower memory limit (much of the space is taken up by the existing dependencies) and the file system can go back to 4MB by reducing the cache size from 60 albums to around 10. To reduce code complexity and size, there is logic that can be removed and still leave a functioning application — such as the extra views, performance monitoring instrumentation, cover art caching, build time logic, FileIO class, and the custom logger (which only exists because I couldn't get the log_x and ESP_LOGx macros to respect the tags and levels). I think these things are useful to advanced users, but I appreciate it increases the support footprint and may make the project less accessible. However, I think the biggest challenge for inexperienced developers will be the multi-threaded architecture. I didn't start out with that in mind, but it makes a big difference from an end-user experience perspective. |
|
I really appreciate you taking the time to engage in a discussion here. The code we end up maintaining here doesn't necessarily need to be the same running on your device i.e. living in your fork. It's fine if our priorities don't exactly align. I would say, push the changes you feel comfortable with, then we'll take it from there.
I don't think we need to change anything (apart from the board definition of course). The app works really well on mine so far.
They come with 4, 8 or 16MB each having 8MB PSRAM. The sizes are coded into the product identifier e.g. N4R8 → 4MB Flash, 8MB PSRAM.
Yes, that's a good example for what I call "artificial complexity". I'm quite confident your logging code is bug free and just works, but the only code not requiring any maintenance is code that isn't there 😜 You are right, the |
…m check, improved Spotify messaging
|
Well, I pretty much had all the changes in and just as I was doing one last round of testing after doing a 'Full Clean' (which I've done before) I started experiencing an issue setting the refresh token with SpotifyArduino. I know why the issue is happening in the library's code, but don't understand why I suddenly started getting it. I'm investigating. |
|
I also spotted an issue with the playback controls; not something I see consistently though IIRC. I didn't dig into this yet. |
|
Thx. Does the playback operation still perform from an end-user perspective (i.e., the skip takes place anyway)? If so, I think I've seen that logging from the very beginning but wasn't able to identify the cause. I kinda forgot it was doing that and assumed it was an issue with the library. If you see anything I am doing to cause it, please let me know. :) |
No, and this seems to correlate with the fact that the error occurs in So, I monitored the stats screen you built, and I found that we get dangerously close to heap exhaustion even in regular playback mode. The value oscillates between 70-something and 98%.
Once I silence the library's debug out (more about that below), with playback stopped when the device starts, I see this - maybe what you refer to? Side note 1: I'm pretty sure there's a bug in the library when it reports the stack size in debug mode; shouldn't be negative.
I still don't know, sorry. Still investigating... Side note 2: I haven't found a way to disable the library's debug mode other than to go into |
src/settings.h
Outdated
| ** timezone Europe/Zurich as per | ||
| ** https://github.com/nayarsystems/posix_tz_db/blob/master/zones.csv | ||
| */ | ||
| #define TIMEZONE "CST6CDT,M3.2.0,M11.1.0" |
There was a problem hiding this comment.
You changed the timezone but not the corresponding comment ("timezone Europe/Zurich as per...").
20:46:53.184 > [ 13836][I][util.cpp:58] initTime(): UTC time: 2025-06-09 18:46:53.
20:46:53.190 > [ 13842][I][util.cpp:97] setTimezone(): Setting timezone to 'CST6CDT,M3.2.0,M11.1.0'.
20:46:53.193 > [ 13850][I][SCLogger.cpp:94] logMessage(): [General] Current local time: 2025-06-09 13:46:53 [src/main.cpp:272][I] (loopTask)
Also, I feel it would make sense to reintroduce the #ifdef DATE_TIME_FORMAT_US-section in this file and use the format specifiers (→ HomeView::handleClock()). The time is currently displayed in US format.
| } | ||
| else | ||
| { | ||
| if (strcmp(currentlyPlaying.trackUri, _currentlyPlayingMetadata.trackUri) != 0) |
There was a problem hiding this comment.
This crash-reboots the device if you are on the Spotify free tier (i.e. not premium) when Spotify injects a commercial into the playback stream.
21:16:00.731 > Backtrace: 0x4008c3ac:0x3ffdb7b0 0x400d95ae:0x3ffdb7c0 0x400d967f:0x3ffdb880 0x400f560b:0x3ffdb920 0x400d925f:0x3ffdbc10 0x400d97b5:0x3ffdbc50
21:16:01.150 > #0 0x4008c3ac in strcmp at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/newlib/newlib/libc/machine/xtensa/strcmp.S:467
21:16:01.150 > #1 0x400d95ae in SpotifyPlayer::refreshCurrentSong(CurrentlyPlaying) at src/SpotifyPlayer.cpp:629
21:16:01.150 > #2 0x400d967f in SpotifyPlayer::getCurrentlyPlayingCallback(CurrentlyPlaying) at src/SpotifyPlayer.cpp:94
21:16:01.150 > #3 0x400f560b in SpotifyArduino::getCurrentlyPlaying(void (*)(CurrentlyPlaying), char const*) at .pio/libdeps/thingpulse-color-kit-grande/SpotifyArduino/src/SpotifyArduino.cpp:719
21:16:01.150 > #4 0x400d925f in SpotifyPlayer::refreshCurrentTrack() at src/SpotifyPlayer.cpp:438
21:16:01.150 > #5 0x400d97b5 in SpotifyPlayer::refreshCurrentSongTask(void*) at src/SpotifyPlayer.cpp:830
…eInit() with a global instance
|
I've pushed the latest batch of changes to the GitHub repository. I resolved the issue I was having yesterday. I'll continue testing them and maybe iterate over the documentation some. I can also provide more context on some of the decisions I made. I'm going to be less available over the next few days and wanted to get out what I have so far. Here is a recap of the changes:
|
|
@marcelstoer
|
|
Concerning the Spotify free tier, can you send me the SpotifyArduino json dump of an ad playing? I wonder how it differs from DJ X talking. Update 6/10: I suspect this code put at the top right after the first spLogI statement in // If not a song, update the UI accordingly and indicate music isn't available
if (currentlyPlaying.currentlyPlayingType != SpotifyPlayingType::track)
{
spLogI(LOGTAG_GENERAL, " _isMusicAvailable set to false. currentlyPlayingType is not track." );
_isMusicAvailable = false;
// force a refresh to get the waiting message
postScuiMessage(SCUIMessageType::UM_MARK_DIRTY,
"",
true);
return;
} |
|
Thanks for your comments!
I set the log level back to 5 and tested this again. I'm reluctant to draw much conclusion from the extra statements. However, it's reassuring that the error doesn't seem to be related to the handshake process. The only thing that sticks out to me is I also updated the platform to espressif32@~6.11.0 (up from 6.10); makes no difference. It's not possible for potentially concurrent requests to garble the SSL stack? The "next track" request still being processed while the next "fetch song info" request is issued?
As you mentioned in the update, it's I tested your proposed fix - all good 👍 |
|
Glad that fixed the Ad issue! I'll clean up and test the approach some and commit it to the PR.
I don't think so for these reasons:
UPDATE: Found this: witnessmenow/spotify-api-arduino#69 |
|
Your reasoning makes perfect sense. I'll investigate further by making "standalone" requests using the library in an isolated sketch.
I'll send an MR for that to the library project when I fix witnessmenow/spotify-api-arduino#76 |
|
Hmmm, I was planning on fixing the Ad issue with this: However, if you are going to add Ad as a supported type, I probably should explicitly check for Track and Episode instead. If you are feeling like contributing back the fix for the logging and adding the Ad type, I have another SpotifyArduino contribution for you if you are game. Could you initialize the pointer instance variables in the SpotifyArduiino header file: _refreshToken isn't initialized today and When |
👍 Being defensive here is more future proof anyway IMO. Spotify could extend the enum with a new value, which would break things for us if we don't explicitly check for "known to be working"-values.
Thanks, I created witnessmenow/spotify-api-arduino#77 to wrap this up. |
|
I have started adding support for US/Non-US date/time formatting on the Home and Clock views. |
|
I think I have everything in that was mentioned except for not using I realize that I didn't necessarily make the same design decisions that you would have. I do try to manage complexity, but at the end of the day, it is about what we're prioritizing. There is a lot going on in this codebase and being able to make sense of the logging based on function and noting which tasks are doing the logging adds a lot of value. Similarly, having the credentials in plain text on a filesystem that is backed up multiple times and also stored in the cloud can be more complex than just encrypting them and not worrying about it. While pulling out items to reduce code maintenance might make sense for you, I hope you will resist the urge until you see indications of a problem. Doing so will make it easier for me to contribute in the future. While I don't know what the future will hold, I plan to keep my copies running for as long as I can and am probably not done with the code. Here is a recap of everything that has been added/updated since I first created the PR.
|
This pull request contributes a fully functional and extensible implementation of the Spotify Controller for the ThingPulse Color Kit Grande. Built on the original prototype, this version introduces a complete UI system, album art caching, and a refined internal architecture.
Key Features:
• Spotify Playback Control: Play, pause, skip tracks from the touch display.
• Album Art Display & Caching: Downloads and locally caches artwork via Spotify Web API.
• Multiple UI Views: Home, Cover Art, Clock, and Diagnostics views with easy screen switching.
• Designed for ESP32: Dual-core support, touch handling, and PlatformIO/Arduino compatibility.
• Extensible & Modular: Built for growth with a clean architecture and support for additional UI modes.
This version preserves the ThingPulse startup branding and includes significant refactoring to improve maintainability, performance, and future development.