Add M5 Unified support (M5 Stick C Plus 2, M5 Stack Core 2)

[davepl]

Description

Moves all M5-based projects to the unified M5 stack

Fixes #582. Fixes #618.

• I read the contribution guidelines in CONTRIBUTING.md.
• I understand the BlinkenPerBit metric, and maximized it in this PR.
• I selected main as the target branch.
• All code herein is subjected to the license terms in COPYING.txt.

[robertlipe]

I had to consult the holy books (https://en.cppreference.com/w/cpp/iterator/size#Version_1) to convince myself they were the same, but I'd consider

ballColors.size() to be more in line with STL norms and would save you from having to make our own.

I don't feel strongly enough about this to tussle over it. If you hate it, don't even type the STFU.

[robertlipe]

I know you don't dig spelling corrections, but this effect (a.k.a. "larson scanner" or "larson lamps" as they were also used on K.I.T.T, also produced by Glen Larson) should probably at least publicly be called "Cylon"

https://galactica.fandom.com/wiki/Cylon

https://shop.evilmadscientist.com/productsmenu/152

If we want to have a REAL flamewar, let's take up plot holes in the sequel. In the early episodes, their spines glowed when they were aroused, yet nobody questioned why Professor Pinhead couldn't build an effective Cylon Detector! (I know... off topic. :-)

[robertlipe]

Odd. Is it still in std::experimental::to_array on yours? Have you run a pkg update lately?

I'm queasy rolling our own std::anything, but please wrap this inside an #if !defined(__cpp_lib_to_array) so we don't conflict with it when it shows up. (I've been using to_array and friends, but I'm living in the future.)

Citation for name (It's in the spec, too.):
https://en.cppreference.com/w/cpp/feature_test
Example usage:
https://gcc.gnu.org/pipermail/libstdc++-cvs/2024q1/040869.html

[robertlipe]

It's out of scope for this PR, but since we make reasonable use of https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/external-ram.html#configuring-external-ram
and I'e (a) quit calling psram_init() on every malloc in my own trees after moving it out of a debug print in main (oof!) and have run various strips (not so many matrix builds) for weeks/months without depletion, what would be the bar to eliminate all this "too clever" code and let
heap_caps_malloc_extmem_enable() just handle all this for us?

I'm willing to do some heavy lifting on this. Just LMK the acceptance criteria for build configurations and test patterns.

[robertlipe]

I know this was a yank and put, but can you please try to associate some kidn of absolute time with "very old"? Code grows older (we don't!) and it can be hard to reconstruct how hard this will be worth preserving in the year 2030 when there's a Mesmerizer in every home.

It doesn't need to be an exact date or a git hash or anything. Even "early 2018" will help our successors carbon-date this kind of code.

[robertlipe]

Can the optimizer trivially prove that CellCount doesn't change as a result of anything we may be doing inside these loops? It might be worth pulling it into a local so it doesn't have to recompute/re-fetch it if not.

[robertlipe]

Someone with access to hardware could earn valuable cleanup points replacing this V7-era ctime nonsense with something like (warning: freehanded code)
auto t = system_clock::now(); // should be a std::chrono::time_point
std::chrono::hh_mm_ss hms {t};
auto as_string = std::format("{}", hms);

I could be convinced to pair-program with someone that had appropriate hardware and wanted to do it together.

Those C-era time functions can't die from the planet soon enough for my taste.

[davepl]

Can’t believe I didn’t know how to spell Cylon :-). Whoops! I’ve only heard it on TV, not read it in books… Std::size should be used in place of ARRAYSIZE On the PSRAM, not sure what you’re advocating for. If you run your own MENUCONFIG for the ESP kit, you can set it to even do static data allocations out of PSRAM, but I was under the impression that since we use the Arduino version, we’re stuffed with the one and only MENUCONFIG setup they offer, so we have to do it ourselves. No? _ Dave

…

On Jul 9, 2024, at 4:03 AM, Robert Lipe ***@***.***> wrote: @robertlipe commented on this pull request. In include/effects/strip/bouncingballeffect.h <#642 (comment)>: > if (_bMirrored) - setPixelsOnAllChannels(_cLength-1-position, _cBallSize, Colors[i % ARRAYSIZE(ballColors)], true); + setPixelsOnAllChannels(_cLength-1-position, _cBallSize, Colors[i % std::size(ballColors)], true); I had to consult the holy books (https://en.cppreference.com/w/cpp/iterator/size#Version_1) to convince myself they were the same, but I'd consider ballColors.size() to be more in line with STL norms and would save you from having to make our own. I don't feel strongly enough about this to tussle over it. If you hate it, don't even type the STFU. In include/effects/strip/faneffects.h <#642 (comment)>: > @@ -1067,8 +1067,8 @@ class FireFanEffect : public LEDStripEffect { for (int i = 0; i < CellCount(); i++) { - int coolingAmount = random(0, Cooling); - abHeat[i] = ::max(0.0, abHeat[i] - coolingAmount * (2.0 - g_Analyzer._VURatio)); + float coolingAmount = random_range(0.0f, 2.0f); Is that a weird indent? In include/effects/strip/misceffects.h <#642 (comment)>: > @@ -503,6 +503,97 @@ class TwinkleEffect : public LEDStripEffect } }; +// SilonEffect I know you don't dig spelling corrections, but this effect (a.k.a. "larson scanner" or "larson lamps" as they were also used on K.I.T.T, also produced by Glen Larson) should probably at least publicly be called "Cylon" https://galactica.fandom.com/wiki/Cylon https://shop.evilmadscientist.com/productsmenu/152 If we want to have a REAL flamewar, let's take up plot holes in the sequel. In the early episodes, their spines glowed when they were aroused, yet nobody questioned why Professor Pinhead couldn't build an effective Cylon Detector! (I know... off topic. :-) In include/effects/strip/misceffects.h <#642 (comment)>: > +// A Battlestar Galactica inspired effect that moves red and green bars back and forth + +class SilonEffect : public LEDStripEffect +{ + public: + + SilonEffect() : LEDStripEffect(EFFECT_MATRIX_SILON, "SilonEffect") + { + } + + SilonEffect(const JsonObjectConst& jsonObject) + : LEDStripEffect(jsonObject) + { + } + + int _offset = 0; If _offset were unsigned, some of the math below gets simpler. In include/globals.h <#642 (comment)>: > + sizeDouble /= 1000; + ++suffixIndex; + } + + std::ostringstream oss; + oss << std::fixed << std::setprecision(precision) << sizeDouble << suffixes[suffixIndex]; + return String(oss.str().c_str()); +} + +// to_array +// +// Because the ESP32 compiler, as of this writing, doesn't have std::to_array, we provide our own (davepl). +// BUGBUG: Once we have compiler support we shoud use the C++20 versions + +template <typename T, std::size_t N> +constexpr std::array<T, N> to_array(const T (&arr)[N]) { Odd. Is it still in std::experimental::to_array on yours? Have you run a pkg update lately? I'm queasy rolling our own std::anything, but please wrap this inside an #if !defined(__cpp_lib_to_array) so we don't conflict with it when it shows up. (I've been using to_array and friends, but I'm living in the future.) Citation for name (It's in the spec, too.): https://en.cppreference.com/w/cpp/feature_test Example usage: https://gcc.gnu.org/pipermail/libstdc++-cvs/2024q1/040869.html In include/types.h <#642 (comment)>: > @@ -241,11 +241,23 @@ inline void * PreferPSRAMAlloc(size_t s) if (psramInit()) { debugV("PSRAM Array Request for %u bytes\n", s); - return ps_malloc(s); + auto p = ps_malloc(s); It's out of scope for this PR, but since we make reasonable use of https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/external-ram.html#configuring-external-ram and I'e (a) quit calling psram_init() on every malloc in my own trees after moving it out of a debug print in main (oof!) and have run various strips (not so many matrix builds) for weeks/months without depletion, what would be the bar to eliminate all this "too clever" code and let heap_caps_malloc_extmem_enable() just handle all this for us? I'm willing to do some heavy lifting on this. Just LMK the acceptance criteria for build configurations and test patterns. In src/network.cpp <#642 (comment)>: > debugV("ProcessIncomingData -- Channel: %u, Length: %u, Seconds: %llu, Micros: %llu ... ", channel16, length32, seconds, micros); + // The very old original implementation used channel numbers, not a mask, and only channel 0 was supported at that time, so if I know this was a yank and put, but can you please try to associate some kidn of absolute time with "very old"? Code grows older (we don't!) and it can be hard to reconstruct how hard this will be worth preserving in the year 2030 when there's a Mesmerizer in every home. It doesn't need to be an exact date or a git hash or anything. Even "early 2018" will help our successors carbon-date this kind of code. In include/effects/strip/faneffects.h <#642 (comment)>: > @@ -1067,8 +1067,8 @@ class FireFanEffect : public LEDStripEffect { for (int i = 0; i < CellCount(); i++) Can the optimizer trivially prove that CellCount doesn't change as a result of anything we may be doing inside these loops? It might be worth pulling it into a local so it doesn't have to recompute/re-fetch it if not. In include/effects/strip/misceffects.h <#642 (comment)>: > + { + setPixelOnAllChannels(_offset, y, CRGB::Red); + setPixelOnAllChannels(MATRIX_WIDTH - 1 - _offset, y, CRGB::Green); + } + } +}; + +// PDPGridEffect +// +// A Display for the front of the PDP-11/34 + +class PDPGridEffect : public LEDStripEffect +{ + public: + + PDPGridEffect() : LEDStripEffect(EFFECT_MATRIX_PDPGRID, "PDPGRIDEffect") PDPGridEffect would be more consistent with others. In src/screen.cpp <#642 (comment)>: > @@ -158,7 +158,7 @@ void BasicInfoSummary(bool bRedraw) time(&t); struct tm *tmp = localtime(&t); char szTime[16]; - strftime(szTime, ARRAYSIZE(szTime), "%H:%M:%S", tmp); + strftime(szTime, std::size(szTime), "%H:%M:%S", tmp); Someone with access to hardware could earn valuable cleanup points replacing this V7-era ctime nonsense with something like (warning: freehanded code) auto t = system_clock::now(); // should be a std::chrono::time_point std::chrono::hh_mm_ss hms {t}; auto as_string = std::format("{}", hms); I could be convinced to pair-program with someone that had appropriate hardware and wanted to do it together. Those C-era time functions can't die from the planet soon enough for my taste. — Reply to this email directly, view it on GitHub <#642 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF7JO27D2V5OWGVGFEDZLO7Q5AVCNFSM6AAAAABKRSICOWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRVGY3DMMJSGU>. You are receiving this because you authored the thread.

[rbergen]

Note: some of my comments are responses to @robertlipe's comments. I've been told by others that GitHub can make it difficult to follow these threads when consumed via email. So, it may be best (if not necessary?) to view the review on GitHub.

So I do have a few comments, but no deal-breakers as far as I'm concerned. I'll push an update to this PR to add an M5StickC Plus2 project to the web installer, and address some of my own (textual) comments in that.

@davepl I'll leave up to you which of Robert's and my remaining comments you want to process before you merge.

[rbergen]

Just noting here that the commits I just pushed address some of the review comments made. I will resolve the conversations in question.

[robertlipe]

Can’t believe I didn’t know how to spell Cylon :-). Whoops! I’ve only heard it on TV, not read it in books…

I'm not afraid to let my Geek Flag fly freely!

Std::size should be used in place of ARRAYSIZE

size() is valid for all C++. std::size is only >= 17. We're past 17 so either is OK. Are you decreeing this a style choice in the project? We have 62 occurrences of the other way. % ack '.size\(\)' src include | wc -l 62 ***@***.*** ~ % cat /tmp/x.cc #include <iostream> #include <array> int main() { std::array<int,10> a; std::cout << a.size(); std::cout << std::size(a); return 0; } ***@***.*** ~ % c++ --std=c++11 /tmp/x.cc /tmp/x.cc:7:21: error: no member named 'size' in namespace 'std' std::cout << std::size(a); ~~~~~^ 1 error generated. ***@***.*** ~ % c++ --std=c++14 /tmp/x.cc /tmp/x.cc:7:21: error: no member named 'size' in namespace 'std' std::cout << std::size(a); ~~~~~^ 1 error generated. ***@***.*** ~ % c++ --std=c++17 /tmp/x.cc ***@***.*** ~ % c++ --std=c++2a /tmp/x.cc ***@***.*** ~ % ./a.out 1010%

On the PSRAM, not sure what you’re advocating for. If you run your own MENUCONFIG for the ESP kit, you can set it to even do static data allocations out of PSRAM, but I was under the impression that since we use the Arduino version, we’re stuffed with the one and only MENUCONFIG setup they offer, so we have to do it ourselves. No?

It's a careful (and thorny) dance between the Arduino layer and the ESP layer, but based on considered study, I think you're wrong on this point. I've spent some time studying this and not totally certain - but it's not the layer that you and I, that worked at OS companies, think about where there's a "freetos.a" and an "libESP-IDF.a" that are linked that are pre-built and immutable. I'm also fuzzy on the lines between platformio, which is more than an IDE, and framework-arduino32 as they're cozy in weird ways. I haven't found it on link lines, for examperl We can't change the allocator use by the OS itself, but the MENUCONFIG is already set for all the boards; we just need to make better use of it. You need a slower computer so you can actually see the build. :-) $ pio run -v --target build -e yulc-demo and notice the end you'll see lots of Framework-Arduino goo being built. This stuff basically is a combination of the Arduino layer AND the ESP-IDF/FreeRTOS layer. Anything that's in ESP-IDF you can still call. You can see snippets of that all over. .platformio/packages/framework-arduinoespressif32/tools/sdk/esp32s3/include/freertos/port/xtensa/include/freertos/portmacro.h 2: * FreeRTOS Kernel V10.4.3 22: * http://www.FreeRTOS.org 70: * - The settings in this file configure FreeRTOS correctly for the given hardware and compiler. 95:/* Task function macros as described on the FreeRTOS.org WEB site. */ 103: * - Required by FreeRTOS This tight coupling is why they can't change ESP-IDF and Arduino independently, which is why we're currently on a GCC from 2018 or something. The build does a -I and -L all over the .espressif and ~/.platformio. If you look for .a's in ~/.espressif, you really only see "compiler stuff" - like approximately what was in the box for MSVC6 or something - libc, libm, libsupc++. Things like rainmaker and ESPNow are precompiled in ls ~/.platformio/packages/framework-arduinoespressif32-libs/esp32s3/lib, but ESP-IDF itself is MOSTLY built via inclusion inside ArduinoESP32. So we might not get to change the RAM allocator used by, say the ESPNOW llibrary but code like ~/.platformio/packages/framework-arduinoespressif32/cores/esp32/esp32-hal-psram.c goes through horrible ifdef torture to ensure that these are all set and used in anything compile inside Arduino itself or by us. So /Users/robertlipe/.platformio/packages/framework-arduinoespressif32/tools/sdk/esp32s3/dio_opi/include/sdkconfig.h 310:#define CONFIG_SPIRAM_USE_MALLOC 1 is already set for us and if you look inside that psram, you'll see it's abstracting ps_malloc() and friends into heap_caps_malloc and such for boardds that have such RAM. I'm going to change this code to quit alling psramInit() on every allocation. I'm going to make the psramInit() in main not be conditional debug print. ...and I'd like to you tell me, very specifically, what I can test to make the call to heap_caps_malloc_extmem_enable() in main::seteup() not be conditional for mesmerizer. I've fliplled that flag and I have tons of S3s and I telnet to them all the time and have never seen a reboot. If you relly mean color server or something, please give me a test case. I'll do the work. Let me know the stress test. ..and, yes, reading review discussions in GitHub is just a mess. I'm surprised that more ideas from Google's tools, used by so many now ex-Googlers, havne't escaped into the wild. :-) Mondrian and Critique rock!

…

Message ID: <PlummersSoftwareLLC/NightDriverStrip/pull/642/c2218144670@ github.com>

[prschguy1]

Hello Dave, thanks for your work on getting us to the new library. I had tried doing that, but had failed to get it working. That being said, it seems your spectrum display is processing artifacts, rather than actual audio. I have included a couple of photos. 1 in the fork preview you provided, and 1 loaded from 11/18/2023. Both are in glass containers, under a slight vacuum. As you can see, m5 plus, plus 2, and stack 2 are processing audio that just isn't there. I don't think this is something that can be addressed with noise floor settings. Hoping you can get this figured out. Additionally, I was using the original soundanalyzer.h files for m5 stack for other esp32 boards. Perhaps that code could be kept. and re-named as misc


-digital mic or other. Just my 2 cents.

Craig

[davepl]

Yup, I’m aware of it, and unfortunately do not have a good solution. Doesn’t mean there isn’t one, but someone with a bigger brain than mine might need to look at soundanalyzer.h Long story short, with the M5 Unified library, the levels never go down, they seem to auto-gain. It’s the same MIC… I’d rather not have multiple M5 impls, but if someone can get the old i2s code to work on all the M5s, that’d likely solve it and would be a good solution. Mostly I’d like to avoid the copout solution of “it works better on old M5s” and I guess I’m using that as a forcing function to get it fixed! - Dave

…

On Jul 14, 2024, at 2:25 PM, prschguy1 ***@***.***> wrote: Hello Dave, thanks for your work on getting us to the new library. I had tried doing that, but had failed to get it working. That being said, it seems your spectrum display is processing artifacts, rather than actual audio. I have included a couple of photos. 1 in the fork preview you provided, and 1 loaded from 11/18/2023. Both are in glass containers, under a slight vacuum. As you can see, m5 plus, plus 2, and stack 2 are processing audio that just isn't there. I don't think this is something that can be addressed with noise floor settings. Hoping you can get this figured out. Additionally, I was using the original soundanalyzer.h files for m5 stack for other esp32 boards. Perhaps that code could be kept. and re-named as misc 20240713_045324_resized.jpg (view on web) <https://github.com/user-attachments/assets/cb189761-051f-4af4-8105-cd2b737b7046> 20240713_051315_resized.jpg (view on web) <https://github.com/user-attachments/assets/9c5d1b88-ef33-4542-bcc7-5c425b12d4da> -digital mic or other. Just my 2 cents. Craig — Reply to this email directly, view it on GitHub <#642 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFZTF5RKIBZZUW5QPPLZMLUEPAVCNFSM6AAAAABKRSICOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRXGQ4DOMZWHA>. You are receiving this because you were mentioned.

[rbergen]

Merging after out-of-band alignment with @davepl.