Waveform: int/uint8_t inconsistency and implied stop PWM#8008
Waveform: int/uint8_t inconsistency and implied stop PWM#8008dok-net wants to merge 12 commits intoesp8266:masterfrom
Conversation
dok-net
force-pushed
the
unifypwm
branch
6 times, most recently
from
36cab6f to
102091c
Compare
dok-net
force-pushed
the
unifypwm
branch
5 times, most recently
from
e191c8f to
3108e68
Compare
earlephilhower
left a comment
earlephilhower
left a comment
There was a problem hiding this comment.
Some logic changes I don't think are correct here.
Also, please try not to mix naming/indentation/style changes with logic changes. It makes reviewing the PR very difficult because multiple unrelated things are all changing at once. There are probably 30 real logic/type change lines in this PR, but it's a few hundred lines due to formatting and renaming changes (which GH diff struggles to match, making it a real pain to examine). Simple change 1-thing PRs are way faster/easier for 3rd parties to look at and review.
There was a problem hiding this comment.
The noinline attribute was added here to save a few bytes of program space...
There was a problem hiding this comment.
It's in a #pragma GCC optimize ("Os") code section anyway. Let the compiler decide?
There was a problem hiding this comment.
Re-added the noinline attribute.
There was a problem hiding this comment.
That could be 10 milliseconds away. forceTimerInterrupt would make it happen in 10 microseconds. I think this needs to be undone.
There was a problem hiding this comment.
I think: the ISR does NOT act on the request immediately when the ISR is triggered, but only as scheduled previously, anyway, because of !pwmState.cnt:
} else if (!pwmState.cnt && pwmState.pwmUpdate) {
// Start up the PWM generator by copying from the mailbox
pwmState.cnt = 1;
pwmState.idx = 1; // Ensure copy this cycle, cause it to start at t=0
pwmState.nextServiceCycle = GetCycleCountIRQ(); // Do it this loop!
// No need for mem barrier here. Global must be written by IRQ exit
}
There was a problem hiding this comment.
So, it's a tradeoff, when it's the first PWM starting, you're right, if anything else is running already, it's wasting resources and may very badly affect PWM/waveform timings.
dok-net
force-pushed
the
unifypwm
branch
6 times, most recently
from
eb8ff9d to
e2fdd64
Compare
|
@s-hadinger I am curious if you would be willing to spend some time on giving an opinion with regard to this PR and/or #8011 performing with LED strips. Real-life use cases are the most interesting, after all :-) |
|
Sorry I'm not familiar with this code. I'm afraid I can't help. |
|
@s-hadinger I am sorry, too, I was just going by things like #7231 (comment) and #7057 and all your much appreciated feedback to #7022 |
dok-net
force-pushed
the
unifypwm
branch
2 times, most recently
from
3dd3be5 to
e54ea30
Compare
dok-net
force-pushed
the
unifypwm
branch
2 times, most recently
from
e577089 to
4b7e7c1
Compare
dok-net
force-pushed
the
unifypwm
branch
2 times, most recently
from
5f225b4 to
d54642c
Compare
dok-net
force-pushed
the
unifypwm
branch
3 times, most recently
from
de95750 to
3ee14cb
Compare
startWaveform already implied stopping PWM before.
…plemented on next PWM period begin anyway.
…ick up new starts at next regular interval.
3 participants
@earlephilhower This is basically a cleanup, and I'm asking for you to review this, because it's your code.
I'm head scratching about the weak-ref magic you performed, it looks convoluted, but appears to be the only working variant - albeit I am worried that
enablePhaseLockedWaveform();does not remove the PWM bits from the final BSP like it was with the previous config menu?