added support for m5stick C plus 2 #4415
Conversation
|
Hi @huuck Thanks for your contribution. As the M5 is just a regular esp32 build there is no need to for a change to platformio.ini, please move the examples of how to build your usermod into the readme file of your usermod. With regards to the usermod itself, it doesn't appear to actually be M5 specific, so it might be better to rename this to be more generic |
|
Hmm... I'm not sure I get it. While it is a regular ESP32, it comes with 2 peripherals attached (the display and mic). Should I move all of the defines for the pins inside the usermod header? |
|
No keep as platformio build flags, but just say in your readme what env to add. We just don't add every combination of board and usermod to the main platformio.ini file or it would be massive |
|
Ah, amazing. Thanks for explaining, I'll move everything to the usermod and resubmit! |
|
@netmindz Fixed, can you re verify, please? ty! |
|
Yeah, that fixes the issue with platformio.ini I wonder though if this kinda overlaps with #4072 |
| tft.setTextFont(1); | ||
| tft.setCursor(100, 100); | ||
|
|
||
| for(int i = 0; i < 16; i++) { |
There was a problem hiding this comment.
Try to avoid magic numbers, e.g 16, use NUM_GEQ_CHANNELS
|
Hm, did not know about that PR, should I wait until the merge is live and push again? I've been having some issues with the eTFT driver, maybe this refactor will clear it up. |
| -D TFT_CS=5 | ||
| -D SPI_FREQUENCY=20000000 | ||
| -D USER_SETUP_LOADED | ||
| -D USERMOD_AUDIOREACTIVE -D UM_AUDIOREACTIVE_USE_NEW_FFT |
There was a problem hiding this comment.
Hi, this is the "old way" for activating the audioreactive usermod. It's better if you use references to standard flags. Also you may want to to add flags for the onboard mic pins.
${esp32.AR_build_flags} ;; default flags required to properly configure ArduinoFFT
-D I2S_SDPIN=34 -D I2S_WSPIN=0 -D I2S_CKPIN=-1 -D MCLK_PIN=-1 ;; PDM microphone pins
-D DMTYPE=5 -D SR_SQUELCH=5 -D SR_GAIN=75 ;; SPM1423 specific sound settings
-D I2S_USE_RIGHT_CHANNEL ;; might be needed, if your microphone uses the right channel instead of left
-D UM_AUDIOREACTIVE_ENABLE ;; enable AR by defaultThe example is actually from the M5StickC, so your pins might be different.
For your info, the full buildenv is from here: https://github.com/atuline/WLED/blob/ce6b9d80273575419fecf2a752162f08e28361e0/platformio.ini#L545
I also remember that some M5 boards needed a special library and call M5.begin() to power on microphone and other build-in devices. Is this the case for your board, too?
There was a problem hiding this comment.
No, both the tft and the mic worked without the M5 library. I wanted to avoid it as it would add too much bloat.
There was a problem hiding this comment.
Ok, so please use ${esp32.AR_build_flags}, instead of -D USERMOD_AUDIOREACTIVE -D UM_AUDIOREACTIVE_USE_NEW_FFT
| lib_deps = | ||
| ${esp32.lib_deps} | ||
| TFT_eSPI @ ^2.3.70 | ||
| https://github.com/kosme/arduinoFFT#419d7b0 |
There was a problem hiding this comment.
Same here - you need to use ${esp32.AR_lib_deps} , instead of referencing a commit tag in ArduinoFFT
${esp32.AR_lib_deps} ;; needed for USERMOD_AUDIOREACTIVE|
|
||
| for(int i = 0; i < 16; i++) { | ||
| tft.fillRect(fftResult[i] / 2, i*15 + 3, 135 - (fftResult[i] / 2), 10, TFT_BLACK); | ||
| tft.fillRect(0, i*15 + 3, fftResult[i] / 2, 10, TFT_GOLD); |
There was a problem hiding this comment.
This way of referencing a variable inside the audioreactive usermod may not work in the future, when each usermod will have it's own namespace. The safer way is to use a local pointer to array
um_data_t *um_data = getAudioData();
uint8_t *fftChannels = (uint8_t*)um_data->u_data[2];Then you can simply use fftChannels[i] to access the data. If audioreactive is disabled, you will automatically get "soundsim" like in any other effect.
Good point - I think the display content is really not specific to the M5StickC2 board, just TFT dimensions might be different for others. The visualizer code might also be added to the generic TFT usermod. |
WalkthroughThe pull request introduces a new usermod for the M5Stick2 Visualiser. A header file is added, defining the Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi! I addressed all of the comments, would it be possible to have another go? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/M5Stick2_Visualiser/M5Stick2_Visualiser.h (1)
160-167: 🛠️ Refactor suggestionUse local pointer for safer audio data access.
Based on past review comments, accessing audio reactive usermod data should be done using a local pointer to handle future namespace changes.
Apply this diff to improve the audio data access:
- um_data_t *um_data; - if (!usermods.getUMData(&um_data, USERMOD_ID_AUDIOREACTIVE)) { - // add support for no audio - um_data = simulateSound(SEGMENT.soundSim); - } - - uint8_t *fftChannels = (uint8_t*)um_data->u_data[2]; + um_data_t *um_data = getAudioData(); + uint8_t *fftChannels = (uint8_t*)um_data->u_data[2];usermods/M5Stick2_Visualiser/README.md (1)
55-55: 🛠️ Refactor suggestionUse AR_lib_deps variable for ArduinoFFT dependency.
Based on past review comments, use the standard AR_lib_deps variable instead of direct GitHub commit reference.
Apply this diff:
- https://github.com/kosme/arduinoFFT#419d7b0
🧹 Nitpick comments (2)
usermods/M5Stick2_Visualiser/M5Stick2_Visualiser.h (1)
168-171: Replace magic number with TFT_WIDTH constant.Use the defined TFT_WIDTH constant instead of the magic number 135 for better maintainability.
Apply this diff:
- tft.fillRect(fftChannels[i] / 2, chan_width * i, 135 - (fftChannels[i] / 2), chan_width - 1, TFT_BLACK); + tft.fillRect(fftChannels[i] / 2, chan_width * i, TFT_WIDTH - (fftChannels[i] / 2), chan_width - 1, TFT_BLACK);usermods/M5Stick2_Visualiser/README.md (1)
21-21: Add language specifiers to code blocks.Add language specifiers to fenced code blocks for better syntax highlighting.
Apply these changes:
- Line 21: Change
```to```ini- Line 62: Change
```to```iniAlso applies to: 62-62
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
21-21: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
usermods/M5Stick2_Visualiser/images/display.jpgis excluded by!**/*.jpgusermods/M5Stick2_Visualiser/images/mic.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
usermods/M5Stick2_Visualiser/M5Stick2_Visualiser.h(1 hunks)usermods/M5Stick2_Visualiser/README.md(1 hunks)wled00/usermods_list.cpp(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
usermods/M5Stick2_Visualiser/README.md
21-21: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (3)
usermods/M5Stick2_Visualiser/M5Stick2_Visualiser.h (2)
83-107: LGTM! Robust pin initialization and error handling.The setup function properly allocates pins using pinManager and handles failures gracefully. The backlight pin initialization is also handled correctly.
10-32: LGTM! Thorough configuration validation.The preprocessor directives ensure all required display parameters are defined at compile time, with proper error messages.
wled00/usermods_list.cpp (1)
245-247: LGTM! Proper usermod registration.The usermod is correctly included and registered following the established pattern in the file.
Also applies to: 490-492
As mentioned above, I added a new target, the M5Stick C PLUS2 with a simple FFT visualiser as a usermod that's enabled by default for it. You can drive the LEDs from any of the data pins with an external power source (haven't tested the Stick's one but I don't recommend it).
Summary by CodeRabbit