stefanbode / Sonoff-Tasmota

Provide ESP8266 based itead Sonoff with Web, MQTT and OTA firmware using Arduino IDE, enhanced with I2C options
GNU General Public License v3.0
126 stars 40 forks source link

Selective Interlock #43

Closed perozonp closed 6 years ago

perozonp commented 6 years ago

I'm building a sprinkler controller with a 16 relays board using two PCF8574. Only 12 of the relays go to the sprinkler valves (yes! I have 12 zones) and the other 4 in the board + the single relay in the Sonoff basic are for different purpose. I need those 12 relays (Relay 2 to Relay 13) to have interlock among them, while the other 5 relays should be operated independently. I've been trying to setup rules but since there are 12 relays the rules result too large for the limit in the number of characters (511) and with only 3 sets of rules I can only fit 9 of the 12 relays there. The original software only has "SetOption 14" which affects all the relays.

Since this fork has support for PCF8574 then probably this feature request is better here: Is it possible to somehow modify the "SetOption 14" to allow selective interlocking? It would be great if I could indicate which group of relays will work this way.

Thanks... Nestor Perozo.

stefanbode commented 6 years ago

Hi Nestor, this was my concern, that someone needs a mixed environment. Maybe with an additional power_t variable, you could flag which relays belong to the interlock and wich none. in your case, the variable in binary should look like 1111111111110000. Should not that complicated to add.

perozonp commented 6 years ago

Thanks, will take a look at that option.

stefanbode commented 6 years ago

You can give it a try. minimal overhead. the new command is INTERLOCKMASK and the format is like described. Therefore to include the first 12 relays to interlock set it to 1111111111110000

Enjoy.

perozonp commented 6 years ago

Thanks Stephan. I'm a bit confused. What is "Paired Interlock"? I would have done this by replacing line 1284 by this:

if ((power & imask) && (mask != imask) && (imask & Settings.interlock_mask)) ExecuteCommandPower(i +1, POWER_OFF, SRC_IGNORE);

Anyway, I'll test it this afternoon and let you know.

perozonp commented 6 years ago

By the way, please do not miss-understand me, I came to this conclusion after spending a couple of hours yesterday trying your suggestion the wrong way (I was messing with the SetDevicePower routine). It worked but the actual states were not reported back to HomeAssistant. Also, this is the first time I do some C++ code (I just have visual basic experience), so I'm learning here from the experts.

Thanks for this wonderful piece of code, it has saved headaches everywhere!

stefanbode commented 6 years ago

Tasmota has an interlock functionality that switches OFF all other relays if you switch one ON. Therefore only ONE relay is on. This is independent of the number of relays defined. Many of my users have valves and blinds. To operate these with the reverse direction you need two relays each. But also here both mustn't be ON. This is paired interlock. the even and the next odd relays get coupled. The MASK is now to remove some of the relays from this restricting behavior.

I will take a look at your code improvement. Always welcome, if it works the same way.

stefanbode commented 6 years ago

Regarding line 1284: Nope you cannot make it that easy because this is inside a loop over all relays this would cause the odd relays always the master and the even relays follow. With the current implementation, the last switched relay is the master and the corresponding one the slave. You have to take care that you do not mess up with the other relays.

perozonp commented 6 years ago

Stephan: now that I understand what is that Paired Interlock mode (SetOption31) I see the INTERLOCKMASK as implemented only works with Paired Interlock and not with the original tasmota interlock (SetOption14). My use case is for sprinklers, where only 1 zone can be active at a given time, so I need to include in the mask which relays are dedicated to the zones and have the interlock mode working among all of them (not in pairs).

I solved this by modifying line 1284 this way:

if ((power & imask) && (mask != imask) && (imask & Settings.interlock_mask) && (mask & Settings.interlock_mask)) ExecuteCommandPower(i +1, POWER_OFF, SRC_IGNORE);

yes, had to check both mask and imask with the interlock_mask to have it properly working.

in addition, had to comment the following lines inside the procedure SetDevicePower:

//  if (Settings.flag.interlock && !Settings.flag.paired_interlock) {
//      power_t mask = 1;
//      uint8_t count = 0;
//      for (byte i = 0; i < devices_present; i++) {
//        if (rpower & mask) count++;
//        mask <<= 1;
//      }
//      if (count > 1) {
//        power = 0;
//        rpower = 0;
//      }
//  }

Not sure why the interlock routine is being handled also in this procedure (where the relays status are not properly reported back using MQTT). I see this procedure is also called from the initialization routines, so probably it is a safe check during startup. If this is the case then this routine should also be enhanced to include the interlock_mask (something similar to the line 1284).

One additional thing: while the PCF8574 are connected the web console doesn't respond to commands (it still shows log info). In order to set the SetOption14 to On and SetOption31 to Off I had to start my sonoff with the two PCF8574 disconnected (to have the console properly respond to commands) and once the setoptions were set then power off, connect the PCFs and the power on again. Can you reproduce this behavior or is it just me probably running out of memory? how can I tell?

Thanks for your assistance on this, now I can move forward to work in the main sprinklers routine in Home Assistant.

Regards... Nestor Perozo.

stefanbode commented 6 years ago

Ahh, therefore you don’t have valves on your sprinkler system , but magnetic opener. Therefore as long as the relay is on the water flows. Let me do some code changes and do the mask also on the normal interlock. This should fulfill your use case.

Why you say the relay status is not reported back on mqtt. This works for me. At least with the paired interlock.

The reason for the change here is that only in this procedure you get the information which relay was changed and to wich status is was changed. Later on you only have the power bin array with the final matrix what to set. Making corrections here is impossible because there is no information about the stage before. As mentioned before. I do not see any problems with web and mqtt regarding the correct state afterwards.

perozonp commented 6 years ago

Ahh, therefore you don’t have valves on your sprinkler system , but magnetic opener. Therefore as long as the relay is on the water flows. Correct, that is my case. I thought that was "everyone else's case" (magnetic opener for sprinkler valves). Are there different valves for sprinkler systems?

Why you say the relay status is not reported back on mqtt. This works for me. Apologies for not explaining this correctly. Before you did the code changes I was trying to do it on my own and the result was that, the interlock worked but the status was not reported back to Home Assistant, so in my phone I saw the relay ON when it was actually OFF. Certainly some poor code may have lead to this behavior. Currently it is working fine (your code + my update to make the INTERLOCKMASK work with the original SetOption14).

Have you experienced any issues with the Web Console (not processing commands) while the PCF8574 are plugged in? I had to turn off the serial log in order to use those two pins to communicate with the PCF8574 (probably has nothing to do with this).

One more thing: since I'm using a SONOFF basic, which has 1 relay, then the relay numbering starts with 1 for the SONOFF relay and from 2 to 17 for the 16 relays in the board controlled by the two PCF8574. I want to use only the first 12 relays in the 16 relays board and leave out of the interlock the SONOFF relay and the last 4 in the board. Then the correct mask for my case is 0x1FFE (tested and works fine!).

stefanbode commented 6 years ago

Ok going back to the interlock mask. Your first code was correct. You only have to add the comparison to the imask, because this is the one that loops through the relays.

        for (byte i = 0; i < devices_present; i++) {
          power_t imask = 1 << i;
          //stb mod
          if ((power & imask) && (mask != imask) && (imask & Settings.interlock_mask)) ExecuteCommandPower(i +1, POWER_OFF, SRC_IGNORE);
          //end
        }

The major error is commenting out the complete procedure in SetDevicePower. With this modification, you break the old behavior. Your problem is, that currently, the procedure does not know something about the interlock mask and where it has to leave the fingers from.

The purpose of this procedure is to count the ON relays and if it is >1 then power off everything. Make sense, if it only counts the relay that is IN the mask and afterward doesn't set ALL relays to 0 but only on the relays that are in the mask.

I would assume it must look this way to ensure also the old functionality is working:

  //stb mod
  if (Settings.flag.interlock && !Settings.flag3.paired_interlock) {
      power_t mask = 1;
      uint8_t count = 0;
      for (byte i = 0; i < devices_present; i++) {
        // only count ON relays that fit to the MASK
        if ((rpower & mask) && (mask & Settings.interlock_mask)) count++;
        mask <<= 1;
      }
      if (count > 1) {
        power &= !Settings.interlock_mask;
        rpower = power;
      }
  }
  //end

Anyhow I need to do some testing and have no devices with me right now to test. Will do this at the weekend

perozonp commented 6 years ago

Yes, your code for the SetDevicePower section looks correct, I bet it will work. Anyway if you have time for it, try commenting out that whole section and evaluate the effect. I currently have it commented and the masked interlock works fine. Unless (as mentioned earlier) it is used as a safety measure during initialization.

Regards...

stefanbode commented 6 years ago

as always there was a small IF missing. Now everything behaves like it should do. I have updated the sour code. Have fun

perozonp commented 6 years ago

Just tested, works as expected. Thanks!!!