letscontrolit / ESPEasy

Easy MultiSensor device based on ESP8266/ESP32
http://www.espeasy.com
Other
3.3k stars 2.22k forks source link

longpulse does not update GPIO state on device webpage and FHEM server #2730

Closed chunter1 closed 1 year ago

chunter1 commented 5 years ago

I am running two FHEM installations with several espeasy nodes. As long as i set GPIOs with the "gpio" command, the change of the state is immediately reported to the FHEM server. However, if i use "longpulse" to set a GPIO, the state is never updated. Same issue happens on the espeasy device webpage. The state is not changed.

TD-er commented 5 years ago

Can you try, enabling the gpio monitoring at boot in the rules:

On System#Boot Do
 Monitor,GPIO,2   // Change for your GPIO pin
EndOn
chunter1 commented 5 years ago

I just tried it, but it does not change anything. Another maybe interesting observation ist, that while a longpuls is running (e.g. longlupse 14 1 600), sending an additional "gpio 14 1" does not change anything either.

For the GPIO14 i am using, i added:

On System#Boot Do Monitor,GPIO,14 EndOn

chunter1 commented 5 years ago

As in my case GPIO14 is configured as "switch"-input, shouldn't any level change (no matter if initiated by a "gpio" or "longpulse" command) be reported back anyway?

Or maybe is another "tempStatus.forceEvent = 1" needed int he "longpulse" function?

TD-er commented 5 years ago

Does this GPIO have multiple plugins using the same pin?

chunter1 commented 5 years ago

No, it's only used to control a relay.

chunter1 commented 5 years ago

Shouldn't there be a: tempStatus.forceEvent = 1; after: tempStatus.command = 1; in the longpulse section?

giig1967g commented 5 years ago

@chunter1 I have to check. I don't remember

TD-er commented 5 years ago

The longpulse command does schedule a PLUGIN_TIMER_IN call to return the GPIO state.

    case PLUGIN_TIMER_IN:
    {
      digitalWrite(event->Par1, event->Par2);

      // setPinState(PLUGIN_ID_001, event->Par1, PIN_MODE_OUTPUT, event->Par2);
      portStatusStruct tempStatus;

      // WARNING: operator [] creates an entry in the map if key does not exist
      const uint32_t key = createKey(PLUGIN_ID_001, event->Par1);
      tempStatus = globalMapPortStatus[key];

      tempStatus.state = event->Par2;
      tempStatus.mode  = PIN_MODE_OUTPUT;
      savePortStatus(key, tempStatus);
      break;
    }

The start of the longpulse:

          portStatusStruct tempStatus;
          const uint32_t key = createKey(PLUGIN_ID_001, event->Par1);

          // WARNING: operator [] creates an entry in the map if key does not exist
          // So the next command should be part of each command:
          tempStatus = globalMapPortStatus[key];

          const bool pinStateHigh             = event->Par2 != 0;
          const uint16_t pinStateValue        = pinStateHigh ? 1 : 0;
          const uint16_t inversePinStateValue = pinStateHigh ? 0 : 1;
          pinMode(event->Par1, OUTPUT);
          digitalWrite(event->Par1, pinStateValue);

          // setPinState(PLUGIN_ID_001, event->Par1, PIN_MODE_OUTPUT, pinStateValue);
          tempStatus.mode    = PIN_MODE_OUTPUT;
          tempStatus.state   = event->Par2;
          tempStatus.output  = event->Par2;
          tempStatus.command = 1; // set to 1 in order to display the status in the PinStatus page
          savePortStatus(key, tempStatus);
          unsigned long timer = time_in_msec ? event->Par3 : event->Par3 * 1000;

          // Create a future system timer call to set the GPIO pin back to its normal value.
          setPluginTaskTimer(timer, event->TaskIndex, event->Par1, inversePinStateValue);
          log = String(F("SW   : GPIO ")) + String(event->Par1) +
                String(F(" Pulse set for ")) + String(event->Par3) + String(time_in_msec ? F(" msec") : F(" sec"));
          addLog(LOG_LEVEL_INFO, log);
          SendStatusOnlyIfNeeded(event->Source, SEARCH_PIN_STATE, key, log, 0);

The differences are:

          tempStatus.output  = event->Par2;
          tempStatus.command = 1; // set to 1 in order to display the status in the PinStatus page
          SendStatusOnlyIfNeeded(event->Source, SEARCH_PIN_STATE, key, log, 0);

These are done at the start of the longpulse and not at the end.

giig1967g commented 5 years ago

@chunter1 I am reviewing my code. What do you think it should happen exactly? I tested the longpulse,1,1,10 function and the pinstate is correctly updated during the longpulse time in the pinstate page.

TD-er commented 5 years ago

Is the pinstate also updated via controllers, like the normal switch does?

giig1967g commented 5 years ago

I have to check.

DittelHome commented 5 years ago

I have the same problem. monitor does not work for longpulse.

on System#Boot do
      monitor gpio,0
endon
on Clock#Time=All,09:00 do    //Every day 09:00 Zisternenlueftung an
 LongPulse,0,0,900
endon
on GPIO#0=1 do
 publish /Heizraum-ESP/Lueftung,0
endon
on GPIO#0=0 do
 publish /Heizraum-ESP/Lueftung,1
endon 

longpulse does not genertate a event ..

giig1967g commented 5 years ago

I will check tonight.

giig1967g commented 5 years ago

It was tricky, but I found the problems: 1) the LONGPULSE command is not working unless the GPIO is defined in the devices list as an INPUT SWITCH. But this is incorrect, for two reasons: a) I might want to call LONGPULSE on a GPIO/MCP/PCF pin that is not listed in the device list page. b) if I am using the LONGPULSE command it means that the GPIO is an OUTPUT PIN so it should NOT be in the devices list as an INPUT SWITCH (because it's an output pin).

The reason why the command is failing is that it calls setPluginTaskTimer that has one parameter that is event->TaskIndex. But this parameter is valid only if the GPIO is listed (incorrectly) in the device as an INPUT SWITCH. Otherwise it's=12 and the funciont returns without resetting the pin.

2) there was a bug in the code. This line was missing in LONGPULSE command and in PLUGIN_TIMER_IN: (tempStatus.monitor) ? tempStatus.forceMonitor = 1 : tempStatus.forceMonitor = 0;

Solution: for problem 1) I suggest to create a new timer function called ls setPluginTimer that will contain the plugin_id instead of the task_index.

for problem 2) I will fix the code

TD-er commented 5 years ago

I will think about this, since I don't yet understand what you mean/need for problem 1) I think the main reason why I don't understand it yet, is related to the time of day :)

giig1967g commented 5 years ago

It's late for me too... :) I will explain better tomorrow

giig1967g commented 5 years ago

@TD-er: I will try to be more clear...

let's say that you have a unit with all your GPIO set as output pins, controlling relays.

Case A) - not working None of them is declared in the device page (as in the device page you should declare only input switch). Assume that you have previously called monitor,gpio,13 and GPIO,13,1.

When you call the longpress command (i.e.longpress,13,1,0), the command does the following: a) change the state of GPIO13 to 0 b) creates the event GPIO#13=0 (from PLUGIN_MONITOR) c) schedules a timer in the next 10 seconds with the function

setPluginTaskTimer(timer, event->TaskIndex, event->Par1, inversePinStateValue); where timer = 10000ms //the timer time event->TaskIndex=12 //as the GPIO is not in the task list, it returns 12 event->Par1 =13 //the gpio inversePinStateValue=1 //the state to be set when the timer kicks in

But the setPluginTaskTimer function, will not create the timer as it will discard this request because the taskindex is not valid. So as the timer has not been created, the GPIO13 will never return back to state =1.

Case B) - working You have GPIO13 declared in the device list as task = 1 as INPUT SWITCH (But as we know, the SWITCH INPUT should be used only for INPUT pins and not for OUTPUT pins.) The setPluginTaskTimer function, will create the timer as the taskIndex will be a valid number. When you call the longpress command (i.e.longpress,13,1,0), the command does the following: a) change the state of GPIO13 to 0 b) creates the event GPIO#13=0 (from PLUGIN_MONITOR) c) schedules a timer in the next 10 seconds with the function

setPluginTaskTimer(timer, event->TaskIndex, event->Par1, inversePinStateValue); where timer = 10000ms //the timer time event->TaskIndex=1 // the GPIO task list event->Par1 =13 //the gpio inversePinStateValue=1 //the state to be set when the timer kicks in

d) after 10 seconds PLUGIN_TIMER_IN is called and d1) change the state of GPIO13 to 1 d2) creates the event GPIO#13=1

But as we know, the SWITCH INPUT should be used only for INPUT pins and not for OUTPUT pins.

SUGGESTION: create a timer type that takes as input the PLUGIN_ID instead of the task_index in order to execute correctly the commands for output pins that do not necessarily have a task_index.

TD-er commented 5 years ago

OK, even past my first coffee of today, I'm still confused here.

First what is the reason a monitored GPIO pin can only monitor on input pins? Why can't it monitor output pins? (not saying it is wrong, just want to know why)

Second. If you are worried about the wrong taskIndex not able to switch it back, why not refuse to set it for the initial change if the taskIndex is wrong? Are we fixing the right problem by your suggestion?

giig1967g commented 5 years ago

First what is the reason a monitored GPIO pin can only monitor on input pins? Why can't it monitor output pins? (not saying it is wrong, just want to know why)

the monitor command is correct to work on input and output pin. (and is working) Is the longpulse command that should work only on output pin

Second. If you are worried about the wrong taskIndex not able to switch it back, why not refuse to set it for the initial change if the taskIndex is wrong?

That's an alternative (or workaround), but it's counterintuitive. Because we would be allowing the longpulse command only if you declare the output pin in the device list as input switch...

Are we fixing the right problem by your suggestion?

in my suggestion we will maintain the current coherence of the switch plugins: the OUTPUT pins will be allowed to run all the PLUGIN_WRITE commands even if they are not declared in the device list.

TD-er commented 5 years ago

in my suggestion we will maintain the current coherence of the switch plugins: the OUTPUT pins will be allowed to run all the PLUGIN_WRITE commands even if they are not declared in the device list.

I know we will break a lot of installed user applications when we change it, but I'm not sure if this is desired behavior. It does make things a lot more complicated by maintaining this.

That's an alternative (or workaround), but it's counterintuitive. Because we would be allowing the longpulse command only if you declare the output pin in the device list as input switch...

Well it must be counter intuitive, since I still don't really understand why it is needed. Maybe later this evening I can get the idea what's the real problem here. I think we're at the same problem again as before, when we concluded the switch plugin has become a horrible unmaintainable beast.

giig1967g commented 5 years ago

Hi , I suspect I am not able to make myself clear... :( Will retry!

Well it must be counter intuitive, since I still don't really understand why it is needed.

what you don't understand if it is needed?

TD-er commented 5 years ago

I do agree it is counter-intuitive if there must be an input switch plugin in order to make it work. What I don't understand is why it is needed to have an input switch plugin defined for that GPIO pin.

giig1967g commented 5 years ago

My suspect was right. I haven't been able to be clear enough. For me it's easier because I have really looked deeply into the code of that plugin :)

So, with the current implementation of setPluginTaskTimer and the PLUGIN_TIMER_IN, the longpulse doesn't work if the GPIO is not defined in the device list. Or, in other words, the longpulse command works only if the GPIO is defined in the devices list. I don't know why it's like that, because I haven't coded the timer parts (I think you did it). But the effect is that today you need the GPIO in device list if you want the longpulse to work. The reason is that the setPluginTaskTimer works only if you provide the task ID and the task_id is valid only for GPIO that are in the device list.

I hope I could made some light on the matter, but if not I am ready to explain it again.

giig1967g commented 5 years ago

fixed with #2738

giig1967g commented 5 years ago

@chunter1 @DittelHome please try test version and tell me if it works with FHEM (I can't test myself): 2738_longpulse_normal_ESP8266_4M1M.zip

chunter1 commented 5 years ago

From my side... I don't see any change. If you reload the "Devices" webpage tab during an ongoing longpulse, the status of the GPIO is still not changing.

giig1967g commented 5 years ago

You will see the change in the pinstate page. Please check. But did you add the monitor command?

On System#Boot Do
 Monitor,GPIO,2   // Change for your GPIO pin
EndOn
DittelHome commented 5 years ago

Good morning together, it is working for me:

74152: SW : GPIO 0 Pulse set for 90 sec 74254: EVENT: GPIO#0=0 74322: ACT : publish /Heizraum-ESP/Lueftung,1 74336: Command: publish 164260: EVENT: GPIO#0=1 164322: ACT : publish /Heizraum-ESP/Lueftung,0 164336: Command: publish

Thats my rule:

on System#Boot do
      monitor gpio,0
endon
on Clock#Time=All,09:00 do    //Every day 09:00 Zisternenlueftung an
 LongPulse,0,0,900
endon
on GPIO#0=1 do
 publish /Heizraum-ESP/Lueftung,0
endon
on GPIO#0=0 do
 publish /Heizraum-ESP/Lueftung,1
endon 
chunter1 commented 5 years ago

Tried it by adding "monitor" and further by deleting the "switch input" in the device list. However, it still doesn't send any event to the FHEM server. Verified if it is generating an event using the "gpio" command and this works as usual.

DittelHome commented 5 years ago

Tried it by adding "monitor" and further by deleting the "switch input" in the device list. However, it still doesn't send any event to the FHEM server. Verified if it is generating an event using the "gpio" command and this works as usual.

I use as connection to fhem pidome-mqtt, you i think fhem-http. On my side a i have had the problem, that the longpulse command doesn't made a event in the past. My devicelist has no switch.

giig1967g commented 5 years ago

@chunter1 Stupid question: did you rebbot after adding the monitor command?

Please your rules and log file

chunter1 commented 5 years ago

I use as connection to fhem pidome-mqtt, you i think fhem-http.

Right, i am using FHEM HTTP since i am not a fan of MQTT ;)

DittelHome commented 5 years ago

I have done some more testings.... The pinstates website is also reflecting the correct values. It makes no difference if i use gpio or longpulse now. For me all is working fine now.

chunter1 commented 5 years ago

@chunter1 Stupid question: did you rebbot after adding the monitor command? Please your rules and log file

Yes, i did reboot the unit.

Here the entry in my rules:

On System#Boot Do monitor GPIO,14 endon

And here the Log when a "longpulse 14 1 600" command is received:

152005: WD : Uptime 3 ConnectFailures 0 FreeMem 19760 WiFiStatus 3 181900: HTTP: longpulse,14,1,600 181907: SW : GPIO 14 Pulse set for 600 sec 181927: EVENT: GPIO#14=1 182021: WD : Uptime 3 ConnectFailures 0 FreeMem 19448 WiFiStatus 3

And here the Log when a "gpio 14 1" command is received:

350261: HTTP: gpio,14,1 350267: SW : GPIO 14 Set to 1 350312: SW : GPIO=14 State=1 Output value=1 350314: EVENT: RELAIS#RELAIS=1.00 350431: EVENT: GPIO#14=1 360317: SW : State 1.00

DittelHome commented 5 years ago

I can't see any bad messages. Your ESP generate's a EVENT as it should. That was missing in the past. I cant see your problem. By the way, the monitor command does only help, if you use it. For example: on GPIO#14=1 do any command // on my side i publish state to fhem endon

Hope this help ...

chunter1 commented 5 years ago

I guess FHEM is missing the event "350314: EVENT: RELAIS#RELAIS=1.00" in the case of a longpulse? The reading in FHEM that normally receives/holds the state is called "RELAIS" and since there is no such event, it will not update. (i guess)

Same applies to a "pulse" command. The state keeps unchanged in the FHEM server Here is the Log of the "puls" command:

1910492: HTTP: pulse,14,1,2000 1912498: SW : GPIO 14 Pulsed for 2000 mS 1912518: SW : GPIO=14 State=0 Output value=0 1912519: EVENT: RELAIS#RELAIS=0.00 1920320: SW : State 1.00

giig1967g commented 5 years ago

you should get two events, one when longpulse starts and one when it ends:

181900: HTTP: longpulse,14,1,600
181907: SW : GPIO 14 Pulse set for 600 sec
181927: EVENT: GPIO#14=1
...
181927: EVENT: GPIO#14=0

@chunter1 I know it may sound absurd, but when I was debugging the issue, I discovered that my unit was not responding correctly on GPIO 14. Can you try to change the GPIO number?

Run these rules:

monitor,gpio,13
longpulse,13,1,10

and report back your log please.

chunter1 commented 5 years ago

Here is the Log for a longpulse on newly defined GPIO4:

147240: HTTP: longpulse,4,1,5 147247: SW : GPIO 4 Pulse set for 5 sec 147268: EVENT: GPIO#4=1 152352: EVENT: GPIO#4=0

Here is the Log for the same longpulse but on GPIO14:

124192: HTTP: longpulse,14,1,5 124198: SW : GPIO 14 Pulse set for 5 sec 124247: EVENT: GPIO#14=1 129247: EVENT: GPIO#14=0

Both are defined to be monitored in the rules.

chunter1 commented 5 years ago

Another test with monitoring removed. I set and reset GPIO14="RELAIS" and GPIO4="RELAIS2" with the "gpio" command:

425504: HTTP: gpio,14,1 425507: SW : GPIO 14 Set to 1 432153: HTTP: gpio,14,0 432156: SW : GPIO 14 Set to 0 432166: SW : GPIO=14 State=0 Output value=0 432174: EVENT: RELAIS#RELAIS=0.00 436540: HTTP: gpio,4,1 436543: SW : GPIO 4 Set to 1 436561: SW : GPIO=4 State=1 Output value=1 436570: EVENT: RELAIS2#RELAIS2=1.00 442421: HTTP: gpio,4,0 442424: SW : GPIO 4 Set to 0 442461: SW : GPIO=4 State=0 Output value=0 442462: EVENT: RELAIS2#RELAIS2=0.00

Why ist the EVENT "EVENT: RELAIS#RELAIS=1.00" not generated?

Update:

I repeated the test and got a correct result now:

592003: HTTP: gpio,14,1 592005: SW : GPIO 14 Set to 1 592061: SW : GPIO=14 State=1 Output value=1 592066: EVENT: RELAIS#RELAIS=1.00 598261: HTTP: gpio,14,0 598264: SW : GPIO 14 Set to 0 598274: SW : GPIO=14 State=0 Output value=0 598274: EVENT: RELAIS#RELAIS=0.00 600955: SW : State 1.00 600956: EVENT: PIR#PIR=1.00 602518: WD : Uptime 10 ConnectFailures 0 FreeMem 17088 WiFiStatus 3 604939: HTTP: gpio,4,1 604942: SW : GPIO 4 Set to 1 604961: SW : GPIO=4 State=1 Output value=1 604970: EVENT: RELAIS2#RELAIS2=1.00 610316: HTTP: gpio,4,0 610319: SW : GPIO 4 Set to 0 610361: SW : GPIO=4 State=0 Output value=0 610362: EVENT: RELAIS2#RELAIS2=0.00

chunter1 commented 5 years ago

Another test with minitoring turned on gives me different results when i repeat the following command sequence:

  1. "gpio 14 1"
  2. "gpio 14 0"
  3. "longpulse 14 1 5"

FIRST RESULT:

30140: SW : GPIO 14 Set to 1 30232: SW : GPIO=14 State=1 Output value=1 30232: EVENT: RELAIS#RELAIS=1.00 30324: EVENT: GPIO#14=1 32453: WD : Uptime 1 ConnectFailures 0 FreeMem 18824 WiFiStatus 3 35389: HTTP: gpio,14,0 35392: SW : GPIO 14 Set to 0 35431: SW : GPIO=14 State=0 Output value=0 35432: EVENT: RELAIS#RELAIS=0.00 35519: EVENT: GPIO#14=0 39275: HTTP: longpulse,14,1,5 39279: SW : GPIO 14 Pulse set for 5 sec 43925: HTTP: gpio,14,0 43928: SW : GPIO 14 Set to 0 43937: SW : GPIO=14 State=0 Output value=0 43938: EVENT: RELAIS#RELAIS=0.00 44023: EVENT: GPIO#14=0

SECOND RESULT:

175558: HTTP: gpio,14,1 175561: SW : GPIO 14 Set to 1 175631: SW : GPIO=14 State=1 Output value=1 175632: EVENT: RELAIS#RELAIS=1.00 175717: EVENT: GPIO#14=1 178573: HTTP: gpio,14,0 178575: SW : GPIO 14 Set to 0 178631: SW : GPIO=14 State=0 Output value=0 178632: EVENT: RELAIS#RELAIS=0.00 178719: EVENT: GPIO#14=0 182453: WD : Uptime 3 ConnectFailures 0 FreeMem 18680 WiFiStatus 3 186104: HTTP: longpulse,14,1,5 186107: SW : GPIO 14 Pulse set for 5 sec 212453: WD : Uptime 4 ConnectFailures 0 FreeMem 18704 WiFiStatus 3

Besides both results are missing the longpulse EVENT "EVENT: RELAIS#RELAIS=1.00", the second result is additionally missing the EVENT "EVENT: RELAIS#RELAIS=0.00".

giig1967g commented 5 years ago

at present, the commands that operate on OUTPUT pins (like longpulse) will only send the EVENT if you decide to monitor it using the monitor command. And the event will be GPIO#14=0.

The main problem is that you are using an output pin configured as an input pin (using input switch plugin). I know that there is no output plugin, but that is the situation.

The fact that GPIO command sends also the tast#value event is probably either a bug or a legacy compatibility. And it might be cancelled in the future. So better not use it.

TD-er commented 5 years ago

The fact that GPIO command sends also the tast#value event is probably either a bug or a legacy compatibility. And it might be cancelled in the future. So better not use it.

Hmm that's something we really need to look into. I guess if we split the GPIO-stuff into separate plugins, this will also solve itself. Then it can be a single checkbox in the task setting whether the pin should also be monitored to send out an event.

tonhuisman commented 1 year ago

The refactoring of the GPIO code is in place for some time already, so this issue seems no longer relevant and can be closed.