Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved framerate control code - strip.show(), strip.service() #4244

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Improved framerate control code - strip.show(), strip.service() #4244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
451cd4c
Improved framerate control in strip.show(), strip.service()
softhack007 Nov 1, 2024
891ea48
Merge remote-tracking branch 'upstream/0_15' into framerate_ac015
softhack007 Nov 2, 2024
50934e6
adressing some review comments
softhack007 Nov 2, 2024
cf1630a
0 FPS = unlimited
softhack007 Nov 4, 2024
029293a
simplify sheduler logic
softhack007 Nov 4, 2024
ab7b2d7
use class="warn" for unlimited mode message
softhack007 Nov 5, 2024
1e761c3
simpler hight FPS warning
softhack007 Nov 6, 2024
0404ec9
changes in response to feedback from @willmmiles
softhack007 Nov 7, 2024
001e2ad
adjust audioreactive for the new FRAME_DELAY logic
softhack007 Nov 8, 2024
6790f8a
Same MIN_FRAME_DELAY=3 for -C3 and -S2
softhack007 Nov 21, 2024
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
6 changes: 3 additions & 3 deletions usermods/audioreactive/audio_reactive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ static uint8_t soundAgc = 0; // Automagic gain control: 0 - n
//static float volumeSmth = 0.0f; // either sampleAvg or sampleAgc depending on soundAgc; smoothed sample
static float FFT_MajorPeak = 1.0f; // FFT: strongest (peak) frequency
static float FFT_Magnitude = 0.0f; // FFT: volume (magnitude) of peak frequency
static bool samplePeak = false; // Boolean flag for peak - used in effects. Responding routine may reset this flag. Auto-reset after strip.getMinShowDelay()
static bool samplePeak = false; // Boolean flag for peak - used in effects. Responding routine may reset this flag. Auto-reset after strip.getFrameTime()
static bool udpSamplePeak = false; // Boolean flag for peak. Set at the same time as samplePeak, but reset by transmitAudioData
static unsigned long timeOfPeak = 0; // time of last sample peak detection.
static uint8_t fftResult[NUM_GEQ_CHANNELS]= {0};// Our calculated freq. channel result table to be used by effects
Expand Down Expand Up @@ -536,8 +536,8 @@ static void detectSamplePeak(void) {
#endif

static void autoResetPeak(void) {
uint16_t MinShowDelay = MAX(50, strip.getMinShowDelay()); // Fixes private class variable compiler error. Unsure if this is the correct way of fixing the root problem. -THATDONFC
if (millis() - timeOfPeak > MinShowDelay) { // Auto-reset of samplePeak after a complete frame has passed.
uint16_t peakDelay = max(uint16_t(50), strip.getFrameTime());
if (millis() - timeOfPeak > peakDelay) { // Auto-reset of samplePeak after at least one complete frame has passed.
samplePeak = false;
if (audioSyncEnabled == 0) udpSamplePeak = false; // this is normally reset by transmitAudioData
}
Expand Down
14 changes: 11 additions & 3 deletions wled00/FX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@
#define WLED_FPS 42
#define FRAMETIME_FIXED (1000/WLED_FPS)
#define FRAMETIME strip.getFrameTime()
#if defined(ARDUINO_ARCH_ESP32) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S2)
#define MIN_FRAME_DELAY 2 // minimum wait between repaints, to keep other functions like WiFi alive
#elif defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
#define MIN_FRAME_DELAY 3 // S2/C3 are slower than normal esp32, and only have one core
#else
#define MIN_FRAME_DELAY 8 // 8266 legacy MIN_SHOW_DELAY
#endif
#define FPS_UNLIMITED 0

/* each segment uses 82 bytes of SRAM memory, so if you're application fails because of
insufficient memory, decreasing MAX_NUM_SEGMENTS may help */
Expand All @@ -68,8 +76,6 @@
assuming each segment uses the same amount of data. 256 for ESP8266, 640 for ESP32. */
#define FAIR_DATA_PER_SEG (MAX_SEGMENT_DATA / strip.getMaxSegments())

#define MIN_SHOW_DELAY (_frametime < 16 ? 8 : 15)

#define NUM_COLORS 3 /* number of colors per segment */
#define SEGMENT strip._segments[strip.getCurrSegmentId()]
#define SEGENV strip._segments[strip.getCurrSegmentId()]
Expand Down Expand Up @@ -739,6 +745,7 @@ class WS2812FX { // 96 bytes
customMappingTable(nullptr),
customMappingSize(0),
_lastShow(0),
_lastServiceShow(0),
_segment_index(0),
_mainSegment(0)
{
Expand Down Expand Up @@ -837,7 +844,7 @@ class WS2812FX { // 96 bytes
getMappedPixelIndex(uint16_t index) const;

inline uint16_t getFrameTime() const { return _frametime; } // returns amount of time a frame should take (in ms)
inline uint16_t getMinShowDelay() const { return MIN_SHOW_DELAY; } // returns minimum amount of time strip.service() can be delayed (constant)
inline uint16_t getMinShowDelay() const { return MIN_FRAME_DELAY; } // returns minimum amount of time strip.service() can be delayed (constant)
inline uint16_t getLength() const { return _length; } // returns actual amount of LEDs on a strip (2D matrix may have less LEDs than W*H)
inline uint16_t getTransition() const { return _transitionDur; } // returns currently set transition time (in ms)

Expand Down Expand Up @@ -949,6 +956,7 @@ class WS2812FX { // 96 bytes
uint16_t customMappingSize;

unsigned long _lastShow;
unsigned long _lastServiceShow;

uint8_t _segment_index;
uint8_t _mainSegment;
Expand Down
25 changes: 17 additions & 8 deletions wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1309,7 +1309,14 @@ void WS2812FX::finalizeInit() {
void WS2812FX::service() {
unsigned long nowUp = millis(); // Be aware, millis() rolls over every 49 days
now = nowUp + timebase;
if (nowUp - _lastShow < MIN_SHOW_DELAY || _suspend) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if removing this line was a good move. MIN_SHOW_DELAY is equal to 3 which is 1 ms more than what's used currently on ESP32 (see line 1315 below).

The more I look into this the more I have the feeling that MIN_SHOW_DELAY should be respected at all cost. But the value of it is debatable (though I think that a values of 8 AKA 125 FPS or 16 AKA 62.5 FPS are good values for ESP32 and ESP8266 respectively).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, using MIN_SHOW_DELAY here means that user can get up to 62 fps, or 125 fps, but nothing else. Why should we limit users this way? Do our users need a nanny?

Technicially the new code is better, because it achieves any FPS chosen by the user, its efficient on CPU time, and it's safe enough on the slower chips to keep wifi running well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should we limit users this way? Do our users need a nanny?

IMO yes, you should limit because it makes code more/easily predictable. And yes, most users "need a nanny". Just look at the comments on Discord or forum. But that's just my opinion. Do as you please.

BTW I did not comment code efficiency. I was merely wondering why changing MIN_SHOW_DELAY and not using that in comparisons throughout the code (instead you use "magic" numbers as @netmindz once said to me).

Copy link
Collaborator Author

@softhack007 softhack007 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

instead you use "magic" numbers as @netmindz once said to me).

In this case, I think the origin and purpose of "2" and "3" is well explained in the PR discussion - there might not be a solid proof of the need, but this is not always necessary imho. Also there are hints in the code comments about the "intend" where these numbers are used. If you have a good idea for names of constants that contain "2" and "3", I'm all ears.

The previous "8" and "16" are legacy values, and afaik nobody here could explain any more where they came from - we guessed that 8 and 16 might help slower CPUs like 8266. WS2812FX uses "2" (both for esp32 and esp8266) and "10" (for AVR arduino).

Let's wait until 0.15.0 is release before merging this PR, so we can get feedback from beta testers. I'll be here to take care of problems that might be related to the change. We can always fine-tune parameters. I think that measurements from @dosipod (see above ) clearly show that this PR has a value for users.

if (_suspend) return;
unsigned long elapsed = nowUp - _lastServiceShow;

if (elapsed <= MIN_FRAME_DELAY) return; // keep wifi alive - no matter if triggered or unlimited
if ( !_triggered && (_targetFps != FPS_UNLIMITED)) { // unlimited mode = no frametime
if (elapsed < _frametime) return; // too early for service
}

bool doShow = false;

_isServicing = true;
Expand All @@ -1326,7 +1333,7 @@ void WS2812FX::service() {
if (!seg.isActive()) continue;

// last condition ensures all solid segments are updated at the same time
if (nowUp > seg.next_time || _triggered || (doShow && seg.mode == FX_MODE_STATIC))
if (nowUp >= seg.next_time || _triggered || (doShow && seg.mode == FX_MODE_STATIC))
{
doShow = true;
unsigned frameDelay = FRAMETIME;
softhack007 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -1376,15 +1383,16 @@ void WS2812FX::service() {
_triggered = false;

#ifdef WLED_DEBUG
if (millis() - nowUp > _frametime) DEBUG_PRINTF_P(PSTR("Slow effects %u/%d.\n"), (unsigned)(millis()-nowUp), (int)_frametime);
if ((_targetFps != FPS_UNLIMITED) && (millis() - nowUp > _frametime)) DEBUG_PRINTF_P(PSTR("Slow effects %u/%d.\n"), (unsigned)(millis()-nowUp), (int)_frametime);
#endif
if (doShow) {
yield();
Segment::handleRandomPalette(); // slowly transition random palette; move it into for loop when each segment has individual random palette
show();
_lastServiceShow = nowUp; // update timestamp, for precise FPS control
}
#ifdef WLED_DEBUG
if (millis() - nowUp > _frametime) DEBUG_PRINTF_P(PSTR("Slow strip %u/%d.\n"), (unsigned)(millis()-nowUp), (int)_frametime);
if ((_targetFps != FPS_UNLIMITED) && (millis() - nowUp > _frametime)) DEBUG_PRINTF_P(PSTR("Slow strip %u/%d.\n"), (unsigned)(millis()-nowUp), (int)_frametime);
softhack007 marked this conversation as resolved.
Show resolved Hide resolved
#endif
}

Expand All @@ -1404,13 +1412,13 @@ void WS2812FX::show() {
// avoid race condition, capture _callback value
show_callback callback = _callback;
if (callback) callback();
unsigned long showNow = millis();

// some buses send asynchronously and this method will return before
// all of the data has been sent.
// See https://github.com/Makuna/NeoPixelBus/wiki/ESP32-NeoMethods#neoesp32rmt-methods
BusManager::show();

unsigned long showNow = millis();
size_t diff = showNow - _lastShow;
size_t fpsCurr = 200;
if (diff > 0) fpsCurr = 1000 / diff;
Expand All @@ -1436,8 +1444,9 @@ uint16_t WS2812FX::getFps() const {
}

void WS2812FX::setTargetFps(uint8_t fps) {
if (fps > 0 && fps <= 120) _targetFps = fps;
_frametime = 1000 / _targetFps;
if (fps <= 250) _targetFps = fps;
if (_targetFps > 0) _frametime = 1000 / _targetFps;
else _frametime = MIN_FRAME_DELAY; // unlimited mode
}

void WS2812FX::setMode(uint8_t segid, uint8_t m) {
Expand Down Expand Up @@ -1485,7 +1494,7 @@ void WS2812FX::setBrightness(uint8_t b, bool direct) {
BusManager::setBrightness(b);
if (!direct) {
unsigned long t = millis();
if (_segments[0].next_time > t + 22 && t - _lastShow > MIN_SHOW_DELAY) trigger(); //apply brightness change immediately if no refresh soon
if (_segments[0].next_time > t + 22 && t - _lastShow > MIN_FRAME_DELAY) trigger(); //apply brightness change immediately if no refresh soon
}
}

Expand Down
10 changes: 9 additions & 1 deletion wled00/data/settings_leds.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,11 @@
gId('psu').innerHTML = s;
gId('psu2').innerHTML = s2;
gId("json").style.display = d.Sf.IT.value==8 ? "" : "none";

// show/hide FPS warning messages
gId('fpsNone').style.display = (d.Sf.FR.value == 0) ? 'block':'none';
gId('fpsWarn').style.display = (d.Sf.FR.value == 0) || (d.Sf.FR.value >= 80) ? 'block':'none';
gId('fpsHigh').style.display = (d.Sf.FR.value >= 80) ? 'block':'none';
}
function lastEnd(i) {
if (i-- < 1) return 0;
Expand Down Expand Up @@ -870,7 +875,10 @@ <h3>Advanced</h3>
<option value="2">Linear (never wrap)</option>
<option value="3">None (not recommended)</option>
</select><br>
Target refresh rate: <input type="number" class="s" min="1" max="120" name="FR" required> FPS
Target refresh rate: <input type="number" class="s" min="0" max="250" name="FR" oninput="UI()" required> FPS
<div id="fpsNone" class="warn" style="display: none;">&#9888; Unlimited FPS Mode is experimental &#9888;<br></div>
<div id="fpsHigh" class="warn" style="display: none;">&#9888; High FPS Mode is experimental.<br></div>
<div id="fpsWarn" class="warn" style="display: none;">Please <a class="lnk" href="sec#backup">backup</a> WLED configuration and presets first!<br></div>
<hr class="sml">
<div id="cfg">Config template: <input type="file" name="data2" accept=".json"><button type="button" class="sml" onclick="loadCfg(d.Sf.data2)">Apply</button><br></div>
<hr>
Expand Down
2 changes: 1 addition & 1 deletion wled00/data/settings_sec.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ <h2>Security & Update setup</h2>
<h3>Software Update</h3>
<button type="button" onclick="U()">Manual OTA Update</button><br>
Enable ArduinoOTA: <input type="checkbox" name="AO">
<hr>
<hr id="backup">
<h3>Backup & Restore</h3>
<div class="warn">&#9888; Restoring presets/configuration will OVERWRITE your current presets/configuration.<br>
Incorrect upload or configuration may require a factory reset or re-flashing of your ESP.</div>
Expand Down