MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.16k stars 19.21k forks source link

[BUG] Neopixels will not consistently change colors #20982

Closed lightmaster closed 3 years ago

lightmaster commented 3 years ago

Bug Description

Neopixels do not change color consistently nor repeatably. For instance, can click the LCD button for the white default, and each time it's clicked, the Neopixel will change. Can Tell it to change to "Blue" and some of the LEDs will change to blue, some to red. This occurs both by gcode and by LCD screen buttons. Also seems to happen irregardless of whether I have 1 Neopixel setup or 2 strips of them setup.

Configuration Files

Marlin config (correct config)

If you've made any other modifications describe them in detail here.

Steps to Reproduce

  1. Configure Neopixel as they are in the attached config files.
  2. Try to change Neopixel to one color/preset.
  3. Change them to the same preset again, with or without changing them to a different preset first.

Expected behavior:

A Neopixel strip should all change to the same color and brightness based on which preset or RGB setting is picked. Setting colors should also be consistent, in that if I pick Preset "A", and then immediately pick Preset "A" again, I shouldn't notice any change on the second click of Preset "A" since they are already on Preset "A".

Actual behavior:

Can tell them to go to the same preset repeatedly and each time the Neopixels will light up differently.

Additional Information

Video showing me sending this command over and over again: M150 R255 U255 B255 P255

ellensp commented 3 years ago

Perhaps it would be an idea to have a #define OVERIDE_F_CPU {frequency in question} in the pins file for this board And in the code a #if defined OVERIDE_F_CPU #define DELAY_NS(x) DELAY_CYCLES((x) * ((OVERIDE_F_CPU) / 1000000UL) / 1000UL) else use the normal #define.

X-Ryl669 commented 3 years ago

Yep. In fact, we could undef F_CPU and define it to the right value so there's less spaghetti code where it's being used. But that means knowing the CPU frequency of all the STM32 boards that Marlin support. Do you know where I could find this information ?

ellensp commented 3 years ago

Add printing F_CPU it to D6 dump. Then you can query it, case by case

X-Ryl669 commented 3 years ago

I'll do that. Meanwhile, I've also created #21048 to collect them. Let's see if it works.

lightmaster commented 3 years ago

Alright, so with the change to using DELAY_CYCLES, I have control of the Neopixels back, but it's not really any better than the default library. Interestingly, if I send a command like M150 R255 U0 B0 P70, first time some pixels will be all red, some all green, some all blue. Send that same command again and they will all be red but too bright. Send it a 3rd time and it'll be closer to what I'd expect from the P70 brightness. If I then send it different colors within a minute or so, they will also look like I'd expect. After waiting a couple minutes, the next command will act like the very first command and mix up the colors.

So sending multiple commands per minute will get it to change like I want, but a few minutes between commands reverts it back to being erratic.

X-Ryl669 commented 3 years ago

Can you send me again your firmware.elf ?

lightmaster commented 3 years ago

Looking at the default neopixel library, it references F_CPU, which is defined in stm32_def.h as SystemCoreClock, which in turn is defined in system_stm32f1xx.c as16000000. I assume that16000000is in Hz, so 16MHz. If this board actually runs at 72MHz, can I try redefiningF_CPUin the neopixel library to72000000` to see if it works better? Is there any risk of frying anything by modifying that value only for the neopixel library?

X-Ryl669 commented 3 years ago

Nope, I've already computed the number of cycles based on 72MHz so if you'v made the change DELAY_NS to DELAY_CYCLES, as requested (I think you have) it'd work as-is.

lightmaster commented 3 years ago

firmware.elf.zip

X-Ryl669 commented 3 years ago

The code is now correct. I can't do anything more without an oscilloscope now. From the behavior you're describing, it looks like that either:

  1. The code is not executed with the same latency every time (since you have a STM32, there is an instruction cache but it should be more or less transparent when run twice in a row). Yet you need to run it 3x to get what you expect.
  2. Something interrupts the NeoPixel::show method. It shouldn't happen since the function starts with a noInterrupt call, but only an oscilloscope could assert this.
  3. The timing are not that good. Again, an oscilloscope would be required to ensure the timings on the pin. You can try to adjust the timing here:

If you use WS2811 leds, the datasheet lists this:

Bit value Hold time (min) Hold time (avg) Hold time (max) Number of cycles at 72MHz
1 (high) 1050 ns 1200ns 1350ns 86
1 (low) 1150 ns 1300ns 1450ns 94
0 (high) 350ns 500ns 650ns 36
0 (low) 1850ns 2000ns 2150ns 144

So you'll have to hack the delay cycles in the code accordingly:

      if(p & bitMask) {
        // data ONE
        // min: 650 typ: 800 max: 950
        GPIO_SET(pin);
        DELAY_CYCLES(86);  //high
        // min: 300 typ: 450 max: 600
        GPIO_CLEAR(pin);
        DELAY_CYCLES(94);  //low
      } else {
        // data ZERO
        // min: 250  typ: 400 max: 550
        GPIO_SET(pin);
        DELAY_CYCLES(36);  //high
        // data low
        GPIO_CLEAR(pin);
        // min: 700 typ: 850 max: 1000
        DELAY_CYCLES(144);
      }

If you have a WS2812b (more likely) then the values should be (in the order above, sorry no time for formatting correctly): One: 58, 32, Zero: 29, 61

Please try with those numbers and report.

lightmaster commented 3 years ago

I have a WS2812b. So for this one you're saying I should use

if(p & bitMask) {
        // data ONE
        // min: 650 typ: 800 max: 950
        GPIO_SET(pin);
        DELAY_CYCLES(58); //DELAY_NS(700);  //high
        // min: 300 typ: 450 max: 600
        GPIO_CLEAR(pin);
        DELAY_CYCLES(32); //DELAY_NS(150);  //low
      } else {
        // data ZERO
        // min: 250  typ: 400 max: 550
        GPIO_SET(pin);
        DELAY_CYCLES(29); //DELAY_NS(100);  //high
        // data low
        // min: 700 typ: 850 max: 1000
        GPIO_CLEAR(pin);
        DELAY_CYCLES(61); //DELAY_NS(750);  //low
      }

This causes me to lose control of them again.

X-Ryl669 commented 3 years ago

Everything here is very fragile. See, the comment says that they should have waited 800ns and the code asked for 700ns delay (and 150ns for a 450ns from datasheet). I think they accounted for the overhead of the GPIO logic and bitshifting code. So, maybe you can try to subtract a small amount to the number I've computed. Use a define so you don't have to change all lines by hand:

#define SUB   8
if(p & bitMask) {
        // data ONE
        // min: 650 typ: 800 max: 950
        GPIO_SET(pin);
        DELAY_CYCLES(58 - SUB); //DELAY_NS(700);  //high
        // min: 300 typ: 450 max: 600
        GPIO_CLEAR(pin);
        DELAY_CYCLES(32 - SUB); //DELAY_NS(150);  //low
      } else {
        // data ZERO
        // min: 250  typ: 400 max: 550
        GPIO_SET(pin);
        DELAY_CYCLES(29 - SUB); //DELAY_NS(100);  //high
        // data low
        // min: 700 typ: 850 max: 1000
        GPIO_CLEAR(pin);
        DELAY_CYCLES(61 - SUB); //DELAY_NS(750);  //low
      }

Without an oscilloscope, it's all trial and error here.

X-Ryl669 commented 3 years ago

Did you try the suggestion above? Having a small value to subtract from the cycle count should make it work again, you'll need to try a few different value (in #define SUB X for many X, but don't got over 22, IMHO) until it's stable enough.

lightmaster commented 3 years ago

16 is the first one that allows for control, but it's less stable than 0. 17 is controllable, but not stable. 18-29 are not quite stable but seem like they are close. I suspect that it's hit a minimum delay and so all of these values are basically the same.

For SUB values greater than 18, I noticed that the first command seems to go to what I'm expecting, but the second and subsequent identical commands make various different LEDs change colors or brightness whereas they should all remain the same. I can also send a command with a low brightness single color like M150 R255 U0 B0 P7 and it will start out brighter than it should with little different colors being used, but the second and subsequent times that that single color, low brightness command is sent, it will go to dim red as in this example command and stay there for each subsequent command. Even after writing all this, I send the M150 R255 U0 B0 P7 command and it still says as dim red. Also, if I change the dim single color, it will immediately go to the color commanded and stay there with repeated commands. It seems that it is mostly stable up to P25 and slowly grows more unstable as the brightness is increased. Somewhere between P35 and P40 it seems to start to fluctuate in colors and brightness and never settles on the commanded values.

Gonna attempt to start adding instead of subtracting to see if I can find a stable value.

X-Ryl669 commented 3 years ago

Try to make a SUB2 define for the second/last delay in each branch of the if. I'm suspecting the other code in the loop is responsible for the instability. I'd increase the SUB2 in that case so it accounts for the latter code.

lightmaster commented 3 years ago

So like this:

      #define SUB 18
      #define SUB2 18
      if(p & bitMask) {
        // data ONE
        // min: 650 typ: 800 max: 950
        GPIO_SET(pin);
        DELAY_CYCLES(58 - SUB); //DELAY_NS(700);  //high
        // min: 300 typ: 450 max: 600
        GPIO_CLEAR(pin);
        DELAY_CYCLES(32 - SUB2); //DELAY_NS(150);  //low
      } else {
        // data ZERO
        // min: 250  typ: 400 max: 550
        GPIO_SET(pin);
        DELAY_CYCLES(29 - SUB); //DELAY_NS(100);  //high
        // data low
        // min: 700 typ: 850 max: 1000
        GPIO_CLEAR(pin);
        DELAY_CYCLES(61 - SUB2); //DELAY_NS(750);  //low
      }

And then keep increasing SUB2?

X-Ryl669 commented 3 years ago

Yep.

lightmaster commented 3 years ago

I'm at work for the night, so I'll try that tomorrow. SUB2 can't get any larger than 32, correct, since DELAY_CYCLES(32 - SUB2) can't be negative, right?

X-Ryl669 commented 3 years ago

Yes. It'd be easier with an oscilloscope, you'd be able to actually measure the timing here. There is this kind of tool that's not too expensive and very useful for stuff like this. It's sad I don't have the same board as you, I could have done the measurement here.

lightmaster commented 3 years ago

Unfortunately with crappy hours at work, right now is not the best time to spend extra money, so gonna have to make do without it.

X-Ryl669 commented 3 years ago

No problem. I can't afford buying another 3d printer board either. Hopefully you'll sort the timing out by trial and error.

lightmaster commented 3 years ago

Would there be any reason to try increasing the DELAY_CYCLES values rather than decreasing them?

X-Ryl669 commented 3 years ago

If the printer is overclocked (not running at 72MHz), you'd need to increase them. Else, you'll get out-of-spec timing and the led won't accept the timing anymore either, so no, there is no other reason to increase them...

X-Ryl669 commented 3 years ago

Did you get any progress on this ?

lightmaster commented 3 years ago

Still working on testing all the permutations of SUB and SUB2 greater than 18. Takes quite a while when you only have like 2 hours a day to work on it and you have to make change, compile, copy to SD card and flash, test... Rinse and repeat.

lightmaster commented 3 years ago

@X-Ryl669 If I wanted to take the oscilloscope approach to measuring the timing, would this be a sufficient oscilloscope, or is the price "too good to be true"? https://www.amazon.com/Quimat-Oscilloscope-BNC-Clip-Assembled-Finished/dp/B07QML4LJL/

X-Ryl669 commented 3 years ago

There is no data on the input bandwidth (not to be confused with the sampling rate). Yet, the minimum displayed time division is 10µs and you want to observe signals that are a lot smaller than that (100ns) so even if it is able to sample such signal, it won't display it (so you will not be able to test them). In order to observe & measure the NeoPixel signal, you must use a oscilloscope that's going down to 50ns (Nyquist frequency for a 100ns signal), so that's 20MHz of input bandwidth (minimum).

In the minimum price, you'll find: This one that's reaching 120MHz for $50 This one that's more compact with 100MHz for $50

I have the previous version from the DS0120M and it works correctly (not as good as those used at work, but they cost 20x more at least). It's enough for general electronic development tasks IMHO.

lightmaster commented 3 years ago

That second link, the $50 "color" shows a white meter that lists a 5MHz bandwidth in the picture. Isn't that too low?

X-Ryl669 commented 3 years ago

Yes it's too low. You should go for the first link. Or wait until I've finished rewriting the Neopixel code in Marlin so we don't rely on such magical number but on actual hardware timer instead...

github-actions[bot] commented 3 years ago

This issue has had no activity in the last 60 days. Please add a reply if you want to keep this issue active, otherwise it will be automatically closed within 10 days.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.