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.

Sign up for GitHub

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

Jump to bottom

Rework playlist generation #275

Merged
merged 11 commits into from
Feb 15, 2024
Merged

Rework playlist generation #275

merged 11 commits into from
Feb 15, 2024

Conversation

laszloh
Copy link
Contributor

@laszloh laszloh commented Dec 5, 2023

This PR updates the generation of the playlist. It changes the container from a char** array to a std::vector<char*> to reduce the housekeeping overhead.

It moves multiple functions to their modern implementations to use the functions provided by stdlib (f.e. randomizing and sorting the playlist).

This is the 1st of 3 PR to move the whole playlist handling to stdlib and modern pointer management.
This PR is not depending on any of the other 2 PRs

For the discussion see: https://forum.espuino.de/t/vorstellung-playlist-optimierung/1890/17

@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch from 66e850c to fc8bce3 Compare December 5, 2023 08:38
@laszloh laszloh marked this pull request as draft December 5, 2023 08:39
@laszloh laszloh marked this pull request as ready for review December 5, 2023 08:46
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch from 84b0ea9 to f54107f Compare December 5, 2023 09:02
@tueddy
Copy link
Collaborator

tueddy commented Dec 5, 2023

Looks good so far!
But i got this crash:

D [10167] ws[/ws][1] connect
Guru Meditation Error: Core 1 panic'ed (LoadProhibited). Exception was unhandled.

Core 1 register dump:
PC : 0x400e2354 PS : 0x00060e30 A0 : 0x800e3a2d A1 : 0x3ffee4a0
A2 : 0x00000000 A3 : 0x3ffee500 A4 : 0x3ffee918 A5 : 0x3f80fa98
A6 : 0x3ffcc888 A7 : 0x3ffc4260 A8 : 0x00000000 A9 : 0x00000000
A10 : 0x3ffee918 A11 : 0x0000000a A12 : 0x3ffee500 A13 : 0x00000009
A14 : 0x00000020 A15 : 0x003fffff SAR : 0x00000011 EXCCAUSE: 0x0000001c
EXCVADDR: 0x00000004 LBEG : 0x400949cd LEND : 0x400949dd LCOUNT : 0xfffffffd

Backtrace: 0x400e2351:0x3ffee4a0 0x400e3a2a:0x3ffee950 0x400e3ca7:0x3ffeebf0 0x400e3d73:0x3ffeecb0 0x402255fb:0x3ffeecd0 0x402270c1:0x3ffeecf0 0x40115d25:0x3ffeed40 0x40115d49:0x3ffeed80 0x4020aa7a:0x3ffeeda0 0x4020aaf9:0x3ffeedd0 0x4020b37e:0x3ffeedf0

#0 0x400e2351:0x3ffee4a0 in ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*>::getOrCreateData() const at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Object/MemberProxy.hpp:54
(inlined by) ArduinoJson::V6213PB2::detail::VariantData* ArduinoJson::V6213PB2::detail::VariantAttorney::getOrCreateData<ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*> const>(ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*> const&) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Variant/VariantAttorney.hpp:44
(inlined by) ArduinoJson::V6213PB2::detail::VariantRefBase<ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*> >::getOrCreateData() const at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Variant/VariantRefBase.hpp:285
(inlined by) ArduinoJson::V6213PB2::detail::VariantRefBase<ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*> >::getOrCreateVariant() const at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Variant/VariantImpl.hpp:120
(inlined by) bool ArduinoJson::V6213PB2::detail::VariantRefBase<ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*> >::set(unsigned int const&) const at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Variant/VariantRefBase.hpp:126
(inlined by) ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*>& ArduinoJson::V6213PB2::detail::MemberProxy<ArduinoJson::V6213PB2::JsonObject, char const*>::operator=(unsigned int const&) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ArduinoJson/src/ArduinoJson/Object/MemberProxy.hpp:33
(inlined by) Web_SendWebsocketData(unsigned int, unsigned char) at src/Web.cpp:1008
#1 0x400e3a2a:0x3ffee950 in _ZL14JSONToSettingsN11ArduinoJson8V6213PB210JsonObjectE$isra$247 at src/Web.cpp:712
#2 0x400e3ca7:0x3ffeebf0 in processJsonRequest(char*) at src/Web.cpp:964
#3 0x400e3d73:0x3ffeecb0 in onWebsocketEvent(AsyncWebSocket*, AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int) at src/Web.cpp:1064
(inlined by) onWebsocketEvent at src/Web.cpp:1041
#4 0x402255fb:0x3ffeecd0 in std::_Function_handler<void (AsyncWebSocket*, AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int), void ()(AsyncWebSocket, AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int)>::_M_invoke(std::_Any_data const&, AsyncWebSocket*&&, AsyncWebSocketClient*&&, AwsEventType&&, void*&&, unsigned char*&&, unsigned int&&) at c:\users\dcars.platformio\packages\toolchain-xtensa-esp32\xtensa-esp32-elf\include\c++\8.4.0\bits/std_function.h:297
#5 0x402270c1:0x3ffeecf0 in std::function<void (AsyncWebSocket*, AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int)>::operator()(AsyncWebSocket*, AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int) const at c:\users\dcars.platformio\packages\toolchain-xtensa-esp32\xtensa-esp32-elf\include\c++\8.4.0\bits/std_function.h:687
(inlined by) AsyncWebSocket::_handleEvent(AsyncWebSocketClient*, AwsEventType, void*, unsigned char*, unsigned int) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ESP Async WebServer/src/AsyncWebSocket.cpp:864
#6 0x40115d25:0x3ffeed40 in AsyncWebSocketClient::_onData(void*, unsigned int) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ESP Async WebServer/src/AsyncWebSocket.cpp:684
#7 0x40115d49:0x3ffeed80 in std::_Function_handler<void (void*, AsyncClient*, void*, unsigned int), AsyncWebSocketClient::AsyncWebSocketClient(AsyncWebServerRequest*, AsyncWebSocket*)::{lambda(void*, AsyncClient*, void*, unsigned int)#7}>::_M_invoke(std::_Any_data const&, void*&&, AsyncClient*&&, std::_Any_data const&, unsigned int&&) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/ESP Async WebServer/src/AsyncWebSocket.cpp:483
(inlined by) _M_invoke at c:\users\dcars.platformio\packages\toolchain-xtensa-esp32\xtensa-esp32-elf\include\c++\8.4.0\bits/std_function.h:297
#8 0x4020aa7a:0x3ffeeda0 in std::function<void (void*, AsyncClient*, void*, unsigned int)>::operator()(void*, AsyncClient*, void*, unsigned int) const at c:\users\dcars.platformio\packages\toolchain-xtensa-esp32\xtensa-esp32-elf\include\c++\8.4.0\bits/std_function.h:687
(inlined by) AsyncClient::_recv(tcp_pcb*, pbuf*, signed char) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/AsyncTCP@src-b1b5fe1257067f03f2d57f92cc6925a2/src/AsyncTCP.cpp:934
#9 0x4020aaf9:0x3ffeedd0 in AsyncClient::_s_recv(void*, tcp_pcb*, pbuf*, signed char) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/AsyncTCP@src-b1b5fe1257067f03f2d57f92cc6925a2/src/AsyncTCP.cpp:1210
#10 0x4020b37e:0x3ffeedf0 in _async_service_task(void*) at .pio/libdeps/lolin_d32_pro_sdmmc_pe/AsyncTCP@src-b1b5fe1257067f03f2d57f92cc6925a2/src/AsyncTCP.cpp:162
(inlined by) _async_service_task at .pio/libdeps/lolin_d32_pro_sdmmc_pe/AsyncTCP@src-b1b5fe1257067f03f2d57f92cc6925a2/src/AsyncTCP.cpp:197

I assume it's access to gPlayProperties.playlist->size() while playlist is null-pointer (not yet created)

@laszloh
Copy link
Contributor Author

laszloh commented Dec 5, 2023

Yes you are right. Playlist was zeroed by setup() and the creation of a dummy playlist was (propably) lost while I rebased.

This commit fixes the problem

tueddy
tueddy reviewed Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@tueddy tueddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/Web.cpp Outdated Show resolved Hide resolved
src/AudioPlayer.cpp Outdated Show resolved Hide resolved
src/AudioPlayer.cpp Outdated Show resolved Hide resolved
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch 5 times, most recently from 8d9fa2e to 6282d8e Compare December 14, 2023 14:05
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch from 6282d8e to aae3fcc Compare January 18, 2024 15:37
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch from aae3fcc to 19c3500 Compare January 29, 2024 15:42
@laszloh
Rename and adapt playlist destruction function
90e4229 
Rename freeMultiCharArray to freePlaylist and specialise it so that it can be used to destroy the content of a playlist.
Move all playlist related functions into it's own header
@laszloh
Optimise playlist generation
431a99c 
Use std::vector instead of POD char* array so that we do not have to care about memory managment.
We still have to take care of the strings in the playlist manually.

Make x_malloc more forgiving by prefereing the PSRAM and not enforcing it.
@laszloh
Change playlist to std::vector pointer
06e73a3
@laszloh
rework SdCard_pickRandomSubdirectory
dbcaf00 
Rework SdCard_pickRandomSubdirectory to fir into the new playlist generator.
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch 3 times, most recently from a84ea5f to dea89a2 Compare February 13, 2024 08:50
@tueddy
Copy link
Collaborator

tueddy commented Feb 13, 2024

Tested & LGTM!
Just a small spell correction for "unsing":
I [83307] Sorting files unsing case-insensitive natural sorting

@laszloh
Rework TrackQueueDispatcher
9d33a74 
Make the TrackQueueDispatcher aware of the new std::vector playlist.
Adapt FreeRTOS queue to use Playlist pointers.
Improve log output of sorting function.
@laszloh
Make the AudioPlayer Task aware of the std::vector
9fb24ca 
Fix compile errors when accessing playlist std::vector. Rework the shuffle and sort function to use the std-lib.
@laszloh
Make cover handler aware of the std::vector
8874782
@laszloh
Remove numberOfTracks and use vector size instead
7a67e3b 
Remove the numberOfTracks, since it's redundant with std::vector->size()
@laszloh
Rework and improve fileValid
fe9e31a
@laszloh
Alway supply an empty Playlist
fa74265 
Make sure there is always a dummy playlist and move initialization of gPlayProperties into the AudioPlayer Task.
Fix formatting issues and the review points

- Add guards to prevent access to playlist if pointer is nullptr. This will be improved in PR 2 of 3 by encapsulating playlist and preventing direct access.
- remove commented code parts
- fix spelling
@laszloh
Reduce logging of invalid files
6d6a357 
Comment the logging of invalid files to reduce the log spam. Add logging of the number of ignored files in the playlist generation.
@laszloh laszloh force-pushed the feature/playlist-optimization-generation branch from dea89a2 to 6d6a357 Compare February 14, 2024 09:46
@laszloh
Copy link
Contributor Author

laszloh commented Feb 14, 2024

Ah classic me. Typo is fixed.

@tueddy tueddy merged commit ab0bcb3 into biologist79:dev Feb 15, 2024
10 checks passed
@laszloh laszloh deleted the feature/playlist-optimization-generation branch February 16, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tueddy tueddy tueddy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@laszloh @tueddy