Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR converts the SoundAnalyzer from a runtime-configurable class to a template-based approach with compile-time microphone type selection. It also standardizes TFT_eSPI configuration to use proper setup headers instead of manual platformio.ini definitions.
- Replaces runtime microphone type switching with compile-time template parameters
- Updates TFT_eSPI configuration to use proper User_Setup includes
- Refactors PeakData from custom class to std::array for better performance
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| include/soundanalyzer.h | Major template refactor - converts SoundAnalyzer to template class with compile-time microphone parameters |
| src/audio.cpp | Updates to use new templated SoundAnalyzer type and increases max FPS from 45 to 60 |
| src/main.cpp | Removes global SoundAnalyzer declaration (now handled in audio.cpp) |
| src/network.cpp | Updates remote peak data handling to use new PeakData array interface |
| src/drawing.cpp | Updates VU meter logic to use new IsRemoteAudioActive() method |
| include/screen.h | Adds conditional TFT_eSPI setup includes for TTGO devices |
| include/globals.h | Adds custom std::sum implementation and TFT_eSPI setup logic |
| platformio.ini | Extensive updates to TFT configuration flags and project-specific settings |
| src/screen.cpp | Screen rendering improvements including FPS throttling and display threshold adjustments |
| src/effects.cpp | Removes duplicate DEFAULT_EFFECT_INTERVAL definition |
| src/deviceconfig.cpp | Fixes missing brace in conditional block |
| include/effects/strip/musiceffect.h | Updates to use array access syntax for PeakData |
| include/effects/strip/fireeffect.h | Updates to use new std::sum function |
| include/effects/matrix/spectrumeffects.h | Updates to use new IsRemoteAudioActive() method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // sum helper | ||
| // Provides std::sum for containers/ranges and iterator pairs. | ||
| // Note: Adding to namespace std is a deliberate convenience for this project. | ||
| namespace std { |
There was a problem hiding this comment.
Adding functions to the std namespace is undefined behavior according to the C++ standard. Consider using a custom namespace like 'nds' or 'nightdriver' instead.
Suggested change
| namespace std { | |
| // Note: Adding to namespace std is undefined behavior. Use a custom namespace instead. | |
| namespace nightdriver { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
robertlipe
approved these changes
Aug 17, 2025
robertlipe
left a comment
robertlipe
left a comment
Collaborator
There was a problem hiding this comment.
I'm unqualified to comment on the signal processing side, but just reviewing the normal programmer-ry stuff, there are some things I'd like you to at least reconsider, but LGTM.
| auto begin = &heat[i * CellsPerLED]; | ||
| auto end = begin + CellsPerLED; | ||
| int sum = std::accumulate(begin, end, 0); | ||
| int sum = std::sum(begin, end); |
Collaborator
There was a problem hiding this comment.
I think you're going backward. ::sum was proposed and never ratified in favof of ::accumulate, I think. cppreference.com, which I trust highly, lists accumulate (used correctly before) and partial_sum
|
|
||
| // sum helper | ||
| // Provides std::sum for containers/ranges and iterator pairs. | ||
| // Note: Adding to namespace std is a deliberate convenience for this project. |
Collaborator
There was a problem hiding this comment.
I think you've recreated accumulate()
I wonder if our ancient c++ has some bit of C++20 goodness that you wanted that just isn't there yet. Buzz me.
| #define DISABLE_ALL_LIBRARY_WARNINGS 1 | ||
| // If the project provides its own TFT_eSPI setup via USER_SETUP_LOADED | ||
| // and build_flags, do not include the TTGO default user setup to avoid | ||
| // macro redefinition warnings. |
Collaborator
There was a problem hiding this comment.
Can we please keep this stuff out of globals? Plop it into screen.cc or something of smaller scope.
| #endif | ||
|
|
||
| #if USE_TFTSPI | ||
| #if TTGO |
Collaborator
There was a problem hiding this comment.
Same file, different scope, different manifest. Bleeah.
| // Mesmerizer (default) tuning | ||
| static constexpr AudioInputParams kParamsMesmerizer{ | ||
| inline constexpr AudioInputParams kParamsMesmerizer{ | ||
| 4.0f, // windowPowerCorrection |
Collaborator
There was a problem hiding this comment.
Optional, but the readability of these blocks would be enhanced using designated initializers.
...kParamsMesmerizer{
.windowPowerCorrection = 4.0f,
.energyNoiseAdept = 0.02f,
Collaborator
There was a problem hiding this comment.
I agree. In this case the code can just show what the documentation is explaining, with the benefit of having the compiler watching.
| -DTFT_BL=45 | ||
| -DTFT_BACKLIGHT_ON=HIGH | ||
| -DTFT_RGB_ORDER=TFT_RGB | ||
| -DTFT_RGB_ORDER=TFT_RGB |
| build_flags = -DDEMO=1 | ||
| -DEFFECTS_DEMO=1 | ||
| -DPROJECT_NAME="\"M5Demo\"" | ||
| -DMATRIX_WIDTH=144*5+38 |
Collaborator
There was a problem hiding this comment.
Please describe the odd geometry here.
| hard_reset | ||
| --no-stub | ||
|
|
||
| board_build.flash_mode = dio |
Collaborator
There was a problem hiding this comment.
If it's the correct board, you shouldn't have to specify these params as they'll be provided by the board definition.
Collaborator
There was a problem hiding this comment.
I think the issue here is that esp32dev covers a lot of actual devices, which may have variations with settings like this (I remember having to explicitly deviate from a board's flash memory type in another specific case before.)
That said, the ttgo env has been in here for a long time, so it is interesting why this is necessary "all of a sudden", now.
| PeakData peaks((float *)(payloadData.get() + STANDARD_DATA_HEADER_SIZE)); | ||
| g_Analyzer.SetPeakData(peaks); | ||
| const float* pFloats = reinterpret_cast<const float*>(payloadData.get() + STANDARD_DATA_HEADER_SIZE); | ||
| PeakData peaks{}; |
Collaborator
There was a problem hiding this comment.
Is this to narrow the window of them chanaging while you're looking at them? Do we need a better queueing/notification/locking scheme?
|
|
||
| constexpr float kMaxFPS = 60.0f; | ||
| const auto targetDelay = PERIOD_FROM_FREQ(kMaxFPS) * MILLIS_PER_SECOND / MICROS_PER_SECOND; | ||
| delay(max(1.0, targetDelay - (millis() - lastFrame))); |
Collaborator
There was a problem hiding this comment.
Should we think about having this, our renderer, and anything else that's too busy spinning just park on a semaphore that's only signalled every FPS times per second so we don't have this kind of synchronization in multiple threads?
Possible downside: stampeding elephants. If we have multiple such threads and we pop the sema to start them all at the "same" time, will they compete for resources and negatively affect the entire system from advancing? (Maybe it was called something different in Win kernels, but in UNIX land, that was the name when everything was sleeping on one resource, a wakeup would be issued, but everything-1 processes would have to then immediately go back to sleep because the resoure was taken by the other things competing for that same resource: "stampeding elephants".
Collaborator
There was a problem hiding this comment.
I think one issue with this approach is that we'd be pinning the FPS to one number, where we actually want different framerates in different places - and our opinions about what the "correct" FPS at a particular place is develops over time.
I also think the "sync to FPS per thread" approach is not so messy that we need to actively avoid it.
Contributor
Author
|
On the sum thing, I’d have no problem using accumulate if it didn’t require the verbosity of all three parameters. So sum allows you just provide the enumerable object.
Unless there’s a reason sum is a bad idea…
- Dave
… On Aug 16, 2025, at 9:40 PM, Robert Lipe ***@***.***> wrote:
@robertlipe approved this pull request.
I'm unqualified to comment on the signal processing side, but just reviewing the normal programmer-ry stuff, there are some things I'd like you to at least reconsider, but LGTM.
In include/effects/strip/fireeffect.h <#746 (comment)>:
> @@ -184,7 +184,7 @@ class FireEffect : public LEDStripEffect
{
auto begin = &heat[i * CellsPerLED];
auto end = begin + CellsPerLED;
- int sum = std::accumulate(begin, end, 0);
+ int sum = std::sum(begin, end);
I think you're going backward. ::sum was proposed and never ratified in favof of ::accumulate, I think. cppreference.com, which I trust highly, lists accumulate (used correctly before) and partial_sum
In include/globals.h <#746 (comment)>:
> @@ -893,6 +894,49 @@ constexpr std::array<T, N> to_array(const T (&arr)[N]) {
return result;
}
+// sum helper
+// Provides std::sum for containers/ranges and iterator pairs.
+// Note: Adding to namespace std is a deliberate convenience for this project.
I think you've recreated accumulate()
I wonder if our ancient c++ has some bit of C++20 goodness that you wanted that just isn't there yet. Buzz me.
In include/globals.h <#746 (comment)>:
> @@ -917,6 +961,12 @@ constexpr std::array<T, N> to_array(const T (&arr)[N]) {
#if USE_TFTSPI
#define DISABLE_ALL_LIBRARY_WARNINGS 1
+ // If the project provides its own TFT_eSPI setup via USER_SETUP_LOADED
+ // and build_flags, do not include the TTGO default user setup to avoid
+ // macro redefinition warnings.
Can we please keep this stuff out of globals? Plop it into screen.cc or something of smaller scope.
In include/screen.h <#746 (comment)>:
> @@ -150,8 +150,12 @@ class Screen : public GFXBase
#endif
#if USE_TFTSPI
+ #if TTGO
Same file, different scope, different manifest. Bleeah.
In include/soundanalyzer.h <#746 (comment)>:
> @@ -73,39 +71,68 @@ struct AudioInputParams {
};
// Mesmerizer (default) tuning
-static constexpr AudioInputParams kParamsMesmerizer{
+inline constexpr AudioInputParams kParamsMesmerizer{
4.0f, // windowPowerCorrection
Optional, but the readability of these blocks would be enhanced using designated initializers.
...kParamsMesmerizer{
.windowPowerCorrection = 4.0f,
.energyNoiseAdept = 0.02f,
In include/soundanalyzer.h <#746 (comment)>:
> 3.0f, // envFloorFromNoise (cap auto-gain at ~1/4 of pure-noise)
0.0f, // frameSNRGate (require ~3:1 SNR to show frame)
1.0f, // postScale (Mesmerizer default)
- 0.333333f, // compressGamma (cube root)
- 30000000 // quietEnvFloorGate
+ (1.0/3.0), // compressGamma (cube root)
Does that decay to a float or a double? I think this will give warnings at fussier levels.
In include/soundanalyzer.h <#746 (comment)>:
> 3.0f, // envFloorFromNoise (cap auto-gain at ~1/4 of pure-noise)
0.0f, // frameSNRGate (require ~3:1 SNR to show frame)
1.0f, // postScale (Mesmerizer default)
- 0.333333f, // compressGamma (cube root)
- 30000000 // quietEnvFloorGate
+ (1.0/3.0), // compressGamma (cube root)
+ 1500000 // quietEnvFloorGate (cutoff gates all ssound when below this level)
ssmall typo.
In include/soundanalyzer.h <#746 (comment)>:
> @@ -187,13 +169,10 @@ class ISoundAnalyzer
virtual float MinVU() const = 0;
virtual int AudioFPS() const = 0;
virtual int SerialFPS() const = 0;
- virtual PeakData::MicrophoneType MicMode() const = 0;
- virtual const PeakData &Peaks() const = 0;
+ virtual bool IsRemoteAudioActive() const = 0;
+ virtual const PeakData & Peaks() const = 0;
We've used the existing spacing throughout, including elsewhere in this file.
In include/soundanalyzer.h <#746 (comment)>:
>
// Peak decay internals (private)
- unsigned long _lastPeak1Time[NUM_BANDS] = {0};
- float _peak1Decay[NUM_BANDS] = {0};
- float _peak2Decay[NUM_BANDS] = {0};
+ std::array<unsigned long, NUM_BANDS> _lastPeak1Time{};
+ std::array<float, NUM_BANDS> _peak1Decay{};
+ std::array<float, NUM_BANDS> _peak2Decay{};
What's the breaking point where you justify a struct Band that has an bandBinStart, bandBinEnd, noiseFloor, ... ? Looking through, there's some locality and combined with the traditional RISC-style small offset addressing modes (it's a lot like MIPS) and cache/locality, there may be some performance benefits as it looks like most code is traversing per band and not per element within a band.
In include/soundanalyzer.h <#746 (comment)>:
> + {
+ debugW("Only read %u of %u bytes from I2S\n", bytesRead, bytesExpected32);
+ return;
+ }
+ // Decide which channel has signal (0 or 1 within each frame)
+ static int s_chanIndex = -1;
+ if (s_chanIndex < 0)
+ {
+ long long sumAbs[2] = {0, 0};
+ for (int i = 0; i < MAX_SAMPLES; ++i)
+ {
+ int32_t r0 = raw32[i * kChannels + 0];
+ int32_t r1 = raw32[i * kChannels + 1];
+ sumAbs[0] += llabs((long long)r0);
+ sumAbs[1] += llabs((long long)r1);
+ }
Won't the channel that doesn't have the L/R pin in a predictable place read 0's on the deadhead channel? If so, you could test inside the loop and break before you've stalled for the whole SAMPLES loop. That many long long adds won't be cheap, though they're only done once. Move that terniary test below into the loop, add a break, and bail.
In include/soundanalyzer.h <#746 (comment)>:
> + static int s_chanIndex = -1;
+ if (s_chanIndex < 0)
+ {
+ long long sumAbs[2] = {0, 0};
+ for (int i = 0; i < MAX_SAMPLES; ++i)
+ {
+ int32_t r0 = raw32[i * kChannels + 0];
+ int32_t r1 = raw32[i * kChannels + 1];
+ sumAbs[0] += llabs((long long)r0);
+ sumAbs[1] += llabs((long long)r1);
+ }
+ s_chanIndex = (sumAbs[1] > sumAbs[0]) ? 1 : 0;
+ }
+ // Downconvert 24-bit left-justified samples from the chosen channel to signed 16-bit
+ // INMP441 produces 24-bit samples left-justified in 32-bit words (bits [31:8])
+ // For better dynamic range, we shift by 16 to get the top 16 bits of the 24-bit sample
But we don't shift by 16, do we?
(This comment/code seems familiar...)
In include/soundanalyzer.h <#746 (comment)>:
> + // For better dynamic range, we shift by 16 to get the top 16 bits of the 24-bit sample
+ // Apply additional scaling since INMP441 may have lower sensitivity than expected
+ for (int i = 0; i < MAX_SAMPLES; i++)
+ {
+ int32_t v = raw32[i * kChannels + s_chanIndex];
+ // Take top 16 bits and apply 2x scaling for INMP441 sensitivity compensation
+ int32_t scaled = (v >> 15); // Shift by 15 instead of 16 for 2x gain
+ ptrSampleBuffer[i] = (int16_t)std::clamp(scaled, (int32_t)INT16_MIN, (int32_t)INT16_MAX);
+ }
+ }
+ #else
+ // ADC or generic 16-bit I2S sources
+ {
+ constexpr auto bytesExpected16 = MAX_SAMPLES * sizeof(ptrSampleBuffer[0]);
+ // I2S is already running continuously; just read from DMA buffers
+ ESP_ERROR_CHECK( i2s_read(I2S_NUM_0, (void *)ptrSampleBuffer.get(), bytesExpected16, &bytesRead, 100 / portTICK_PERIOD_MS));
Funny whitespace.
Not "funny haha"; "funny weird"
In include/soundanalyzer.h <#746 (comment)>:
> + #else
+ // ADC or generic 16-bit I2S sources
+ {
+ constexpr auto bytesExpected16 = MAX_SAMPLES * sizeof(ptrSampleBuffer[0]);
+ // I2S is already running continuously; just read from DMA buffers
+ ESP_ERROR_CHECK( i2s_read(I2S_NUM_0, (void *)ptrSampleBuffer.get(), bytesExpected16, &bytesRead, 100 / portTICK_PERIOD_MS));
+ if (bytesRead != bytesExpected16)
+ {
+ debugW("Could only read %u bytes of %u in FillBufferI2S()\n", bytesRead, bytesExpected16);
+ return;
+ }
+ }
+
+ #endif
+
+ // Compute stats and copy into FFT input buffer
for (int i = 0; i < MAX_SAMPLES; i++)
{
int16_t s = ptrSampleBuffer[i];
I don't suppose we could get you to completely forget Hungarian Notation, could we? :-)
In include/soundanalyzer.h <#746 (comment)>:
> @@ -775,10 +740,11 @@ class SoundAnalyzer : public ISoundAnalyzer
~SoundAnalyzer()
{
- free(_vReal);
- free(_vImaginary);
- free(_vPeaks);
- free(_livePeaks);
+ // Stop I2S if it was started
+ #if !USE_M5 && (ELECROW || USE_I2S_AUDIO || !defined(USE_I2S_AUDIO))
I'm not sure how exactly M5 factors in here, but I think that last case is wrong.
If anyone can confidently figure out WTH ELECROW means here, I'd like it to just turn on the pins it needs and eliminate the ELECROW stuff completely (or at least in the I2S context) WIth that, this expression might be as simple ase
#if USE_I2S_AUDIO
as I think that captures the essence of what turns on the bits we're trying to turn off here.
Testing USE_THING || !defined(USE_THING) sure looks unintentional.
In include/soundanalyzer.h <#746 (comment)>:
> .dma_buf_len = (int)MAX_SAMPLES,
- .use_apll = false};
-
- // i2s pin configuration
+ .use_apll = false,
+ .tx_desc_auto_clear = false,
+ .fixed_mclk = 0};
+
+ // i2s pin configuration - explicitly set pin modes
+ pinMode(I2S_BCLK_PIN, OUTPUT); // Bit clock is output from ESP32
I'm pretty sure you don't need to add these. i2s_set_pin() below will do that.
It's intuitively obvious to even the most casual observer that this is handled by
https://github.com/espressif/esp-idf/blob/a9d0f22193acdf47a5a4db36832ae7068818962b/components/driver/i2s.c#L313
...yeah, after readign that code I'm now only 95% sure. But pinMode ultimately calls gpio_matrix_frobnicate_and_set(), so it should be the same.
But I AM sure that this code doesn't exist in current ESP-IDF or Arduino, so adding this is just more code for someone (cough, cough) to reconsider and remove soon. Unless you have hardware that doesn't work without these three lines, please drop them.
In platformio.ini <#746 (comment)>:
> -DTOUCH_CS=0
-DST7789_2_DRIVER
-DTFT_WIDTH=135
-DTFT_HEIGHT=240
-DTFT_BL=45
-DTFT_BACKLIGHT_ON=HIGH
- -DTFT_RGB_ORDER=TFT_RGB
+ -DTFT_RGB_ORDER=TFT_RGB
Trailing Whitesapce.
In platformio.ini <#746 (comment)>:
> @@ -551,6 +573,27 @@ build_flags = -DDEMO=1
extends = dev_lilygo_amoled
build_flags = -DDEMO=1
-DEFFECTS_DEMO=1
+ -DPROJECT_NAME="\"M5Demo\""
+ -DMATRIX_WIDTH=144*5+38
Please describe the odd geometry here.
In platformio.ini <#746 (comment)>:
> @@ -996,9 +1039,37 @@ build_flags = -DGENERIC=1
[env:ttgo]
extends = dev_esp32
+upload_speed = 460800
+upload_flags = --before
+ default_reset
+ --after
+ hard_reset
+ --no-stub
+
+board_build.flash_mode = dio
If it's the correct board, you shouldn't have to specify these params as they'll be provided by the board definition.
In src/network.cpp <#746 (comment)>:
> @@ -480,8 +481,10 @@ bool ProcessIncomingData(std::unique_ptr<uint8_t []> & payloadData, size_t paylo
seconds,
micros);
- PeakData peaks((float *)(payloadData.get() + STANDARD_DATA_HEADER_SIZE));
- g_Analyzer.SetPeakData(peaks);
+ const float* pFloats = reinterpret_cast<const float*>(payloadData.get() + STANDARD_DATA_HEADER_SIZE);
+ PeakData peaks{};
Is this to narrow the window of them chanaging while you're looking at them? Do we need a better queueing/notification/locking scheme?
In src/screen.cpp <#746 (comment)>:
> }
+
+ // Throttle screen updates to no more than kMaxFPS
+
+ constexpr float kMaxFPS = 60.0f;
+ const auto targetDelay = PERIOD_FROM_FREQ(kMaxFPS) * MILLIS_PER_SECOND / MICROS_PER_SECOND;
+ delay(max(1.0, targetDelay - (millis() - lastFrame)));
Should we think about having this, our renderer, and anything else that's too busy spinning just park on a semaphore that's only signalled every FPS times per second so we don't have this kind of synchronization in multiple threads?
Possible downside: stampeding elephants. If we have multiple such threads and we pop the sema to start them all at the "same" time, will they compete for resources and negatively affect the entire system from advancing? (Maybe it was called something different in Win kernels, but in UNIX land, that was the name when everything was sleeping on one resource, a wakeup would be issued, but everything-1 processes would have to then immediately go back to sleep because the resoure was taken by the other things competing for that same resource: "stampeding elephants".
In include/soundanalyzer.h <#746 (comment)>:
> @@ -446,7 +533,7 @@ class SoundAnalyzer : public ISoundAnalyzer
const float fMin = LOWEST_FREQ;
const float fMax = std::min<float>(HIGHEST_FREQ, SAMPLING_FREQUENCY / 2.0f);
const float binWidth = (float)SAMPLING_FREQUENCY / (MAX_SAMPLES / 2.0f);
- int prevBin = 2;
+ int prevBin = 4; // Start at bin 4 to skip DC (bin 0), very low freq (bins 1-3)
Meh. That's worse, IMO, Copilot.
—
Reply to this email directly, view it on GitHub <#746 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF4AYWKDBAC7WGHX6J33OABSFAVCNFSM6AAAAACEAVDAUWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMRWGE3DMOJZGQ>.
You are receiving this because you authored the thread.
|
Collaborator
|
But accumulate is awesome! Passing beginning and end is just a common C++
(OK, STL) idiom. Sure, typing .begin() and .end() is annoying, but if my
1990s vi can autocomplete that, I'll bet whatever you're using can, too.
It's a price we pay to open it up to use lambdas and strides and execution
sequences like exploiting parallel threads and SIMD and such. Of course it
can sometimes unroll and do all those tricks we expect optimizers to do.
(Trivia: did you know that the early UNIX peephole optimizers were tiny
little sed scripts that ran on the generated assembler?) Build KV pairs
into a URL or JSON object or whatever.
Initial value has some use in things like NEMA-style checksums that don't
start with 0, but it becomes handy with type coercion. 0, 0ULL, 0f, and
0.0 will each trigger different types of addition. std::string("") will
trigger string concatenation. So that ",0" earns its weight. Could/should
it be a default? OK, if I were kind I'd probably seriously entertain that
one. But start and end are here.
If you just really can't stand the typing, please replace your body with
something like
#define sumup(arg) accumulate(arg.begin(), arg.end(), 0)
This grosses me out, but please, please, please do not ever put something
new in std::. A new hire coming into the code that knows C++ inside and out
and knows all 8,437 STL entries in std:: will see this, and their brain
will explode because everything they've spent years learning about std::
won't apply here, won't be reusable, etc.
So please do NOT do this for this reason. Add a #define if you must (spit,
cough, sputter, gag) or shove a new symbol into nd: namespace or something
if you really, really want a single argument form. We've now both spent
more keystrokes on this than either one of us will save by inventing a new
shortcut.
This just isn't a place for "innovation" in the language, IMO.
RJL
On Sun, Aug 17, 2025 at 12:22 AM David W Plummer ***@***.***>
wrote:
… *davepl* left a comment (PlummersSoftwareLLC/NightDriverStrip#746)
<#746 (comment)>
On the sum thing, I’d have no problem using accumulate if it didn’t
require the verbosity of all three parameters. So sum allows you just
provide the enumerable object.
Unless there’s a reason sum is a bad idea…
- Dave
> On Aug 16, 2025, at 9:40 PM, Robert Lipe ***@***.***> wrote:
>
>
> @robertlipe approved this pull request.
>
> I'm unqualified to comment on the signal processing side, but just
reviewing the normal programmer-ry stuff, there are some things I'd like
you to at least reconsider, but LGTM.
>
> In include/effects/strip/fireeffect.h <
#746 (comment)>:
>
> > @@ -184,7 +184,7 @@ class FireEffect : public LEDStripEffect
> {
> auto begin = &heat[i * CellsPerLED];
> auto end = begin + CellsPerLED;
> - int sum = std::accumulate(begin, end, 0);
> + int sum = std::sum(begin, end);
> I think you're going backward. ::sum was proposed and never ratified in
favof of ::accumulate, I think. cppreference.com, which I trust highly,
lists accumulate (used correctly before) and partial_sum
>
> In include/globals.h <
#746 (comment)>:
>
> > @@ -893,6 +894,49 @@ constexpr std::array<T, N> to_array(const T
(&arr)[N]) {
> return result;
> }
>
> +// sum helper
> +// Provides std::sum for containers/ranges and iterator pairs.
> +// Note: Adding to namespace std is a deliberate convenience for this
project.
> I think you've recreated accumulate()
>
> I wonder if our ancient c++ has some bit of C++20 goodness that you
wanted that just isn't there yet. Buzz me.
>
> In include/globals.h <
#746 (comment)>:
>
> > @@ -917,6 +961,12 @@ constexpr std::array<T, N> to_array(const T
(&arr)[N]) {
>
> #if USE_TFTSPI
> #define DISABLE_ALL_LIBRARY_WARNINGS 1
> + // If the project provides its own TFT_eSPI setup via
USER_SETUP_LOADED
> + // and build_flags, do not include the TTGO default user setup to
avoid
> + // macro redefinition warnings.
> Can we please keep this stuff out of globals? Plop it into screen.cc or
something of smaller scope.
>
> In include/screen.h <
#746 (comment)>:
>
> > @@ -150,8 +150,12 @@ class Screen : public GFXBase
> #endif
>
> #if USE_TFTSPI
> + #if TTGO
> Same file, different scope, different manifest. Bleeah.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > @@ -73,39 +71,68 @@ struct AudioInputParams {
> };
>
> // Mesmerizer (default) tuning
> -static constexpr AudioInputParams kParamsMesmerizer{
> +inline constexpr AudioInputParams kParamsMesmerizer{
> 4.0f, // windowPowerCorrection
> Optional, but the readability of these blocks would be enhanced using
designated initializers.
> ...kParamsMesmerizer{
> .windowPowerCorrection = 4.0f,
> .energyNoiseAdept = 0.02f,
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > 3.0f, // envFloorFromNoise (cap auto-gain at ~1/4 of pure-noise)
> 0.0f, // frameSNRGate (require ~3:1 SNR to show frame)
> 1.0f, // postScale (Mesmerizer default)
> - 0.333333f, // compressGamma (cube root)
> - 30000000 // quietEnvFloorGate
> + (1.0/3.0), // compressGamma (cube root)
> Does that decay to a float or a double? I think this will give warnings
at fussier levels.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > 3.0f, // envFloorFromNoise (cap auto-gain at ~1/4 of pure-noise)
> 0.0f, // frameSNRGate (require ~3:1 SNR to show frame)
> 1.0f, // postScale (Mesmerizer default)
> - 0.333333f, // compressGamma (cube root)
> - 30000000 // quietEnvFloorGate
> + (1.0/3.0), // compressGamma (cube root)
> + 1500000 // quietEnvFloorGate (cutoff gates all ssound when below this
level)
> ssmall typo.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > @@ -187,13 +169,10 @@ class ISoundAnalyzer
> virtual float MinVU() const = 0;
> virtual int AudioFPS() const = 0;
> virtual int SerialFPS() const = 0;
> - virtual PeakData::MicrophoneType MicMode() const = 0;
> - virtual const PeakData &Peaks() const = 0;
> + virtual bool IsRemoteAudioActive() const = 0;
> + virtual const PeakData & Peaks() const = 0;
> We've used the existing spacing throughout, including elsewhere in this
file.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> >
> // Peak decay internals (private)
> - unsigned long _lastPeak1Time[NUM_BANDS] = {0};
> - float _peak1Decay[NUM_BANDS] = {0};
> - float _peak2Decay[NUM_BANDS] = {0};
> + std::array<unsigned long, NUM_BANDS> _lastPeak1Time{};
> + std::array<float, NUM_BANDS> _peak1Decay{};
> + std::array<float, NUM_BANDS> _peak2Decay{};
> What's the breaking point where you justify a struct Band that has an
bandBinStart, bandBinEnd, noiseFloor, ... ? Looking through, there's some
locality and combined with the traditional RISC-style small offset
addressing modes (it's a lot like MIPS) and cache/locality, there may be
some performance benefits as it looks like most code is traversing per band
and not per element within a band.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > + {
> + debugW("Only read %u of %u bytes from I2S\n", bytesRead,
bytesExpected32);
> + return;
> + }
> + // Decide which channel has signal (0 or 1 within each frame)
> + static int s_chanIndex = -1;
> + if (s_chanIndex < 0)
> + {
> + long long sumAbs[2] = {0, 0};
> + for (int i = 0; i < MAX_SAMPLES; ++i)
> + {
> + int32_t r0 = raw32[i * kChannels + 0];
> + int32_t r1 = raw32[i * kChannels + 1];
> + sumAbs[0] += llabs((long long)r0);
> + sumAbs[1] += llabs((long long)r1);
> + }
> Won't the channel that doesn't have the L/R pin in a predictable place
read 0's on the deadhead channel? If so, you could test inside the loop and
break before you've stalled for the whole SAMPLES loop. That many long long
adds won't be cheap, though they're only done once. Move that terniary test
below into the loop, add a break, and bail.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > + static int s_chanIndex = -1;
> + if (s_chanIndex < 0)
> + {
> + long long sumAbs[2] = {0, 0};
> + for (int i = 0; i < MAX_SAMPLES; ++i)
> + {
> + int32_t r0 = raw32[i * kChannels + 0];
> + int32_t r1 = raw32[i * kChannels + 1];
> + sumAbs[0] += llabs((long long)r0);
> + sumAbs[1] += llabs((long long)r1);
> + }
> + s_chanIndex = (sumAbs[1] > sumAbs[0]) ? 1 : 0;
> + }
> + // Downconvert 24-bit left-justified samples from the chosen channel
to signed 16-bit
> + // INMP441 produces 24-bit samples left-justified in 32-bit words
(bits [31:8])
> + // For better dynamic range, we shift by 16 to get the top 16 bits of
the 24-bit sample
> But we don't shift by 16, do we?
> (This comment/code seems familiar...)
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > + // For better dynamic range, we shift by 16 to get the top 16 bits
of the 24-bit sample
> + // Apply additional scaling since INMP441 may have lower sensitivity
than expected
> + for (int i = 0; i < MAX_SAMPLES; i++)
> + {
> + int32_t v = raw32[i * kChannels + s_chanIndex];
> + // Take top 16 bits and apply 2x scaling for INMP441 sensitivity
compensation
> + int32_t scaled = (v >> 15); // Shift by 15 instead of 16 for 2x gain
> + ptrSampleBuffer[i] = (int16_t)std::clamp(scaled, (int32_t)INT16_MIN,
(int32_t)INT16_MAX);
> + }
> + }
> + #else
> + // ADC or generic 16-bit I2S sources
> + {
> + constexpr auto bytesExpected16 = MAX_SAMPLES *
sizeof(ptrSampleBuffer[0]);
> + // I2S is already running continuously; just read from DMA buffers
> + ESP_ERROR_CHECK( i2s_read(I2S_NUM_0, (void *)ptrSampleBuffer.get(),
bytesExpected16, &bytesRead, 100 / portTICK_PERIOD_MS));
> Funny whitespace.
> Not "funny haha"; "funny weird"
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > + #else
> + // ADC or generic 16-bit I2S sources
> + {
> + constexpr auto bytesExpected16 = MAX_SAMPLES *
sizeof(ptrSampleBuffer[0]);
> + // I2S is already running continuously; just read from DMA buffers
> + ESP_ERROR_CHECK( i2s_read(I2S_NUM_0, (void *)ptrSampleBuffer.get(),
bytesExpected16, &bytesRead, 100 / portTICK_PERIOD_MS));
> + if (bytesRead != bytesExpected16)
> + {
> + debugW("Could only read %u bytes of %u in FillBufferI2S()\n",
bytesRead, bytesExpected16);
> + return;
> + }
> + }
> +
> + #endif
> +
> + // Compute stats and copy into FFT input buffer
> for (int i = 0; i < MAX_SAMPLES; i++)
> {
> int16_t s = ptrSampleBuffer[i];
> I don't suppose we could get you to completely forget Hungarian
Notation, could we? :-)
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > @@ -775,10 +740,11 @@ class SoundAnalyzer : public ISoundAnalyzer
>
> ~SoundAnalyzer()
> {
> - free(_vReal);
> - free(_vImaginary);
> - free(_vPeaks);
> - free(_livePeaks);
> + // Stop I2S if it was started
> + #if !USE_M5 && (ELECROW || USE_I2S_AUDIO || !defined(USE_I2S_AUDIO))
> I'm not sure how exactly M5 factors in here, but I think that last case
is wrong.
>
> If anyone can confidently figure out WTH ELECROW means here, I'd like it
to just turn on the pins it needs and eliminate the ELECROW stuff
completely (or at least in the I2S context) WIth that, this expression
might be as simple ase
> #if USE_I2S_AUDIO
> as I think that captures the essence of what turns on the bits we're
trying to turn off here.
> Testing USE_THING || !defined(USE_THING) sure looks unintentional.
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > .dma_buf_len = (int)MAX_SAMPLES,
> - .use_apll = false};
> -
> - // i2s pin configuration
> + .use_apll = false,
> + .tx_desc_auto_clear = false,
> + .fixed_mclk = 0};
> +
> + // i2s pin configuration - explicitly set pin modes
> + pinMode(I2S_BCLK_PIN, OUTPUT); // Bit clock is output from ESP32
> I'm pretty sure you don't need to add these. i2s_set_pin() below will do
that.
>
> It's intuitively obvious to even the most casual observer that this is
handled by
>
https://github.com/espressif/esp-idf/blob/a9d0f22193acdf47a5a4db36832ae7068818962b/components/driver/i2s.c#L313
> ...yeah, after readign that code I'm now only 95% sure. But pinMode
ultimately calls gpio_matrix_frobnicate_and_set(), so it should be the
same.
>
> But I AM sure that this code doesn't exist in current ESP-IDF or
Arduino, so adding this is just more code for someone (cough, cough) to
reconsider and remove soon. Unless you have hardware that doesn't work
without these three lines, please drop them.
>
> In platformio.ini <
#746 (comment)>:
>
> > -DTOUCH_CS=0
> -DST7789_2_DRIVER
> -DTFT_WIDTH=135
> -DTFT_HEIGHT=240
> -DTFT_BL=45
> -DTFT_BACKLIGHT_ON=HIGH
> - -DTFT_RGB_ORDER=TFT_RGB
> + -DTFT_RGB_ORDER=TFT_RGB
> Trailing Whitesapce.
>
> In platformio.ini <
#746 (comment)>:
>
> > @@ -551,6 +573,27 @@ build_flags = -DDEMO=1
> extends = dev_lilygo_amoled
> build_flags = -DDEMO=1
> -DEFFECTS_DEMO=1
> + -DPROJECT_NAME="\"M5Demo\""
> + -DMATRIX_WIDTH=144*5+38
> Please describe the odd geometry here.
>
> In platformio.ini <
#746 (comment)>:
>
> > @@ -996,9 +1039,37 @@ build_flags = -DGENERIC=1
>
> [env:ttgo]
> extends = dev_esp32
> +upload_speed = 460800
> +upload_flags = --before
> + default_reset
> + --after
> + hard_reset
> + --no-stub
> +
> +board_build.flash_mode = dio
> If it's the correct board, you shouldn't have to specify these params as
they'll be provided by the board definition.
>
> In src/network.cpp <
#746 (comment)>:
>
> > @@ -480,8 +481,10 @@ bool ProcessIncomingData(std::unique_ptr<uint8_t
[]> & payloadData, size_t paylo
> seconds,
> micros);
>
> - PeakData peaks((float *)(payloadData.get() +
STANDARD_DATA_HEADER_SIZE));
> - g_Analyzer.SetPeakData(peaks);
> + const float* pFloats = reinterpret_cast<const
float*>(payloadData.get() + STANDARD_DATA_HEADER_SIZE);
> + PeakData peaks{};
> Is this to narrow the window of them chanaging while you're looking at
them? Do we need a better queueing/notification/locking scheme?
>
> In src/screen.cpp <
#746 (comment)>:
>
> > }
> +
> + // Throttle screen updates to no more than kMaxFPS
> +
> + constexpr float kMaxFPS = 60.0f;
> + const auto targetDelay = PERIOD_FROM_FREQ(kMaxFPS) * MILLIS_PER_SECOND
/ MICROS_PER_SECOND;
> + delay(max(1.0, targetDelay - (millis() - lastFrame)));
> Should we think about having this, our renderer, and anything else
that's too busy spinning just park on a semaphore that's only signalled
every FPS times per second so we don't have this kind of synchronization in
multiple threads?
>
> Possible downside: stampeding elephants. If we have multiple such
threads and we pop the sema to start them all at the "same" time, will they
compete for resources and negatively affect the entire system from
advancing? (Maybe it was called something different in Win kernels, but in
UNIX land, that was the name when everything was sleeping on one resource,
a wakeup would be issued, but everything-1 processes would have to then
immediately go back to sleep because the resoure was taken by the other
things competing for that same resource: "stampeding elephants".
>
> In include/soundanalyzer.h <
#746 (comment)>:
>
> > @@ -446,7 +533,7 @@ class SoundAnalyzer : public ISoundAnalyzer
> const float fMin = LOWEST_FREQ;
> const float fMax = std::min<float>(HIGHEST_FREQ, SAMPLING_FREQUENCY /
2.0f);
> const float binWidth = (float)SAMPLING_FREQUENCY / (MAX_SAMPLES / 2.0f);
> - int prevBin = 2;
> + int prevBin = 4; // Start at bin 4 to skip DC (bin 0), very low freq
(bins 1-3)
> Meh. That's worse, IMO, Copilot.
>
> —
> Reply to this email directly, view it on GitHub <
#746 (review)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AA4HCF4AYWKDBAC7WGHX6J33OABSFAVCNFSM6AAAAACEAVDAUWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMRWGE3DMOJZGQ>.
> You are receiving this because you authored the thread.
>
—
Reply to this email directly, view it on GitHub
<#746 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD32YY2JZW26QQTTZS7D3OAGQ7AVCNFSM6AAAAACEAVDAUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCOJUGE2DAMJUGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
rbergen
reviewed
Aug 17, 2025
rbergen
left a comment
rbergen
left a comment
Collaborator
There was a problem hiding this comment.
Summary: it's not technically broken, but I really, really don't like the std::sum thing.
| // sum helper | ||
| // Provides std::sum for containers/ranges and iterator pairs. | ||
| // Note: Adding to namespace std is a deliberate convenience for this project. | ||
| namespace std { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Mesmerizer (default) tuning | ||
| static constexpr AudioInputParams kParamsMesmerizer{ | ||
| inline constexpr AudioInputParams kParamsMesmerizer{ | ||
| 4.0f, // windowPowerCorrection |
Collaborator
There was a problem hiding this comment.
I agree. In this case the code can just show what the documentation is explaining, with the benefit of having the compiler watching.
| free(_vPeaks); | ||
| free(_livePeaks); | ||
| // Stop I2S if it was started | ||
| #if !USE_M5 && (ELECROW || USE_I2S_AUDIO || !defined(USE_I2S_AUDIO)) |
Collaborator
There was a problem hiding this comment.
It's the syntax one would use if they want to say "don't do the following if USE_I2S_AUDIO is explicitly set to 0". I.e. default to it being 1 when undefined.
| hard_reset | ||
| --no-stub | ||
|
|
||
| board_build.flash_mode = dio |
Collaborator
There was a problem hiding this comment.
I think the issue here is that esp32dev covers a lot of actual devices, which may have variations with settings like this (I remember having to explicitly deviate from a board's flash memory type in another specific case before.)
That said, the ttgo env has been in here for a long time, so it is interesting why this is necessary "all of a sudden", now.
|
|
||
| constexpr float kMaxFPS = 60.0f; | ||
| const auto targetDelay = PERIOD_FROM_FREQ(kMaxFPS) * MILLIS_PER_SECOND / MICROS_PER_SECOND; | ||
| delay(max(1.0, targetDelay - (millis() - lastFrame))); |
Collaborator
There was a problem hiding this comment.
I think one issue with this approach is that we'd be pinning the FPS to one number, where we actually want different framerates in different places - and our opinions about what the "correct" FPS at a particular place is develops over time.
I also think the "sync to FPS per thread" approach is not so messy that we need to actively avoid it.
Collaborator
|
We're on different edges of bedtime, but quickly before I retire...
Summary: it's not technically broken, but I *really, really* don't like
the std::sum thing.
It is, technically, broken to use a reserved namespace. That's why they're
reserved. But, we agree that it's icky, so let's please just not.
We're on C++20, so it's possible (and better) to use
std::ranges::range_value_t<Range>.
Well. We're on what G++ thought C++ after C++ 20 would look like, from the
view of G++ in 8.2 from 2018 or something. This is why we're on 2b
instead of 2a, even. There are a couple of leaps of faith of forward
compatibility there.We know what some parts of the future look like well
before specs are finalized, and others meld and reshape a little as we get
closer. Ranges was one of those. I know I've tried to do non-trivial things
with ranges in our existing code (which is stuck on an ancient, PITA
toolchain because PlatformIO stinks) because ranges for effects code is
_awesome_ ... and because they don't work very well with the version of the
library/compiler we're actually stuck with for now.
I know you're all about tired of hearing me whine about how hard it is to
pull this whole code base into the recent past, but Ranges is definitely a
sticky area where our existing tooling does NOT match what landed in the
spec a few years after our tooling (now something like 8 years behind) was
looking ahead for it to land.
Will that specific aspect of ranges work? Maybe. The specific aspect that I
needed was stillborn until GCC10 or so while we're still trapped on 8
\Stop I2S if it was started
+ #if !USE_M5 && (ELECROW || USE_I2S_AUDIO || !defined(USE_I2S_AUDIO))
It's the syntax one would use if they want to say "don't do the following
if USE_I2S_AUDIO is explicitly set to 0". I.e. default to it being 1 when
undefined.
But we've tried to avoid that, right? Since I just invntented the
USE_i2s_audio path a week ago (and you even called out that I, alone, had
boards using it that i hadn't submitted yet) we should be able to NOT have
a super strange case here. Let's try to make it less odd.
|I think one issue with this approach is that we'd be pinning the FPS to
one number, where we actually want different framerates in different places
- and our opinions about what the "correct" FPS at a particular place is
develops over time.
That's a little weird, but I dont' totally hate that line of reasoning.
Enjoy your morning! I'm off to call it a night. :-)
Nighty night!
RJL
… Message ID:
***@***.***
com>
|
Collaborator
I used Dave's usual standard for "technically broken" in cases like these, which is "it doesn't compile". ;) But yeah, you and I agree this is very disagreeable, which I think is the main point. About the question I asked about the "why", I've now seen the answer from @davepl (I really need to remember to scroll down the PR page to look at email exchanges before I comment...), and IMHO that's not enough to hack into the I don't think the "this is shorter when calling" argument warrants deviating from the standard in this case, let alone extending it. And I agree that there is an actual benefit of showing intent by explicitly setting the init value, even in non-
I'm sure the -std build flag refers to 2a?
I think you knew very well that it doesn't. :) I added another comment on my earlier one. Those approaches (one of which I label as a form of over-engineering myself) I have confirmed to compile. Again/still, the main thing is that I strongly believe the whole (
When you boil it down, this is basically a shorthand for: #ifndef USE_I2S_AUDIO
#define USE_I2S_AUDIO 1
#endif
#if !USE_M5 && (ELECROW || USE_I2S_AUDIO)I agree that code is easier to follow, and more in line with our usual approach. |
Contributor
Author
|
I took everything Robert had to say on the matter into account, and revised my approach accordingly. I removed the addition of std:: sum.
I did, however, add a specialization of std::accumulate that just takes the collection and returns you the sum. Perhaps there were long nights of arguments about patterns, but I don’t care, I like this approach even if its naive. Maybe its bad to ever touch std at all, but I wasn’t raised to believe that. So you’d have to teach me that now :-)
- Dave
… On Aug 17, 2025, at 3:31 AM, Rutger van Bergen ***@***.***> wrote:
rbergen
left a comment
(PlummersSoftwareLLC/NightDriverStrip#746)
<#746 (comment)>
It is, technically, broken to use a reserved namespace. That's why they're reserved. But, we agree that it's icky, so let's please just not.
I used Dave's usual standard for "technically broken" in cases like these, which is "it doesn't compile". ;) But yeah, you and I agree this is very disagreeable, which I think is the main point.
About the question I asked about the "why", I've now seen the answer from @davepl <https://github.com/davepl> (I really need to remember to scroll down the PR page to look at email exchanges before I comment...), and IMHO that's not enough to hack into the std namespace. One of my own cosmetic objections to using std stuff is that it often makes the code that uses them more verbose - and to me less intuitive - than tailor-made, self-baked alternatives, but I accept that sticking to the standard when possible has benefits that far outweigh that objection - no matter how cleverly the AI-generated code has been put together.
I don't think the "this is shorter when calling" argument warrants deviating from the standard in this case, let alone extending it. And I agree that there is an actual benefit of showing intent by explicitly setting the init value, even in non-string cases.
This is why we're on 2b instead of 2a, even.
I'm sure the -std build flag refers to 2a?
Will that specific aspect of ranges work? Maybe.
I think you knew very well that it doesn't. :) I added another comment on my earlier one. Those approaches (one of which I label as a form of over-engineering myself) I have confirmed to compile.
Again/still, the main thing is that I strongly believe the whole (std::)sum() thing shouldn't exist at all in the first place.
It's the syntax one would use if they want to say "don't do the following if USE_I2S_AUDIO is explicitly set to 0". I.e. default to it being 1 when undefined.
But we've tried to avoid that, right? Since I just invented the USE_i2s_audio path a week ago (and you even called out that I, alone, had boards using it that i hadn't submitted yet) we should be able to NOT have a super strange case here. Let's try to make it less odd.
When you boil it down, this is basically a shorthand for:
#ifndef USE_I2S_AUDIO
#define USE_I2S_AUDIO 1
#endif
#if !USE_M5 && (ELECROW || USE_I2S_AUDIO)
I agree that code is easier to follow, and more in line with our usual approach.
—
Reply to this email directly, view it on GitHub <#746 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF2T3SEXS4JAF6AUPJ33OBKXJAVCNFSM6AAAAACEAVDAUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCOJUGI4TIMJSG4>.
You are receiving this because you were mentioned.
|
Collaborator
|
There's no point teaching something to someone who doesn't want to be taught. ;) It's clear that you've considered the input and made a decision. Conventions are meant to make things predictable when people collaborate, and according to C++ conventions you indeed really shouldn't add stuff to |
rbergen
reviewed
Aug 17, 2025
Comment on lines
+900
to
+904
| template <typename Range> | ||
| inline auto accumulate(const Range& r) | ||
| -> std::remove_cv_t<std::remove_reference_t<decltype(*std::begin(r))>> | ||
| { | ||
| using T = std::remove_cv_t<std::remove_reference_t<decltype(*std::begin(r))>>; |
Collaborator
There was a problem hiding this comment.
std::remove_cv_t<std::remove_reference_t<decltype(*std::begin(r))>> can be replaced by the more concise std::decay_t<decltype(*std::begin(r))>.
You may have to #include <type_traits> to make it compile.
This was referenced Aug 19, 2025
Merged
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gets rid of the runtime Mic information, makes it a templated argument.
Uses the proper setup headers instead of manual definitions in platformio.ini
mainas the target branch.