adafruit / Adafruit_DotStar

GNU General Public License v3.0
98 stars 58 forks source link

show: Use BusIO SPI transactions to set clock freq #49

Closed digitalcircuit closed 2 years ago

digitalcircuit commented 2 years ago

In short

Criteria Rank Reason
Impact ★★☆ 2/3 Makes DotStar LEDs even faster for persistence of vision/strobe/etc
Risk ★★☆ 2/3 Applying higher speed may introduce instability
Intrusiveness ★☆☆ 1/3 Small set of changes, shouldn't interfere with other pull requests

Details

The BusIO update in Adafruit_DotStar v1.2.0 specified 8 MHz as the desired SPI frequency. However, it overlooked the need to begin and end transactions on the Adafruit_SPIDevice in order to actually apply this frequency and ensure the settings are correct after any unrelated SPI transactions to other devices.

Alternatively, if avoiding the use of SPI transactions is intentional (e.g. to allow others to customize the SPI frequency before/after calling show()), the DotStar library code should probably have a comment to explain this.

Testing

Steps

  1. Fetch the DotStar library, with or without patches
  2. Compile and upload the following strandtest_perf.ino code
  3. Observe the Arduino serial monitor
  4. Verify LEDs function correctly with no flickering or glitching near the end

Before

Average of 3479 µs delay to update 300 DotStar LEDs.

After

Average of 2270 µs delay to update 300 DotStar LEDs.

Example code

strandtest_perf.ino

// Simple strand test for Adafruit Dot Star RGB LED strip.
// Mostly copied from https://github.com/adafruit/Adafruit_DotStar/blob/master/examples/strandtest/strandtest.ino
// Includes commands to measure timing of updating LED strand

#include <Adafruit_DotStar.h>

// For testing: 300 DotStar pixels
#define NUMPIXELS 300 // Number of LEDs in strip

// For testing: hardware SPI
Adafruit_DotStar strip(NUMPIXELS, DOTSTAR_BGR);

void setup() {
  Serial.begin(115200);     // Initialize to fastest common baud rate

  strip.begin();            // Initialize pins for output
  strip.show();             // Turn all LEDs off ASAP
}

// Runs 10 LEDs at a time along strip, cycling through red, green and blue.

int      head  = 0, tail = -10; // Index of first 'on' and 'off' pixels
uint32_t color = 0xFF0000;      // 'On' color (starts red)

void loop() {
  strip.setPixelColor(head, color); // 'On' pixel at head
  strip.setPixelColor(tail, 0);     // 'Off' pixel at tail

  unsigned long start = micros();   // Track start time
  strip.show();                     // Refresh strip
  Serial.println(micros() - start); // Print time taken

  delay(20);                        // Pause 20 milliseconds (~50 FPS)

  if(++head >= NUMPIXELS) {         // Increment head index.  Off end of strip?
    head = 0;                       //  Yes, reset head index to start
    if((color >>= 8) == 0)          //  Next color (R->G->B) ... past blue now?
      color = 0xFF0000;             //   Yes, reset to red
  }
  if(++tail >= NUMPIXELS) tail = 0; // Increment, reset tail index
}
ladyada commented 2 years ago

@caternuson also take a look since i thiiiink ya did the busioification?

ladyada commented 2 years ago

@digitalcircuit this is one of the nicest, most helpful PR we've seen this year - wish we could give it an award. we look forward to reviewing any other PR's you submit to our repos!

caternuson commented 2 years ago

Nice! This looks good. The busioification really just translated the logic that was already in place - so this has been in the library even before that.

digitalcircuit commented 2 years ago

@ladyada Thank you! I'm glad I was able to help out, and if I find other useful changes or fixes I'll certainly send PRs. Thank you (and the Adafruit team and contributors) for the awesome hardware, guides, and libraries!

@caternuson That makes sense. I recall in 2019 when implementing an SPI speed override for my simple lighting controller firmware (which I reverted after this was merged, yay!) that based on the delay for show() the default speed on an ItsyBitsy M4 wasn't even 6 MHz, let alone 8 MHz due to logic set in place for the Arduino Due. However, I hadn't revisited this until today, and it's quite possible that later changes resulted in 8 MHz being applied more widely and I simply didn't notice.

I repeated the same tests in the first linked commit before submitting this PR, and strangely enough, while 8 MHz caused me slight problems with 300 LEDs over 10 meters in 2019, it's perfectly fine today. Maybe Adafruit's BusIO or changes in other Arduino components made SPI more reliable.

ladyada commented 2 years ago

yep - ideally we want to put all I2C / SPI management logic into adafruit_busio because there are so many finicky platform-specific bugs and requirements. if there's something still wrong with SPI that you encounter - plz open an issue there :)