Minicore 3.1.x breaks FastLED library

[memeticae]

On (at least) an arduino nano, i.e. atmega328p ...

Using any version of the FastLED library, every minicore version >= 3.1.x fails to illuminate leds using the FastLED library.
It compiles ok, just show darkness.
Switching to avr, works ok.

I haven't yet found the time to delve deeper, at this time I just use the 3.0.x version.
(Both with and without LTO)

[felias-fogg]

Hi @memeticae

Could you tell me which example is not working, or upload the sketch that you used?

[memeticae]

Hello,

This is just the fastled blink example.

/// @file    Blink.ino
/// @brief   Blink the first LED of an LED strip
/// @example Blink.ino

#include <FastLED.h>

#define NUM_LEDS 1
#define DATA_PIN 7

CRGB leds[NUM_LEDS];

void setup() { 
    FastLED.addLeds<PL9823, DATA_PIN, RGB>(leds, NUM_LEDS);
}

void loop() { 
  // Turn the LED on, then pause
  leds[0] = CRGB::Red;
  FastLED.show();
  delay(500);
  // Now turn the LED off, then pause
  leds[0] = CRGB::Black;
  FastLED.show();
  delay(500);
}

fastled version used 3.10.1
Works, as in the led is blinking, with minicore version 3.0.4
but with versions 3.1x, the led stays dark.
Tried with both LTO enabled and disabled.

As mentioned, this is on an arduino nano, i.e. 328p

[MCUdude]

Interesting. The jump from 3.0.4 to 3.1.x only added support for debugging, so it shouldn't affect code that's been built without debugging enabled @felias-fogg?

[memeticae]

Please note: I'm using PL9823 instead of WS28xx.
Apart from RGB order and some timing differences, this should not affect the fault.

PS Have you been able to reproduce it?

[felias-fogg]

Hi,

I just tried it with a WS2812 and with 3.1.x it shows a constant white instead of blinking red with 3.0.x. I will investigate. As @MCUdude remarked, that should not happen at all!

[felias-fogg]

Simple answer: The problem happens only when Optimize for debugging in the Sketch menu has been activated. So deactivate this option.

So, why does that happen at all?

When optimization for debugging is activated, the compiler tries to stick as much as possible to the structure of the source program instead of optimizing for space, which is the default. This can, of course, change the timing behavior of a program. Indeed, when capturing the signal with a logic analyzer, the signals look quite different. Without Optimize for debuggingactivated, everything looks right. You have a frequency of 800 kHz, and the short and long high pulses are as expected (870 ns and 250 ns). When this option is activated, the frequency is down to 500 kHz, and all high pulses have a length of 500 ns. A cure would be to find the module in FastLED that needs to be compiled with -Os. I have not found that yet, though. Apparently, FastLED uses inline assembler which is immune against such things.

[memeticae]

Ah!
Nice catch!

I hadn't checked that, and indeed it was turned on.
(I have used it once, in a completely different project, but it seems the arduino ide uses it as a global -instead of per project- setting, weird enough.)

For sure, without "optimize for debugging" enabled my own program works again with minicore 3.1.x

It would seem the 328p is just to slow to allow for both debugging and bitbanging the output?
(It has no RMT as the esp32's has.)
So everything has to been done in software.

Thank you very much for taking the time to deduce the problem, and finding a reason for it.

[felias-fogg]

I filed an issue with FASTLed. Let's see what is coming out of that. It would be great to find a solution to allow for debugging even when using FastLED.

[felias-fogg]

I think, I located the problem and know how to fix it: FastLED/FastLED#2000 (comment)

Why this fix works is beyond my understanding

[felias-fogg]

OK, PR submitted.

[memeticae]

Wow! Just wow.

I have looked at the .lst file minicore made, and I saw exactly what you described, every instruction is surrounded by
mov r10,16 mov r16,r10
pairs.

One question though, since the comment in clockless_trinket.h states:

// The variables that our various asm statements use. The same block of variables needs to be declared for
// all the asm blocks because GCC is pretty stupid and it would clobber variables happily or optimize code away too aggressively
#define ASM_VARS : /* write variables */

Is the "&" your solution provides really necessary?
Wouldn't changing all +'s to ='s suffice?
(As per GCC docs: 6.12.3.3 Constraint Modifier Characters)

So the value need not be saved?

I don't know, I'm only self-taught in this stuff.

Anyway, thank you (again) for tackling this, I have learned heaps the past few days.
:)

[felias-fogg]

Hi,

Yes, it might well be that "=" does the job. I never tried. I am still surprised that the "=&a" did the job ;-). I also tried to change all + into =& without much success. I changed the "a" to "r", which should have no bad implications because it is not necessary to have this register as one of the upper ones. But this kills everything.

I guess the kind of generating assembly code done in the file is simply not very compiler-friendly. Usually, you have inputs and the registers are loaded with them, and you have outputs where the register values are stored into memory. The way it is done here relies on the fact that certain variables are kept in registers. And this, in turn, is dependent on the optimization level. So, all in all, a very brittle construction. However, I never understood which variable was associated with r10 that always got loaded into r16.

[memeticae]

I see!

For myself, I do not think the kind of "optimisation" used in FastLED is the way to go.

Why not call out the GCC guys for help if you encounter problems with it?
Maybe they would have just said: "You are using it wrong", and might have offered a better solution.
(This is software, not an iphone4)

Instead of bluntly calling GCC "stupid".
(Which it clearly is not.)

I think I am going to steer away from FastLED, if this is how the developers feel about GCC.
Also, it makes them prone to version dependency, and good luck with that.

It just reminds me to much of this xkcd joke.

Anyway, thanks very much for your help and insight.

It has helped me to steer away from inline assembly (which I have used in some projects), and will help me to just write better code, so the compiler doesn't have to "second guess" my intentions.

(Which is possible, in both C and C++, if one constrains ones code enough to disallow ambiguousness)

Best regards, &c.

[memeticae]

Oh!

But to get to the gist of it: It wasn't minicore that was broken.

It just showed errors elsewhere. (much like Gcc does?)

;)

[felias-fogg]

Hi @memeticae,

Three things. First, sometimes one needs to use assembly code, namely, when timing is tight. For example, when you do bit-banging, as in the case at hand. There is no way around that, in particular, if you want to be independent of the applied optimization level. I would not write it the way it was done in FASTLed, though. However, because of all the corner cases that are covered, I do not see an easy way to rewrite that code. And anyway, I want to complete my GDB server.

Second, as mentioned by the current maintainer, this is ancient code, probably written by the original author, who died a couple of years ago in an accident. Nobody wants to touch it since it works (most of the time), but nobody fully understands it.

Third, the compiler version used in the Arduino context is very dated. The current version is 13; the version used in the Arduino IDE is version 7. So perhaps, with the new version, one would get a completely different result ;-).

And a fourth thing: My solution does not work for the new ATmegas :-(.

Cheers,
Bernhard