setBrightness(0) does nothing some times

[Karshilistic]

Greetings.

It seems like setting the brightness to 0 (or using the equivalent method turnOff()) don't work in certain cases.
The scenario I am trying to deal with right now is as follow:

• Three lights initialized using the DimmableLight class, defined as an array (code below).
• Turning on any one light works (lights[0].setBrightness(255)).
• Turning on a second light works (lights[1].setBrightness(255)).
• Turning on a third light doesn't work (lights[2].setBrightness(255)). Turning off one of the two previous lights turns on this third light (lights[1].setBrightness(0) causes lights[2] to turn on).

Another simpler case:

• Three lights initialized using the DimmableLight class, defined as an array (code below).
• Turning on light one works (lights[0].setBrightness(255)).
• Turning it off doesnt work (lights[0].setBrightness(0))

I am using an ESP8266 (NodeMCU v1.0 board), arduino framework. Using dimmable_lights v.1.3.0
I also tested with v1.2.0, which works, but flickering is substantial when brightness is set to 255.

Test code:

#include <Arduino.h>
#include "dimmable_light.h"

// NUMBER OF CHANNELS
#define numChannels 3

// PIN SETTINGS
const byte syncPin = 12;
const byte outPin[numChannels] = { 14, 15, 16 };

// LIGHTS SETTINGS
DimmableLight lights[numChannels] = {
  {outPin[0]},
  {outPin[1]},
  {outPin[2]}
};

void setup(void) {
  Serial.begin(115200);
  Serial.println("");

  Serial.print("Init the dimmable light class... ");
  DimmableLight::setSyncPin(syncPin);
  DimmableLight::begin();
  Serial.println("Done!");
  Serial.println(String("Number of instantiated lights: ") + DimmableLight::getLightNumber());
}

void loop(void)
{
  lights[0].setBrightness(255);
  delay(2000);
  lights[0].setBrightness(0);
  delay(2000);
}

[fabianoriccardi]

Hi Karshilistic,
I'm investigating right now, I'll update you as soon as possible.

[Karshilistic]

Awesome! I've been digging in thyristor.cpp for the issue, but it is a little tricky to familiarize myself with someone else's code of this size. The comments are helping a lot. Good job on this.

Let me know where the issue was.

Edit: I have an intuition that this is a problem with a for loop somewhere in which the loop counter (i for example) is initialized after the index of the light in question. I don't know if that makes sense. Anyway, I'm going to continue reading thyristor.cpp in hopes of helping.

[fabianoriccardi]

I've just pushed a couple of commit that should solve your problem.

The bug was in that tangle of code triggered when calling setDelay(microsec). It was necessary a bit of refactoring to simply it and rewrite it. I know how much is difficult to read and understand other's code.

Let me know it solves your problem :)

[Karshilistic]

The test code works great. Testing other use cases and will get back to you.

What do you mean by concurrency issues mentioned in the commit?

[Karshilistic]

Alright I think this solved it, however, I am interested in what you meant by "concurrency issues". Cheers!

[fabianoriccardi]

Finally, thanks for the feedback!
Tomorrow I will revise the old code and I will give a comprehensive answer.

[fabianoriccardi]

Firstly, in setDelay() there is a part of code that aims to optimize the reordering (ascending) of delays. The delay values need to be ordered to ease the next phases when timers must be set.

This ordering is performed in setDelay() to not slow down interrupts, especially zero_cross_int(), which in turn it simply copies the delay values in its own private structure pinDelay.

The concurrency problem arises from the fact that zero_cross_int may be triggered during the reordering phase. To manage this case, I have introduced updatingStruct to check is setDelay() is modifying the structure. If this is the case, the interrupt skips the copy phase and for the current semiperiod it applies the old delay values. Then, when zero_cross_int() is called again, and no reordering is active, the new values will be copied in the pinDelay.

If you call setDelay continuously, you may never be able to apply new delay values (this situation is unlikely in common usage, where other operations are performed between a setDelay and the next one). Personally I don't think this is the best solution, but it is the more elegant solution without condition variables and threads. These 2 constructs are helpful in these kinds of situations. Unfortunately, in Arduino environment, they are not available, apart some board-dependant implementation.

Hoping this may help :)

[fabianoriccardi]

Hi again,
I've just pushed the new version 1.4.0, containing the fix to this bug

[joelgamal]

I am facing the same issue, with version 1.4.0. using NodeMCU, 3 channels, after turning off, one of the three channels remains on

[fabianoriccardi]

Could you post your code?

[Karshilistic]

I have actually noticed on different bulbs the following scenario:

1. Turn on channel 1: lights up correctly
2. Turn off channel 1: doesn't turn off fully, just very low brightness. Didn't show up on previous light bulbs.
3. Turn on channel 2: channel 2 turns on and channel 1 turns off completely now.

Happens with the last light to be closed only. Doesn't happen if one or two other channels are on.

[diegoadan]

Hi fabiuz7

I have a similar issue, I am using only one channel, and randomly when I call setBrightness(255) the thyristor is completely turned off, so I have to call setBrightness(254) as the maximum value to avoid get the device randomly turned off. I have set verbosity level to 3 and the output shows very normal values for pin and delay.

I suspect the "return" in the following block causes the function to exit before the thyristor is activated again:

[Thyristor.cpp]

  if(_allThyristorsOnOff){
    for(int i=0; i<Thyristor::nThyristors; i++){
      if(pinDelay[i].delay==semiPeriodLength-endMargin){
        digitalWrite(pinDelay[i].pin, LOW);
      }else{
        digitalWrite(pinDelay[i].pin, HIGH);
      }
      thyristorManaged++;
    }

#if defined(MONITOR_FREQUENCY)
    if(!Thyristor::frequencyMonitorAlwaysEnabled){
      interruptEnabled = false;
      detachInterrupt(digitalPinToInterrupt(Thyristor::syncPin));
      
      queue.reset();
      total = 0;

      lastTime = 0;
    }
#elif defined(FILTER_INT_MONITOR)
    lastTime = 0;
    interruptEnabled = false;
    detachInterrupt(digitalPinToInterrupt(Thyristor::syncPin));
#else
    interruptEnabled = false;
    detachInterrupt(digitalPinToInterrupt(Thyristor::syncPin));
#endif

    return;
  }

The return in the above block causes next code to not to be executed:

  //Turn on thyristors with 0 delay (always on)
  while(thyristorManaged<Thyristor::nThyristors && pinDelay[thyristorManaged].delay==0){
    digitalWrite(pinDelay[thyristorManaged].pin, HIGH);
    thyristorManaged++;
  }

And as consequence the thyristor remains off until you call setBrightness() again.

Thanks for this great library,