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.

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

Allow specifying waveform generator in source code #7800

Merged
merged 7 commits into from
Jan 17, 2021

Conversation

earlephilhower
Copy link
Collaborator

Allows code to explicitly specify which waveform generator it wants,
without needing to use one of the 100 IDE menus or adding a -D
compile-time define.

Uses weakrefs to allow for apps to call enablePhaseLockedWaveform();
within their setup() (or anywhere, really) and have the phase locked
versions override the default waveform generators automatically.

For example:

void setup() {
  // Uncomment following line to use phase-locked waveform generator
  // enablePhaseLockedWaveform();
  Serial.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);     // Initialize the LED_BUILTIN pin as an output
  analogWriteRange(1000);
}
void loop() {
  analogWrite(LED_BUILTIN, 100);
  delay(1000);                      // Wait for a second
  analogWrite(LED_BUILTIN, 900);
  delay(2000);                      // Wait for two seconds (to demonstrate the active low LED)
}

Also adds an example showing it's use.

Allows code to explicitly specify which waveform generator it wants,
without needing to use one of the 100 IDE menus or adding a `-D`
compile-time define.

Uses weakrefs to allow for apps to call `enablePhaseLockedWaveform();`
within their `setup()` (or anywhere, really) and have the phase locked
versions override the default waveform generators automatically.

For example:

````
void setup() {
  // Uncomment following line to use phase-locked waveform generator
  // enablePhaseLockedWaveform();
  Serial.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);     // Initialize the LED_BUILTIN pin as an output
  analogWriteRange(1000);
}
void loop() {
  analogWrite(LED_BUILTIN, 100);
  delay(1000);                      // Wait for a second
  analogWrite(LED_BUILTIN, 900);
  delay(2000);                      // Wait for two seconds (to demonstrate the active low LED)
}
````

Also adds an example showing it's use.
@earlephilhower
Copy link
Collaborator Author

@Tech-TX and @dok-net , can you give this a try and make sure I didn't miss a weak call somewhere?

Your existing test apps should work fine, just add or remove a enablePhaseLockedWaveform() in setup and the backend should swap automatically and everything (tone, analogWrite, startWaveform) should "just work."

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Fade example is nice, both version work and the two .elf contents apparently show correct linking.
Great job !

@dok-net
Copy link
Contributor

dok-net commented Dec 31, 2020

@earlephilhower I've created a branch for ease of consumption, that contains my requested changes to your PR: https://github.com/dok-net/arduino-esp8266/tree/pr7800

@dok-net
Copy link
Contributor

dok-net commented Dec 31, 2020

@earlephilhower While I like the simplicity of switching the "modes" that's afforded by your fine PR, I am really concerned that it eats ever more memory:

PR 7800, phase locked:

ICACHE *: 32768           - flash instruction cache
IROM   *: 236352          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27984   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1496  )         - initialized variables (global, static) in RAM\HEAP 
RODATA *: 896   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 26104 )         - zeroed variables      (global, static) in RAM\HEAP

PR 7800, pwm-locked:

ICACHE *: 32768           - flash instruction cache
IROM   *: 236880          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 28176   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1496  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 892   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25672 )         - zeroed variables      (global, static) in RAM\HEAP
 
before, master, phase locked:

ICACHE *: 32768           - flash instruction cache
IROM   *: 236144          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27952   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1492  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 892   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25592 )         - zeroed variables      (global, static) in RAM\HEAP

before, master, pwm-locked:

ICACHE *: 32768           - flash instruction cache
IROM   *: 236416          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 28148   \ 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...)
DATA   *: 1492  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 892   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25672 )         - zeroed variables      (global, static) in RAM\HEAP

@dok-net
Copy link
Contributor

dok-net commented Dec 31, 2020

About the new example, I am afraid there's two problems with the use of PolledTimeout. First, in setup, there exists a more proper way to integrate PolledTimeout and yield, I've used it, was discussed in some other PR. Please consider using the template for that.
Also, I think in loop(), there is rather correlation at work than causation. This is misleading, especially for an example. I'm talking about how the PolledTimeout is supposed to fire at "half phase", but that's in no way deterministic, or am I blinding missing something? Please give it a glance.

Address @dok-net's comments and also remove the _weak/_bound version of
startWaveform() since it's invariant of the actual waveform generator.
@earlephilhower
Copy link
Collaborator Author

Thanks for the feedback, @dok-net . Good catch on the dead code. I've thrown in a new push with those changes and which removes the weak/bound bounce for startWaveform() since it's identical for all versions and just a wrapper for startWaveformClockCycles(). That should help drop a few bytes of code, too.

As for the PolledTimeout, the example is just the existing BlinkPolledTimeout() more or less. I've adjusted the periodic timer name to be more correct. It should be sync'd, more or less, that's what the timeout is doing... The idea was just to show something w/analogWrite(not present in any examples!) simple and easy to grok. We can see about more comprehensive ones later.

@dok-net
Copy link
Contributor

dok-net commented Dec 31, 2020

I still see one forward declaration that I think is redundant :-)

@dok-net
Copy link
Contributor

dok-net commented Dec 31, 2020

@earlephilhower This should be the PolledTimeout template to use for built-in yielding:

using periodicYieldUs = esp8266::polledTimeout::timeoutTemplate<true, esp8266::polledTimeout::YieldPolicy::YieldOrSkip, TimePolicy::TimeFastMicros>;
periodicYieldUs stepPeriod(50000);

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 17, 2021

Is it good to go ?

@earlephilhower
Copy link
Collaborator Author

I have no changes and know of no problems, so good to go from me...

@earlephilhower earlephilhower merged commit f5fd591 into esp8266:master Jan 17, 2021
@earlephilhower earlephilhower deleted the wvfsel branch January 17, 2021 23:57
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