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

Make waveform generator a NMI to run always, increase accuracy #5578

Merged
merged 12 commits into from
Jan 11, 2019

Conversation

earlephilhower
Copy link
Collaborator

Make the waveform generator an NMI using the same code as in 2.4.0.
Making it NMI will ensure it runs even when interrupts are disabled.

Fixes #5568

Make the waveform generator an NMI using the same code as in 2.4.0.
Making it NMI will ensure it runs even when interrupts are disabled.

Fixes esp8266#5568
@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Jan 3, 2019

Ran the SpeedTest.ino on SPIFFS in PR #5511 with a 1KHz PWM output on a single pin and it's looking good.

Attempted the WiFi Server test at https://gist.github.com/arunoda/3f17eba4f3f1fd5e1a7adc86e7b62ca7 with a 1KHz output on a single pin as well, TCP v2 hi BW, 160MHz, and got about 670KByte/sec (5.3Mbit/sec) downloads from the 8266. W/o PWM I get ~700KByte/sec, so this ~5% lower perf with PWM

At 10KHz I can get occasional SPIFFS WDT timeouts when writing 512KByte at a time in 256byte chunks (but adding in yields() every 32KB fixes this while reducing write BW by under 10%, so it's not really a showstopper), but WiFi works fine and delivers ~620KBytes/sec (4.96Kbit/sec) doe to increased CPU usage by the PWM generator.

Simple and stupid downloads to the ESP8266 from a local server on my LAN of a 10M file take 51s using simple client code and no PWM and 57s with the same 10khz PWM (so again ~10% lower). No WDT or anything seen with this. Code below, but this is not meant to be a good example snippet, only a straightforward one. :)

  WiFiClient client;
  client.connect("192.168.1.8", 80);
  client.write("GET /zero\r\n\r\n");
  uint32_t start, end;
  Serial.printf("%d start\n", start =millis());
      while(client.status()) {
       const int bufSize = 1460;
       // Initialize a random set of bytes
       byte clientBuf[bufSize];
       client.readBytes((uint8_t *)clientBuf, bufSize);
    }
  Serial.printf("%d end\n", end = millis());
  Serial.printf("delta=%d\n", end - start);

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

This looks ok to me code-wise. I don't know the implications for functionality.

@earlephilhower earlephilhower changed the title Make waveform generator a NMI to run always WIP - Make waveform generator a NMI to run always Jan 4, 2019
@earlephilhower
Copy link
Collaborator Author

Argh, looking over it one last time I realize it's now going to be a problem as-adjusted.

Before we could twiddle with IRQ data structures as long as we bracketed it with disable-irq/.../enable-irq.

For NMI, disable/enable irq doesn't do anything. So we could catch an interrupt while in the middle of changing values and bad stuff will happen.

Need to make it lockless somehow, may be a significant effort.

@earlephilhower earlephilhower force-pushed the wvftonmi branch 3 times, most recently from 6459e89 to 326e701 Compare January 5, 2019 07:52
Make the waveform generator lockless by doing all dangerous structure
updates in the interrupt handler.  start/stopWaveform set a flag and
cause an interrupt.  They wait for the interrupt to complete and clear
those flags before returning.

Also rework the Waveform[] array to be lockless, results in ~48b
additonal heap.

About 40b of IRAM additional code generated vs. non-NMI.

Tweak parameters to make more accurate timing at 80 and 160 MHz.
Try and minimize the IRAM needed to run the IRQ while keeping performance at
or better than before.
Calculate first and last pins to scan for PWM, significantly increasing
accuracy for pulses under 10us at 80MHz.  Now if you are using a single
PWM channel at 80MHz you can generate a 1.125us pulse (down from ~4us).

Rework the IRQ logic to avoid potential WDT errors.  When at 80MHz it
appears that interrupts occuring faster than 10us apart on the timer
cause WDT errors.  Avoid it by increasing the minimum delay between
IRQs on the timer accordingly.

Torture test below passed @ 80 and 160Mhz:
void loop() {
int pin = rand() % 8;
  int val = rand() % 1000;
  switch(pin) {
    case 0: pin = D1; break;
    case 1: pin = D2; break;
    case 2: pin = D3; break;
    case 3: pin = D4; break;
    case 4: pin = D5; break;
    case 5: pin = D6; break;
    case 6: pin = D7; break;
    case 7: pin = D8; break;
  }
  analogWrite(pin, val);
  delay(30);
}
@earlephilhower earlephilhower changed the title WIP - Make waveform generator a NMI to run always Make waveform generator a NMI to run always, increase accuracy Jan 7, 2019
@earlephilhower earlephilhower added this to the 2.5.0 milestone Jan 7, 2019
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_waveform.c Show resolved Hide resolved
stopWaveform may be called from an interrupt (it's called by digitalWrite)
so we can't call delay().  Make it a busy wait for the IRQ to clear the
waveform.

Only set a new timeout of 10us when starting a new waveform when there
is no other event coming sooner than that.

Update formatting and comments per @devyte's requests.

Replace MicrosecondsToCycles() with standard Arduino call.
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Looks ok now, nothing jumps out at me.

@earlephilhower earlephilhower merged commit 8a64a12 into esp8266:master Jan 11, 2019
@earlephilhower earlephilhower deleted the wvftonmi branch January 11, 2019 23:55
@Jason2866
Copy link
Contributor

With this change Tasmota is nearly unresponsible. Before it worked very well.

@earlephilhower
Copy link
Collaborator Author

@Jason2866, please open an issue and be much more specific or we're not going to be able to help you out. Also, are you sure it's related to this specific PR and not #5513 which was noticed at the 1st 2.5.0-dev release. Thx, -EFP3

@Jason2866
Copy link
Contributor

@earlephilhower Thx for your comment. Did more testing. At the end the behaviour was a part defective Wemos Mini!! Everything is fine now :-)

@davidmpye
Copy link

This is causing me a problem :-(

If you try to use something timing critical like FastLED (or Adafruit_Neopixel) and PWM at the same time, you run into issues.

When I write the LED data out, I need to have interrupts disabled, otherwise, the NMI firing causes the FastLED data sequence to glitch, making the LEDs flash randomly. The fastled library suggests disabling interrupts during the write sequence, but obviously this fails if using NMI.

Is it possible to create a #define or similar to set whether the PWM code uses NMI or not?

David

@earlephilhower
Copy link
Collaborator Author

@davidmpye the most compatible is going to be stopWaveform/etc. right before the code in question followed by startWaveform/etc. right after. There may be other code in the processor (WiFi, etc.) which also comes in the binary SDK. This isn't as bad as it sounds since the tone/analogWrite would be garbage while writing the LEDs anyway since w/IRQs disabled they won't do anything at all and will be stuck at full high or low.

Alternately, look in the core_esp8266_waveform.cpp file where it's pretty straightforward to hack code in to make it a normal IRQ (but will fail during SPIFFS writes or other periods when IRQs are disabled for any period).

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.

PWM is blocked during SPIFFS write operations
4 participants