davepl
commented
4 months ago 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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670124552:
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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670126408:
@@ -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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670140836:
@@ -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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670143797:
+// 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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670179638:
- 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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670207668:
@@ -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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670218808:
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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670240112:
@@ -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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670251849:
- {
- 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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#discussion_r1670294736:
@@ -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 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/642#pullrequestreview-2165666125, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF7JO27D2V5OWGVGFEDZLO7Q5AVCNFSM6AAAAABKRSICOWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRVGY3DMMJSGU. You are receiving this because you authored the thread.
rbergen
robertlipe
prschguy1
Description
Moves all M5-based projects to the unified M5 stack
Fixes #582. Fixes #618.
mainas the target branch.