PlummersSoftwareLLC / NightDriverStrip

NightDriver client for ESP32
https://plummerssoftwarellc.github.io/NightDriverStrip/
GNU General Public License v3.0
1.29k stars 210 forks source link

Menagerie of changes - Stocks effect, AMOLED support, etc. #626

Closed davepl closed 1 month ago

davepl commented 1 month ago

Description

Adds a new Stock effect and the required API, as well as AMOLED S3 support

Contributing requirements

davepl commented 1 month ago

Good feedback!

The UNITY flag forces doubles to be the same size on the ESP32 and the x86 so that they can share data. There’s a Unity project that can render video and graphics and send them to the mesmerizer, but it only works if you spec the float size.

As to the specifics of size, they are lost to the ages now. Which is to say “I forget”!

On May 13, 2024, at 6:14 AM, Robert Lipe @.***> wrote:

@robertlipe approved this pull request.

Looks good! Nice use of modern C++ for time. Minor style bits for consistency.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598394778:

  • callback(StockData()); // HTTP request error
  • }
  • http.end(); // Close the connection properly
  • }
  • // GetAllQuotes
  • //
  • // Retrieves all quotes for the given list of symbols, provided as a comma-separated string
  • // Calls getQuote for each symbol in the list, and saves it in the stockData map
  • void GetAllQuotes(const String& symbols, StockDataCallback callback = nullptr)
  • {
  • // Lambda to split the symbols string by comma, because somehow this is cooler than strtok() was in 1989
  • // and I want to flex, but also because strtok() is not thread-safe and we're using threads here. So there.
  • // Also, I'm using std::string here because std::getline() is easier to use with std::string than with String. Fwiw, I have very similar code sitting in a branch for the same reasons. Expect to see something like this in utils some day.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598396032:

  • callback(StockData()); // Parsing error
  • }
  • }
  • else
  • {
  • Serial.printf("[HTTP] GET failed, error: %s\n", http.errorToString(httpCode).c_str());
  • if (callback)
  • callback(StockData()); // HTTP request error
  • }
  • http.end(); // Close the connection properly
  • }
  • // GetAllQuotes
  • //
  • // Retrieves all quotes for the given list of symbols, provided as a comma-separated string Please end sentences in periods throughout.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598399561:

+class PatternStocks : public LEDStripEffect +{

  • AnimatedText textSymbol = AnimatedText("STOCK", CRGB::White, &Apple5x7, 1.0f, MATRIX_WIDTH, 0, 0, 0);
  • AnimatedText textPrice = AnimatedText("PRICE", CRGB::Grey, &Apple5x7, 1.0f, MATRIX_WIDTH, 8, 0, 8);
  • AnimatedText textChange = AnimatedText("CHANGE", CRGB::White, &Apple5x7, 1.0f, MATRIX_WIDTH, 16, 0, 16);
  • AnimatedText textVolume = AnimatedText("VOLUME", CRGB::Grey, &Apple5x7, 1.0f, MATRIX_WIDTH, 24, 0, 24);
  • +private:

  • int iCurrentStock = 0;
  • bool isUpdating = false; // Flag to check if update is in progress
  • system_clock::time_point lastUpdate; // Time of last update
  • system_clock::time_point nextFetch = system_clock::now(); // Time of last quote pull
  • std::map<String, StockData> stockData; // map of stock symbols to quotes Bonus points for modern time handling. SO much easier to read than struct timeval.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598405012:

  • stockData.close = doc["close"].as();
  • stockData.volume = doc["volume"].as();
  • for (JsonVariant point : doc["points"].as())
  • {
  • StockPoint stockPoint;
  • stockPoint.dt = system_clock::from_time_t(point["dt"].as());
  • stockPoint.val = point["val"].as();
  • stockData.points.push_back(stockPoint);
  • }
  • if (callback)
  • callback(stockData); // Successful retrieval and parsing
  • }
  • else
  • {
  • Serial.printf("Failed to parse JSON: %s\n", error.c_str()); Consider DebugE so it shows in red.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598405664:

  • stockPoint.val = point["val"].as();
  • stockData.points.push_back(stockPoint);
  • }
  • if (callback)
  • callback(stockData); // Successful retrieval and parsing
  • }
  • else
  • {
  • Serial.printf("Failed to parse JSON: %s\n", error.c_str());
  • if (callback)
  • callback(StockData()); // Parsing error
  • }
  • }
  • else
  • {
  • Serial.printf("[HTTP] GET failed, error: %s\n", http.errorToString(httpCode).c_str()); Likewise.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598411553:

+

  • for (const String& symbol : symbolList)
  • {
  • GetQuote(symbol, [this, callback](const StockData& stockDataReceived)
  • {
  • if (!stockDataReceived.symbol.isEmpty()) // Check if the stock data is not empty
  • this->stockData[stockDataReceived.symbol] = stockDataReceived;
  • if (callback)
  • callback(stockDataReceived); // Optionally, call the callback for each symbol's data
  • });
  • }
  • }
  • // Should this effect show its title.
  • // The stocks effect does not show a title so it doesn't obscure our text display ? And stock's. Also, inconsistent period use.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598415815:

+

  • PatternStocks() : LEDStripEffect(EFFECT_MATRIX_STOCKS, "Stocks")
  • {
  • }
  • PatternStocks(const JsonObjectConst& jsonObject) : LEDStripEffect(jsonObject)
  • {
  • }
  • ~PatternStocks()
  • {
  • }
  • static void FetchQuotesTask(void *pvParameters)
  • {
  • auto instance = static_cast<PatternStocks>(pvParameters); // Cast the void pointer back to the correct type Musing: why can't auto deduce the right type here? Is the type really a Patternstocks**?

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598430413:

  • int w = MATRIX_WIDTH;
  • int h = MATRIX_HEIGHT - y;
  • int n = std::min((uint)MATRIX_WIDTH, currentStock.points.size());
  • if (n > 0)
  • {
  • float max = 0.0f;
  • float min = numeric_limits::max();
  • // We have the high and low data in the stock, but let's not trust it and calculate it ourselves
  • for (int i = 0; i < n; i++)
  • {
  • max = std::max(max, currentStock.points[i].val);
  • min = std::min(min, currentStock.points[i].val);
  • } Is this (min, max) = std::minmax_element(points.begin(), points.end()); That allows simd use and possible unrolling if size is known.

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598432873:

  • g()->drawLine(x0, y0, x1, breakevenY, CRGB::Green);
  • }
  • }
  • }
  • }
  • // Draw
  • //
  • // Draws the stock display made up of four animated text flyers (price, symbol, change, volume)
  • // and the stock history graph
  • void Draw() override
  • {
  • static uint lastCount = 0;
  • //static StockData stockData; Stray?

In include/effects/matrix/PatternStocks.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598433314:

+

  • // Draw
  • //
  • // Draws the stock display made up of four animated text flyers (price, symbol, change, volume)
  • // and the stock history graph
  • void Draw() override
  • {
  • static uint lastCount = 0;
  • //static StockData stockData;
  • g()->fillScreen(BLACK16);
  • g()->fillRect(0, 0, MATRIX_WIDTH, 9, g()->to16bit(CRGB(0,0,128)));
  • // Periodically refecth the stock data from the server Typos.

In include/gfxbase.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598438170:

 }
 void drawPixel(int16_t x, int16_t y, uint16_t color) override
 {
     if (isValidPixel(x, y))
         leds[XY(x, y)] = from16Bit(color);
  • else Should these just become asserts as they indicate a programming error?

In include/globals.h https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598442632:

@@ -1326,7 +1312,7 @@ extern RemoteDebug Debug; // Let everyone in the project know about it

     #define USE_M5DISPLAY 1                               // enable the M5's LCD screen
  • elif ESP32FEATHERTFT || PANLEE || LILYGOTDISPLAYS3

  • elif USE_TFTSPI || ESP32FEATHERTFT || PANLEE || LILYGOTDISPLAYS3

    Consider just moving use-tftspi into those three build definitions?

We have a constant tension of the build and the config trying to outsmart each other...

In platformio.ini https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598447559:

@@ -30,8 +30,8 @@ extra_configs = ; Options that are used (or extended) in all device sections (and hence environments) are defined here

[base] -upload_port = -monitor_port = +upload_port = +monitor_port = No trailing spaces, please. This causes diff thrash.

In platformio.ini https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#discussion_r1598449218:

+monitor_speed = 115200 +upload_speed = 1500000 +board_build.partitions = config/partitions_custom_noota.csv
+build_flags = ${base.build_flags}

  • -DLILYGO_TDISPLAY_AMOLED_SERIES=1
  • -DARDUINO_USB_CDC_ON_BOOT=1
  • -DAMOLED_S3=1
  • -DUSE_SCREEN=1
  • -DTOGGLE_BUTTON_1=21
  • -DTOGGLE_BUTTON_2=0
  • -DNUM_INFO_PAGES=2
  • -DLED_PIN0=2
  • -DLV_CONF_PATH="../../../../include/amoled/lv_conf.h"
  • -DLV_CONF_INCLUDE_SIMPLE
  • ${psram_flags.build_flags}
  • ${unity_double_flags.build_flags} What does this have to do with Unity?

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#pullrequestreview-2052567410, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCFYOVIQ4OJ4T5LXS3KLZCC4D7AVCNFSM6AAAAABHTMRH76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJSGU3DONBRGA. You are receiving this because you authored the thread.

davepl commented 1 month ago

Please end sentences in periods throughout.

Never! I’m a fast-coding rebel who plays by own rules, fast and loose. Consider DebugE so it shows in red.

Fixed! Likewise.

Also fixed. Also, inconsistent period use.

I’m not going back to prison… they won’t take me alive! Musing: why can't auto deduce the right type here? Is the type really a Patternstocks**?

Fixed Is this (min, max) = std::minmax_element(points.begin(), points.end()); That allows simd use and possible unrolling if size is known.

SimD likely doesn’t exist on the ESP32, but I’ve been wrong before. Stray?

Fixed!

Typos.

Fixed!!

Should these just become asserts as they indicate a programming error?

No, because DrawText calls it with out of bounds indices, and we don’t control DrawText, so we have to allow out of range. It’s the first case I couldn’t avoid. [base] -upload_port = -monitor_port = +upload_port = +monitor_port = No trailing spaces, please. This causes diff thrash.

Ignore whitespace is your friend, but point taken.

Cheers, Dave

davepl commented 1 month ago

You’ve said that three times without providing code. Show me the money.

On May 13, 2024, at 12:03 PM, Robert Lipe @.***> wrote:

@robertlipe commented on this pull request.

Esp32-s3 has simd.

Std:algorithm is almost always clearer to a reader and is usually computationally a win.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#pullrequestreview-2053519841, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF2TYGJBPRDMAO5LC5LZCEFB3AVCNFSM6AAAAABHTMRH76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJTGUYTSOBUGE. You are receiving this because you authored the thread.

Thermotronica commented 1 month ago

Sent from my iPhoneMessage ID: @.***>

robertlipe commented 1 month ago

Sorry I repeated my second message. I got distracted and your comment broke the threading, so I didn't see it when I got back to the thread so I wasn't sure I'd sent it.. (Though you're somewhat right - the compiler will not generate SIMD in this case. The person that ports the code to a more modern CPU can work that out.)

Given what a pain min() and max() are when mixing types (did you remember this when you coded the two casts or did you catch them later upon a compiler error?) it seems self-evident that letting STL handle these things is better ust by counting the lines in min_max1 and min_max2 below. minmax_element() will generate the right returns whether they're floats, doubles, ints, longs, strings, or anything that it knows how to compare intrinsically, and it accepts a comparator if you want to sort by something else, like sorting colors by brightness.

std::pair<float, float> min_max1(std::vector&v) { int n = std::min((int)MATRIX_WIDTH, (int)v.size());

    float max = 0.0f;
    float min = std::numeric_limits<float>::max();
    if (n > 0)
    {

        // We have the high and low data in the stock, but let's not

trust it and calculate it ourselves

        for (int i = 0; i < n; i++)
        {
            max = std::max(max, v[i]);
            min = std::min(min, v[i]);
        }
    }
    return std::pair{min, max};

}

include

// returns <float, float> auto min_max2(std::vector&v) { return std::minmax_element(begin(v), end(v)); }

// Test harness. std::vector fill_vec() { std::vector v(50); //srand(/(unsigned)/time(NULL)); // ick. std::generate(v.begin(), v.end(), std::rand); return v; }

include

int main() { auto v = fill_vec();

const auto [f1,f2] = min_max1(v); printf("1: %f, %f\n", f1, f2);

const auto [f3,f4] = min_max2(v); printf("2: %f, %f\n", f3, f4);

return 0; }

Duly noted that minmax() returns pointers to the two elements in the tuple. Since they might be large (see also: strings) new copies are not allocated. You can unpack with first() and last() if that tickles your fancy. If we wanted to mimic sorting only fhe first MATRIX_WIDTH, we could set the end element to (begin(v) + MATRIX_SIZE).

Efficiency was one of my points, and I'll concede that on modern Espressif, this example's a bust. (That's not always the case. Intel's demonstrated an

that will sometimes transparently spawn threads to walk large collections.) Hopefully you'll agree that std::algorithm versions of walking over loops are more readable. Now that i know we can apply "fast and loose rules", writing code and reviews should be more fun! On Mon, May 13, 2024 at 3:28 PM David W Plummer ***@***.***> wrote: > You’ve said that three times without providing code. Show me the money. > > > On May 13, 2024, at 12:03 PM, Robert Lipe ***@***.***> wrote: > > > > > > @robertlipe commented on this pull request. > > > > Esp32-s3 has simd. > > > > Std:algorithm is almost always clearer to a reader and is usually > computationally a win. > > > > — > > Reply to this email directly, view it on GitHub < > https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#pullrequestreview-2053519841>, > or unsubscribe < > https://github.com/notifications/unsubscribe-auth/AA4HCF2TYGJBPRDMAO5LC5LZCEFB3AVCNFSM6AAAAABHTMRH76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJTGUYTSOBUGE>. > > > You are receiving this because you authored the thread. > > > > — > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >
davepl commented 1 month ago

It’s more concise and likely more efficient, but no… not more readable to me! But I have an aversion to STL iterators, as much as I love the std algorithms.

But without looking it up, what does minmax even return? The min? The max? A tuple of minmax? Now I’ve got to go look it up, whereas the pedantic code was at least clear.

Totally a personal thing, but:

for (auto p : set) // Yay!

for (auto p = set.begin(); p!= set.end(); p++) // Less yay

Now if one could just write:

auto m = set.max(); // Super Yay!

…I’d be happy with that.

On May 13, 2024, at 5:21 PM, Robert Lipe @.***> wrote:

auto min_max2(std::vector&v) { return std::minmax_element(begin(v), end(v));

davepl commented 1 month ago

This is about as tidy as I can get it, because you get an intereator back, not a value.

auto [min_it, max_it] = std::minmax_element(points.begin(), points.end());

// Dereference the iterators to get the actual min and max values
int min = *min_it;
int max = *max_it;

If there was a way to write:

auto [min, max] = std::minmax_elements(points);

Then I’d be sold!

On May 13, 2024, at 5:21 PM, Robert Lipe @.***> wrote:

Sorry I repeated my second message. I got distracted and your comment broke the threading, so I didn't see it when I got back to the thread so I wasn't sure I'd sent it.. (Though you're somewhat right - the compiler will not generate SIMD in this case. The person that ports the code to a more modern CPU can work that out.)

Given what a pain min() and max() are when mixing types (did you remember this when you coded the two casts or did you catch them later upon a compiler error?) it seems self-evident that letting STL handle these things is better ust by counting the lines in min_max1 and min_max2 below. minmax_element() will generate the right returns whether they're floats, doubles, ints, longs, strings, or anything that it knows how to compare intrinsically, and it accepts a comparator if you want to sort by something else, like sorting colors by brightness.

std::pair<float, float> min_max1(std::vector&v) { int n = std::min((int)MATRIX_WIDTH, (int)v.size());

float max = 0.0f; float min = std::numeric_limits::max(); if (n > 0) {

// We have the high and low data in the stock, but let's not trust it and calculate it ourselves

for (int i = 0; i < n; i++) { max = std::max(max, v[i]); min = std::min(min, v[i]); } } return std::pair{min, max}; }

include

// returns <float, float> auto min_max2(std::vector&v) { return std::minmax_element(begin(v), end(v)); }

// Test harness. std::vector fill_vec() { std::vector v(50); //srand(/(unsigned)/time(NULL)); // ick. std::generate(v.begin(), v.end(), std::rand); return v; }

include

int main() { auto v = fill_vec();

const auto [f1,f2] = min_max1(v); printf("1: %f, %f\n", f1, f2);

const auto [f3,f4] = min_max2(v); printf("2: %f, %f\n", f3, f4);

return 0; }

Duly noted that minmax() returns pointers to the two elements in the tuple. Since they might be large (see also: strings) new copies are not allocated. You can unpack with first() and last() if that tickles your fancy. If we wanted to mimic sorting only fhe first MATRIX_WIDTH, we could set the end element to (begin(v) + MATRIX_SIZE).

Efficiency was one of my points, and I'll concede that on modern Espressif, this example's a bust. (That's not always the case. Intel's demonstrated an

that will sometimes transparently spawn threads to walk large collections.) Hopefully you'll agree that std::algorithm versions of walking over loops are more readable. Now that i know we can apply "fast and loose rules", writing code and reviews should be more fun! On Mon, May 13, 2024 at 3:28 PM David W Plummer ***@***.***> wrote: > You’ve said that three times without providing code. Show me the money. > > > On May 13, 2024, at 12:03 PM, Robert Lipe ***@***.***> wrote: > > > > > > @robertlipe commented on this pull request. > > > > Esp32-s3 has simd. > > > > Std:algorithm is almost always clearer to a reader and is usually > computationally a win. > > > > — > > Reply to this email directly, view it on GitHub < > https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#pullrequestreview-2053519841>, > or unsubscribe < > https://github.com/notifications/unsubscribe-auth/AA4HCF2TYGJBPRDMAO5LC5LZCEFB3AVCNFSM6AAAAABHTMRH76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJTGUYTSOBUGE>. > > > You are receiving this because you authored the thread. > > > > — > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub , or unsubscribe . You are receiving this because you authored the thread.
robertlipe commented 1 month ago

Indeed, there is a requirement to be proficient in the language one professes to program in. Does memmove(a, b, n) move from a to to a? I still look it up and that's been around a lot longer.

std::min_element returns the minimum. std::max_element returns the maximum. std::minmax_element returns a tuple with pointers to the first and last, as I described and for the reason I described. In RISC V or MIPS, for anything that fits in a register, they go in %R0 and %R1.

Since it's a personal thing, I can see that nobody's ever going to drag you toward modern C++, so I'll surrender.

Iterators weren't awesome, but there are data collections that aren't contiguous arrays and sometimes p->begin(); p->end(); p = p->next() is just what it takes. The nicer syntax (actually, the nicer nicer syntax that doesn't make a copy like yours does) for (const auto& p : set) is indeed nicer still. Sometimes new tricks are cool to learn. The denser syntax leaves less ambiguity what the programmer expressed to the compiler - whether that is what they MEANT or not, remains a problem, of course. :-)

I'm happy to use library functions because someone else has debugged them and we know at an instant what they're expected to do instead of sussing out that your loop looks at the first PIXELS width of the array for max and min.

I'm done with this one.

On Mon, May 13, 2024 at 7:31 PM David W Plummer @.***> wrote:

It’s more concise and likely more efficient, but no… not more readable to me! But I have an aversion to STL iterators, as much as I love the std algorithms.

But without looking it up, what does minmax even return? The min? The max? A tuple of minmax? Now I’ve got to go look it up, whereas the pedantic code was at least clear.

Totally a personal thing, but:

for (auto p : set) // Yay!

for (auto p = set.begin(); p!= set.end(); p++) // Less yay

Now if one could just write:

auto m = set.max(); // Super Yay!

…I’d be happy with that.

  • Dave

On May 13, 2024, at 5:21 PM, Robert Lipe @.***> wrote:

auto min_max2(std::vector&v) { return std::minmax_element(begin(v), end(v));

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/626#issuecomment-2109053865, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD36Z4Q3LHJ4Z7HSXCXTZCFLO7AVCNFSM6AAAAABHTMRH76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGA2TGOBWGU . You are receiving this because you were mentioned.Message ID: @.***>

rbergen commented 1 month ago

I've just pushed an update to this PR, which includes the following changes:

I note that the CI build of the heltecv3demo project started failing sometime during today - this is unrelated to the last bullet in the list of changes I just mentioned. The error message is that pins_arduino.h cannot be found for that project, and Google seems to say that this is always caused by an underlying problem (board or platformio.ini configuration).

This could be a temporary issue that is located and is to be fixed in a dependency. If it persists until the time this PR is ready to merge otherwise, I will exclude the heltecv3demo project from CI builds for the time being.

rbergen commented 1 month ago

I've removed the UPC block from the README as I suggested. As the heltecv3demo build still fails for a reason that must be rooted in a dependency, I've disabled CI for that env for the time being.

Barring any objections, I will merge this PR in its current state tomorrow (May 20, 2024) morning (GMT+2).