Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion usermods/EXAMPLE/library.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"name:": "EXAMPLE",
"name": "EXAMPLE",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

revert, see #4623

"dependencies": {}
}
4 changes: 2 additions & 2 deletions usermods/sht/library.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name:": "sht",
"name": "sht",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

revert, it will be fixed with this pending PR: #4623

"dependencies": {
"robtillaart/SHT85": "~0.3.3"
}
}
}
4 changes: 4 additions & 0 deletions usermods/user_fx/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Usermod user FX

This Usermod is a common place to put various user's LED effects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add some additional info as seen in other usermods. Also don't forget your contact information so users may ask for help.

3 changes: 3 additions & 0 deletions usermods/user_fx/library.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name:": "user_fx"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is what you want to fix

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
102 changes: 102 additions & 0 deletions usermods/user_fx/user_fx.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#include "user_fx.h"

#include <cstdint>

#define XY(_x, _y) (_x + SEG_W * (_y))

static uint16_t mode_static(void) {
SEGMENT.fill(SEGCOLOR(0));
return strip.isOffRefreshRequired() ? FRAMETIME : 350;
}

static uint16_t mode_diffusionfire(void) {
static uint32_t call = 0;

if (!strip.isMatrix || !SEGMENT.is2D())
return mode_static(); // not a 2D set-up

const int cols = SEG_W;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SEG_W and SEG_H are macros that obscure their meaning. It would be nice to have explanation of their meaning (in comment), i.e. "segment width in pixels" or something similar.

const int rows = SEG_H;

const uint8_t refresh_hz = map(SEGMENT.speed, 0, 255, 20, 80);
const unsigned refresh_ms = 1000 / refresh_hz;
const int16_t diffusion = map(SEGMENT.custom1, 0, 255, 0, 100);
const uint8_t spark_rate = SEGMENT.intensity;
const uint8_t turbulence = SEGMENT.custom2;

unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t));
if (!SEGENV.allocateData(dataSize))
return mode_static(); // allocation failed

@coderabbitai coderabbitai Bot Apr 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid re-allocating the scratch buffer every frame

SEGENV.allocateData() is invoked on every call even after memory has been assigned, which can lead to heap fragmentation and skips a precious few µs of CPU time per frame.

-  if (!SEGENV.allocateData(dataSize))
+  if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))
     return mode_static();  // allocation failed
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t));
if (!SEGENV.allocateData(dataSize))
return mode_static(); // allocation failed
unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t));
- if (!SEGENV.allocateData(dataSize))
+ if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))
return mode_static(); // allocation failed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is ok as allocateData() does its checks.
Even though SEGENV.data is not null it may point to a memory not allocated by the effect and may not be of correct size.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No description provided.

if (SEGENV.call == 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would also add comment like "when call is 0 (first execution of effect function) clear segment".

SEGMENT.fill(BLACK);
SEGENV.step = 0;
call = 0;
}

if ((strip.now - SEGENV.step) >= refresh_ms) {
// Reserve one extra byte and align to 2-byte boundary to avoid hard-faults

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you did not reserve the extra byte...
also this is much more readable:

uintptr_t addr = reinterpret_cast<uintptr_t>(original_pointer);
uintptr_t aligned = (addr + 1) & ~1;

uint8_t *bufStart = SEGMENT.data + SEGMENT.length();
uintptr_t aligned = (uintptr_t(bufStart) + 1u) & ~uintptr_t(0x1u);
uint16_t *tmp_row = reinterpret_cast<uint16_t *>(aligned);
SEGENV.step = strip.now;
call++;

// scroll up
for (unsigned y = 1; y < rows; y++)
for (unsigned x = 0; x < cols; x++) {
unsigned src = XY(x, y);
unsigned dst = XY(x, y - 1);
SEGMENT.data[dst] = SEGMENT.data[src];
}

if (hw_random8() > turbulence) {
// create new sparks at bottom row
for (unsigned x = 0; x < cols; x++) {
uint8_t p = hw_random8();
if (p < spark_rate) {
unsigned dst = XY(x, rows - 1);
SEGMENT.data[dst] = 255;
}
}
}

// diffuse
for (unsigned y = 0; y < rows; y++) {
for (unsigned x = 0; x < cols; x++) {
unsigned v = SEGMENT.data[XY(x, y)];
if (x > 0) {
v += SEGMENT.data[XY(x - 1, y)];
}
if (x < (cols - 1)) {
v += SEGMENT.data[XY(x + 1, y)];
}
tmp_row[x] = min(255, (int)(v * 100 / (300 + diffusion)));
}

for (unsigned x = 0; x < cols; x++) {
SEGMENT.data[XY(x, y)] = tmp_row[x];
if (SEGMENT.check1) {
CRGB color = ColorFromPalette(SEGPALETTE, tmp_row[x], 255, NOBLEND);
SEGMENT.setPixelColorXY(x, y, color);
} else {
uint32_t color = SEGCOLOR(0);
SEGMENT.setPixelColorXY(x, y, color_fade(color, tmp_row[x]));
}
}
}
}
return FRAMETIME;
}
static const char _data_FX_MODE_DIFFUSIONFIRE[] PROGMEM =
"Diffusion Fire@!,Spark rate,Diffusion Speed,Turbulence,,Use "
"palette;;Color;;2;pal=35";

void UserFxUsermod::setup() {
strip.addEffect(255, &mode_diffusionfire, _data_FX_MODE_DIFFUSIONFIRE);
}
void UserFxUsermod::loop() {}
uint16_t UserFxUsermod::getId() { return USERMOD_ID_USER_FX; }

static UserFxUsermod user_fx;
REGISTER_USERMOD(user_fx);
12 changes: 12 additions & 0 deletions usermods/user_fx/user_fx.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#pragma once
#include "wled.h"

class UserFxUsermod : public Usermod {
private:
public:
void setup();

void loop();

uint16_t getId();
};
1 change: 1 addition & 0 deletions wled00/const.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@
#define USERMOD_ID_DEEP_SLEEP 55 //Usermod "usermod_deep_sleep.h"
#define USERMOD_ID_RF433 56 //Usermod "usermod_v2_RF433.h"
#define USERMOD_ID_BRIGHTNESS_FOLLOW_SUN 57 //Usermod "usermod_v2_brightness_follow_sun.h"
#define USERMOD_ID_USER_FX 58 //Usermod "user_fx"

//Access point behavior
#define AP_BEHAVIOR_BOOT_NO_CONN 0 //Open AP when no connection after boot
Expand Down