Skip to content

Conversation

davepl
Copy link
Contributor

@davepl davepl commented Aug 5, 2025

Description

Adds a new specialization of M5DEMO that runs just the 2 PDP effects with no effect interval.
Adds a new effect, PDPCMXEffect, which replicates the LED display of a Thinking Machines CM-5

In keeping with the blinkenperbit metric, I opted to just add a specialization of the M5DEMO project rather than entirely new project... less change.

Contributing requirements

  • 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.

@davepl davepl requested a review from rbergen August 5, 2025 18:38
@davepl
Copy link
Contributor Author

davepl commented Aug 5, 2025 via email

@robertlipe
Copy link
Collaborator

robertlipe commented Aug 6, 2025 via email

@davepl davepl changed the title Add PDPWOPR project. PDPCMX effect Add PDPWOPR project. PDPCMX effect. New Audio engine. Aug 12, 2025
@davepl davepl requested review from rbergen and robertlipe August 12, 2025 01:23
@davepl davepl self-assigned this Aug 12, 2025
Copy link
Collaborator

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

I gave this a thorough review to really study it. (About 90 minutes worth.) ...Even the parts that made my head hurt.

This is going to be a helluva merge conflict for me since I'd already replaced a lot of the configuration machinery in a very similar way, but I'll suck that up. This really lays the plumbing for new boards/effects to be totally in custom_blah.ini without touching a file that forces the world to recompile, and that makes me happy.

I'm officially happy with this. Thank you.

@@ -91,7 +91,7 @@ board = adafruit_feather_esp32s3_tft
monitor_speed = 1500000
upload_speed = 1500000
lib_deps = ${base.lib_deps}
TFT_eSPI
bodmer/TFT_eSPI @ ^2.5.43

[dev_esp32]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, while you're in here since I think you're the only one with m5stack hardware, can you peek down around 161 in [dev_m5] and see if updating M5Unified to a newer version fixes the many warnings on every build?

That's been on my list for a while.

Your call whether that's this PR or this one, but it seems kind of related.

#if USE_M5
// Disable the deprecated Atom messages
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the real problem here that our M5 package is picking up an old version? I poked at this a little and pio pkg list -e m5something reported something substantially under what's in m5stack/M5Unified.

Fixing that doesn't have to be part of this PR, but leaving the smoke detectors going off instead of taking the batteries out of them will get one of us to look deeper into this.

Serial.println("Band layout (start-end):");
for (int b = 0; b < NUM_BANDS; b++)
{
Serial.print(b);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider a debugI() to save you five calls. You're inside an #ifdef, so the level doesn't matter a lot.

_MicMode = PeakData::MESMERIZERMIC;
#endif

#if M5STICKCPLUS2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually _micMode = AudioInputParams() ?

In a near future world, microphones might be runtime configurable. We should try to avoid preprocessor tests when we can.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not exactly. AudioInputParams() determines what to return based on _micMode, which is set here. So it's more _micMode -> AudioInputParams() - which may be what you meant.

Which does mean both _micMode and AudioInputParams()'s return value are determined by a define. Not that it matters much at the moment, because AudioInputParams() returns the same for all _micMode values.

Import("env")

try:
import intelhex
Copy link
Collaborator

Choose a reason for hiding this comment

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

That might have been a transient bug.
platformio/platform-espressif32#1632

board_build.partitions = config/partitions_custom_noota.csv
-DEFFECTS_DEMO=1
-DPROJECT_NAME="\"M5Demo\""
-DMATRIX_WIDTH=144*5+38
Copy link
Collaborator

Choose a reason for hiding this comment

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

In at least one of these, please explain what the 5+38 is about.

@davepl
Copy link
Contributor Author

davepl commented Aug 12, 2025 via email

@robertlipe
Copy link
Collaborator

robertlipe commented Aug 12, 2025 via email

@robertlipe
Copy link
Collaborator

robertlipe commented Aug 12, 2025 via email

@davepl
Copy link
Contributor Author

davepl commented Aug 12, 2025 via email

@robertlipe
Copy link
Collaborator

robertlipe commented Aug 12, 2025 via email

@rbergen
Copy link
Collaborator

rbergen commented Aug 12, 2025

It's actually basic semantic versioning compatibility rules. As long as the major version is 0 (like in the case of 0.1.17), minor version bumps are also treated as "breaking" because the API is considered unstable. Quoting from the npm semver docs about carets: "Allows changes that do not modify the left-most non-zero digit in the [major, minor, patch] tuple. In other words, this allows patch and minor updates for versions 1.0.0 and above, patch updates for versions 0.X >=0.1.0, and no updates for versions 0.0.X."

Copy link
Collaborator

@rbergen rbergen left a comment

Choose a reason for hiding this comment

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

One or two things you may want to look at, but nothing serious enough to block approval.

It's a very nice overhaul of our project configuration ball of wool, for sure!

_MicMode = PeakData::MESMERIZERMIC;
#endif

#if M5STICKCPLUS2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not exactly. AudioInputParams() determines what to return based on _micMode, which is set here. So it's more _micMode -> AudioInputParams() - which may be what you meant.

Which does mean both _micMode and AudioInputParams()'s return value are determined by a define. Not that it matters much at the moment, because AudioInputParams() returns the same for all _micMode values.

// === EFFECT SETS ===
// These sections are shared by multiple projects

#elif defined(EFFECTS_MINIMAL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just mentioning here that if any of the "effective effects sets" for any of the known project names (-DPROJECT_NAME in platformio.ini) has changed in this update, the EFFECT_SET_VERSION define for that block needs to be bumped, to make the people flashing this update get the updated set. If PROJECT_NAME and EFFECT_SET_VERSION are the same as what had been persisted to JSON with the then-current effect set, the device will load the set persisted to JSON instead of what is put together here.

framework = arduino
build_type = release
build_unflags = -std=gnu++11
lib_extra_dirs = ${PROJECT_DIR}/lib
monitor_filters = esp32_exception_decoder
extra_scripts = pre:tools/bake_site.py
extra_scripts = pre:install_intelhex.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, if it turns out install_intelhex.py is no longer necessary, then neither is this.

@davepl
Copy link
Contributor Author

davepl commented Aug 13, 2025

Thanks for the reviews! It builds, so in it goes :-)

@davepl davepl merged commit 837476d into main Aug 13, 2025
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants