Split All Lights Off /Restore

[thekameleon]

It is a little confusing what All Lights Off does, is it suppose to restore the lights to normal operation or turn them off. I think the feature should be separated into two. Ability to restore the lights to normal operation and the ability to just turn them all off (All Lights Off)

[melovictor]

The way I see it is...

Menu "Lights on" - LightsOn() Menu "Lights off" - LightsOff() Menu "Lights auto"(Restore lights) - LightsAuto()

Is this what you are suggesting? The only problem I see is that if the user forgets to set it back to "Lights auto"...well...lights will be either always on ot always off...

Let's see what other people have to say...

Cheers, Victor

[robertoimai]

Yeah, what you mentioned should work. It's just that it can be confusing when you see Lights Off and you expect it to turn your lights off, but all it does is put it back to auto. So, creating the auto version, we can make the off be a true off.

[melovictor]

Help with bitwise operations!

Overrider -> Relay.RelayMaskOn &= ~LightsOnPorts; Turn on -> Relay.RelayMaskOn |= LightsOnPorts; Turn off -> ???

Or should we be setting RelayMaskOff instead?

Learning, learning, learning....

[robertoimai]

Just use bitSet and bitClear. It's a lot easier to read and understand :) For Auto, we set maskoff and clear maskon For On, we set maskon For Off, we clear maskoff

[melovictor]

EDITED: Here is a scratch...might need ajustments...questions on the bottom...

void ReefAngelClass::LightsOff()
{
    // reset ports
    //Relay.RelayMaskOn &= ~LightsOnPorts;
    bitClear(Relay.RelayMaskOff, LightsOnPorts);    
#ifdef RelayExp
    for ( byte i = 0; i < MAX_RELAY_EXPANSION_MODULES; i++ )
    {
        //Relay.RelayMaskOnE[i] &= ~LightsOnPortsE[i];
        bitClear(Relay.RelayMaskOffE[i], LightsOnPortsE[i]);
    }
#endif  // RelayExp
#if defined DisplayLEDPWM && !defined REEFANGEL_MINI
#if defined(__SAM3X8E__)
    VariableControl.SetActinic(0);
    VariableControl.SetDaylight(0);
#else
    PWM.SetActinic(0);
    PWM.SetDaylight(0);
#endif
#endif  // defined DisplayLEDPWM && !defined REEFANGEL_MINI
    Relay.Write();
    bitClear(StatusFlags, LightsOnFlag);
#if defined RA_TOUCH || defined RA_TOUCHDISPLAY
    menu_button_functions1[4] = &ReefAngelClass::LightsOn;
    if (DisplayedMenu==TOUCH_MENU)
        SetDisplayedMenu(DEFAULT_MENU);
#endif  // RA_TOUCH
#ifdef RA_TOUCHDISPLAY
    SendMaster(MESSAGE_COMMAND,COMMAND_LIGHTS_OFF,0);
#endif // RA_TOUCHDISPLAY
}

And what about the LightsOnFlag? Auto - bitClear(StatusFlags, LightsOnFlag); On - bitSet(StatusFlags,LightsOnFlag); Off - should I clear it as well???

Auto - menu_button_functions1[4] = &ReefAngelClass::LightsOn; On - menu_button_functions1[4] = &ReefAngelClass::LightsOff; Off - ??? Why do we need to change the reference? I don't get it...can someone explain?

Victor

[robertoimai]

Yes, Off should clear Off - bitClear(StatusFlags, LightsOnFlag); For the menu, I think we should have: Auto - menu_button_functions1[4] = &ReefAngelClass::LightsOn; On - menu_button_functions1[4] = &ReefAngelClass::LightsAuto; Off - menu_button_functions1[4] = &ReefAngelClass::LightsAuto; And we would need a LightsAuto() function.

[melovictor]

"It's just that it can be confusing when you see Lights Off and you expect it to turn your lights off, but all it does is put it back to auto." The idea here is to rename the old LightsOff() function to LightsAuto() and then create a brand new LightsOff() in which will actually shut the lights off. My only concern is that if the user sets the lights off and forgets about it...because then they will never go on again....I think we need to discuss that...Maybe add an alert sign to the head unit's display and/or LED...What do you think?

[melovictor]

Code changes condensation for more clear analysis

LightsOff()
    bitClear(Relay.RelayMaskOff, LightsOnPorts);
    bitClear(Relay.RelayMaskOffE[i], LightsOnPortsE[i]);
    bitClear(StatusFlags, LightsOnFlag);
    menu_button_functions1[4] = &ReefAngelClass::LightsAuto;

LightsAuto()
    bitSet(Relay.RelayMaskOff, LightsOnPorts);
    bitClear(Relay.RelayMaskOn, LightsOnPorts);
    bitSet(Relay.RelayMaskOffE[i], LightsOnPortsE[i]);
    bitClear(Relay.RelayMaskOnE[i], LightsOnPortsE[i]);
    bitClear(StatusFlags,LightsOnFlag);
    menu_button_functions1[4] = &ReefAngelClass::LightsOn;

LightsOn()
    bitSet(Relay.RelayMaskOn, LightsOnPorts);
    bitSet(Relay.RelayMaskOnE[i], LightsOnPortsE[i]);
    bitSet(StatusFlags,LightsOnFlag);
    menu_button_functions1[4] = &ReefAngelClass::LightsAuto;

[robertoimai]

Your point is the same exact reason I was never a fan of having the mode to shut lights off and never created it in the past.

[melovictor]

It is your call....either you close the ticket or we work in a possible solution for this. Like I said, if the user selects lights off we could create a visual warning on head unit's display and something else for the webplaform and the other interfaces...something like the WC mode where the user needs to interact otherwise he has no access to other functionalities...I am not sure yet how it hows with the wifi but I'll figure out anytime soon. Victor.