cpldcpu / light_ws2812

Light weight library to control WS2811/WS2812 based LEDS and LED Strings for 8-Bit AVR microcontrollers.
GNU General Public License v3.0
946 stars 207 forks source link

Unable to use light_ws2812 with USBMIDI #93

Closed svanimisetti closed 3 years ago

svanimisetti commented 3 years ago

When using this library with USBMIDI with Digispark board (ATtiny85) and on Arduino IDE, I am getting the following error.

Capture

If I use both libraries independently, I have no issues. I created a small test sketch to demonstrate the issue. See following code snippet. Please comment/uncomment first two lines to test different combinations. I also tested the case where the WS2812 setOutput() function was called after some delay (millis() > 10000) to see if it helps as USBMIDI is started using bit-bang after a certain delay. However, even that did not help.

#define INCL_USBMIDI
#define INCL_WS2812

#ifdef INCL_USBMIDI
#include <usbmidi.h>
#endif

#ifdef INCL_WS2812
#include <WS2812.h>
WS2812 LED(1); // 1 LED
cRGB value;
#endif

void setup() {
#ifdef INCL_WS2812
  LED.setOutput(5); // Digital Pin 9
#endif
}

void loop() {

#ifdef INCL_USBMIDI
  // echo back any midi input
  USBMIDI.poll();
  while (USBMIDI.available()) {
    USBMIDI.write(USBMIDI.read());
    USBMIDI.flush();
  }
#endif

#ifdef INCL_WS2812
  value.b = 10; value.g = 0; value.r = 0; // RGB Value -> Blue
  LED.set_crgb_at(0, value); // Set value at LED found at index 0
  LED.sync(); // Sends the value to the LED
  delay(500); // Wait 500 ms

  value.b = 0; value.g = 0; value.r = 10; // RGB Value -> Red
  LED.set_crgb_at(0, value); // Set value at LED found at index 0
  LED.sync(); // Sends the value to the LED
  delay(500); // Wait 500 ms
#endif

}

Any suggestions or recommendations to fix this are much appreciated. If there is a fundamental architectural issue that prevents these two libraries from being used together - please do advise at the earliest - I will try to move to a different development board where space is not a constraint (so that I can use Adafruit's NeoPixel library) and USB support is available in hardware. Incidentally, the same issue is repeatable with Adafruit NeoPixel library on this board.

cpldcpu commented 3 years ago

The problem is that the WS2812 has to block interrupts to ensure that the timing of the protocol is met. V-USB heavily relies on interrupts to react to USB traffic. Whenever there is USB traffic coming in while data is sent to the WS2812 there will be an error.

Some options, none of them are good or easy:

1) Change WS2812 library so interrupts are not blocked. This will mean that USB works, but your data transmission to the WS2812 string may be corrupted some times or often

2) Reduce WS2812 string length to a very short one. (EDIT: Looks like you already did this)

3) Switch to a microcontroller with hardware USB support or use the serial port (best option, in my opinion).

It may also be possible to solve this using low level timing hacks, but then you should not start with the Arduino libraries.

svanimisetti commented 3 years ago

@cpldcpu - Thanks for the quick response. I did have a hunch that this has something to do with timing or interrupt issues. My application is not timing sensitive - in the sense that a USB packet is received every 5-10 seconds - and then the lights are processed (1-5 lights turned on at any given time) every 5-6 seconds.

My initial implementation was using a Pro Micro (ATmega32u4) - and everything was working fine. I just wanted to optimize since I was only using 40% memory and 4 pins on that board. I thought Digispark was a perfect fit - as my code occupies 96% memory and uses all pins on the board - and was perfect fit for my application.

Before I completely give up on Digispak - I would like to attempt (1) from your response. I am not experienced enough so any initial pointers to modify WS2812 library to not block interrupts will be greatly appreciated. I can test - and if I see it unusable for my application, I will move to ATmega32u4.

cpldcpu commented 3 years ago

Well, essentially you need to comment out this line:

https://github.com/cpldcpu/light_ws2812/blob/cd149996012fe96bc3d7883cb18a0103fd8e8b3a/light_ws2812_Arduino/light_WS2812/light_ws2812.cpp#L95

There is a high probility that it wont work at all, though...

If you only seldomly send data by USB you could also try to wait for a little while before updating the WS2812. Possibly there are more USB packets to be processed after receiving the data.

svanimisetti commented 3 years ago

Thanks! I did exactly that after reading your response in the morning - but unfortunately it did not work - it still does not recognize the USB device. I am going to poke around some more and see. I also observed some flicker on the pixels, and I guess it goes back to your point about transmission corruption.

svanimisetti commented 3 years ago

I finally figured it out. The example was not working as the delay() function messes around with the USBMIDI timing. I turned off interrupts and modified the example to use millis() instead of delay() so that the interrupts are honored properly. Now my application works as expected. Thanks you so much @cpldcpu!

svanimisetti commented 3 years ago

Documenting for future self and others who may be interested. Commenting line 95 in light_ws2812.cpp and using the following test script - I am able to run both USBMIDI and blink neopixel without any noticeable flicker or issues. I understand that this is very specific to my application and removing cli() is not an ideal option - but this works just fine for me! Thanks again!

#include <usbmidi.h>
#include <WS2812.h>

WS2812 LED(1); // 1 LED
cRGB value;
unsigned long int tstamp;
bool red_on = false;

void setup() {
  LED.setOutput(5); // Digital Pin 9
}

void loop() {

  // echo back any midi input
  USBMIDI.poll();
  while (USBMIDI.available()) {
    USBMIDI.write(USBMIDI.read());
    USBMIDI.flush();
  }

  if(millis()-tstamp>500 && !red_on) {
    value.b = 10; value.g = 0; value.r = 0; // RGB Value -> Blue
    LED.set_crgb_at(0, value); // Set value at LED found at index 0
    LED.sync(); // Sends the value to the LED
    tstamp = millis();
    red_on = true;
  }

  if(millis()-tstamp>500 && red_on) {
    value.b = 0; value.g = 0; value.r = 10; // RGB Value -> Red
    LED.set_crgb_at(0, value); // Set value at LED found at index 0
    LED.sync(); // Sends the value to the LED
    //delay(500); // Wait 500 ms
    tstamp = millis();
    red_on = false;
  }

}
svanimisetti commented 3 years ago

After some more testing - it appears that the limit of pixel count is 39 before the USBMIDI interrupts kill the NeoPixel update process. Also noticed that for pixel count >1, all the pixels start bright white. I could not trace this to any part of code where cRGB is set to (255,255,255). Is this by design? Is the user expected to clear the pixels?

svanimisetti commented 3 years ago

Closing this issue as I took a different approach and wrote a simple protocol handler from ground up based on @bigjosh's SimpleNeoPixel implementation. It turned out to be extremely lightweight (638 bytes of program space and 14 bytes of dynamic memory). So when I use this with USBMIDI on a Digispark it consumes no more than 75% program space - pretty neat! Since this is stream based (rather than frame based), its extremely fast too. The interrupts are cleared (cli) and set (sei) very locally for each stream update - so it also does not affect USBMIDI. I have not had any issues with large strips (upwards of 144 pixels) - and technically it can support 1000s of LEDs. A stream based approach works for me as the refresh happens every 0.5-1 second. Thanks for your help and prompt recommendations.