sinricpro / esp8266-esp32-sdk

Library for https://sinric.pro - simple way to connect your device to Alexa, Google Home, SmartThings and cloud
https://sinric.pro
Other
230 stars 124 forks source link

Update problem when send more request in the same time for different data #10

Closed conuxconux closed 4 years ago

conuxconux commented 4 years ago

Hi.

I think so that will be a problem with your last update that data update with different delay for the same request.

I have centralized the request in a loop routine:

  if ( updateState )
  {
      SinricProThermostat& mySensor = SinricPro[SENSOR_ID];
      mySensor.sendTemperatureEvent(temperature, humidity); 
      mySensor.sendTargetTemperatureEvent(humidity);
      mySensor.sendPowerStateEvent(relState);
      if ( isAutoMode )
      {
        mySensor.sendThermostatModeEvent("AUTO"); 
      }
      else
      {
        if (relState == HIGH)
        {
          mySensor.sendThermostatModeEvent("HEAT");       
        }
        else
        {
          mySensor.sendThermostatModeEvent("COOL"); 
        }         
      }
      updateState = false;    
  }

In this situation some features update in different time.

sivar2311 commented 4 years ago

each event is using its own "timer" for message limiting. so there should no problem sending different events at the same time (they wont be blocked) limitation settings right now are: per event type 10 messages can be send with a distance of 1 second, after that you have to wait a minute until next event of the same type can be sent In the code above, you're using "currentTemperature", "targetTemperature", "powerState" and "ThermostatMode" events...each have its own limitation like described before

conuxconux commented 4 years ago

after that you have to wait a minute until next event of the same type can be sent

I tell you a sample.

I power on the device, after little time power off. I don't see any changes before a minute.

I power on the device in AUTO mode, after little time change it in HEAT mode I don't see any change before a minute.

Is it right?

kakopappa commented 4 years ago

@conuxconux Hi there,

Can you improve your logic to send the status if it has been changed? for example, I can see that it is sending power status/thermostat mode regardless of whether it has been changed or not.

Thanks in advnce

sivar2311 commented 4 years ago

This will not a problem unless you keep the minimum distance about 1 second per event and not sending more than 10 events in a short period of time.

For each event-type you have 10 events "free" which can be sent with a minimum distance of 1 second. After sending one of those 10 events, it have to wait a minute to be releasesd and can "used" again.

sivar2311 commented 4 years ago

As far as i can see if any one of your states changes, you sending 4 events. You should only send an event when the state related to this is event has changed. Example: It seems, your code send onPowerState event even when temperature or humidity changed. What do you want to achieve with this?

conuxconux commented 4 years ago

Example: It seems, your code send onPowerState event even when temperature or humidity changed.

Yes it is. I understand what you mean @sivar2311 and @kakopappa. But if I modify my code and send only the required event to update, if I send power on event and after little time because I change idea I update power event with power off (that is the same event), I will see the update after 1 minute?

If yes I think that the 1 minute delay should be applied only for the event that arrive from sensor to avoid that a no good implementation (my first one for example) can sending much data to the server: for example temperature and humidity should be delayed. But for power state event or thermostat mode or target temperature it did not should be applied they should be free to update without delay.

This what I think. But is not a problem for me. If I know how it works I will see the data updated after 1 minute.

Many thanks

sivar2311 commented 4 years ago

Example: It seems, your code send onPowerState event even when temperature or humidity changed.

Yes it is. I understand what you mean @sivar2311 and @kakopappa. But if I modify my code and send only the required event to update, if I send power on event and after little time because I change idea I update power event with power off (that is the same event), I will see the update after 1 minute?

No, because every EventType has its own limitations You can send 10 powerStates events (with a minimum distance of 1 second) until powerState event will limited to 1 event per minute.

It is like this (each "|" representing an event): |-|-|-|-|-|-|-|-|-|--------------|--------------|----------------|---------------|

conuxconux commented 4 years ago

I’m sorry then I didn't understand. Then it could be a delay in the Alexa app.

Thank you

sivar2311 commented 4 years ago

If you're using Alexa App to control your device, no event is involved! Alexa App sending a request to your device, your device will answer this request with a response. This is what the callbacks are used for.

Events are used, when state on device itself has changed.

Assume youre device having a power on/off button. If this button is pressed, your code can / should send a powerStateEvent to sinric server to update the device state on alexa side. So you will see the new state in Alexa App

conuxconux commented 4 years ago

Yes, this is clear to me.

When for example I ask Alexa to turn on the device both by voice or by clicking on the button in the App, the event arrives in onPowerState. In this I perform some actions including for example the change of the thermostat mode from "COOL" to "AUTO". When I do it from a physical button on the device, i check it in the loop cycle it is clear. In this case I send events powerState and the mode. I setting it in this case to "HEAT" to understand that it is in manual mode.

Only I see this mode or power state does not always update immediately in the Alexa App.

sivar2311 commented 4 years ago

So you're sending events while inside a callback? Don't do this! You have to find another way for this.

As i said before: Events are only for state changes by the device itself.

conuxconux commented 4 years ago

No I don't. I set a flag "updateState" to "true" and then in the loop cycle I send events. I have just changed the previous code so only requested events are sent. Now I'll see if it works.

  if ( updateState )
  {
      SinricProThermostat& mySensor = SinricPro[SENSOR_ID];

      if ( dataToUpdate.sensorData )
      {
        mySensor.sendTemperatureEvent(temperature, humidity); 
        mySensor.sendTargetTemperatureEvent(humidity);       
      }
      if ( dataToUpdate.powerState )
      {
        mySensor.sendPowerStateEvent(relState);
      }

      if ( dataToUpdate.isAutoMode )
      {
        if ( isAutoMode )
        {
          mySensor.sendThermostatModeEvent("AUTO"); 
        }
        else
        {
          if (relState == HIGH)
          {
            mySensor.sendThermostatModeEvent("HEAT");       
          }
          else
          {
            mySensor.sendThermostatModeEvent("COOL"); 
          }         
        }        
      }

      dataToUpdate.sensorData = false;
      dataToUpdate.powerState = false;
      dataToUpdate.isAutoMode = false;

      updateState = false;    
  }
sivar2311 commented 4 years ago

I set a flag "updateState" to "true" and then in the loop cycle I send events. I have just changed the previous code so only requested events are sent. Now I'll see if it works.

This is good!

But I don't get the behavior you want to achieve with this. May you explain how your device is working?

conuxconux commented 4 years ago

When I turn it on from the Alexa app or via voice it goes into "AUTO" mode, when I press the physical button in "HEAT" mode, when "COOL" is turned off. In the first case it detects the humidity and temperature data, calculates the dew point and remains on until it is reached. If I activate it manually, it stays on until I turn it off manually. Using the "AUTO" or "HEAT" mode I understand whether it goes to Automatic or Manual from the Alexa app or asking for the set temperature.

sivar2311 commented 4 years ago

Sorry, i don't get it. Maybe i got lost in translation.

I have just changed the previous code so only requested events are sent. Now I'll see if it works.

Is your new code working right now?

conuxconux commented 4 years ago

No it does't works. I have worked to know what is the problem but it is very strange. It seems that some message are losted in random order.

I attach the loop and you can test it only press the button. Can you remove the piece of the code that call other function, I left them for completeness, maybe you understand what is the problem.

I have tested with delay within the message or without they but it is alway random. I works for one or two excecution (one execution is a change of state from off to on and from on to off).

Have you any suggestion?

void loop() {
  ArduinoOTA.handle();
  server.handleClient();

  delay(50);

  SinricPro.handle();

  updateData();

  buttonState = digitalRead(BUTTON);

  if (buttonState == LOW)
  {
    relState = !relState;
    digitalWrite(RELE, relState);
    writeEpromData(); 
    dataToUpdate.powerState = true; 
    dataToUpdate.isAutoMode = true;
    skipMessage = true;
    updateState = true;        
    delay(1000);
  }

  if ( updateState )
  {
      SinricProThermostat& mySensor = SinricPro[SENSOR_ID];

      if ( dataToUpdate.sensorData )
      {
        //delay(DELAY_TO_SEND_MSG);        
        mySensor.sendTemperatureEvent(temperature, humidity); 
        //delay(DELAY_TO_SEND_MSG);
        mySensor.sendTargetTemperatureEvent(humidity);      
      }
      if ( dataToUpdate.powerState )
      {
        //delay(DELAY_TO_SEND_MSG);
        mySensor.sendPowerStateEvent(relState == HIGH ? true : false);
      }

      if ( dataToUpdate.isAutoMode )
      {
        if ( isAutoMode )
        {
          //delay(DELAY_TO_SEND_MSG);
          mySensor.sendThermostatModeEvent("AUTO");  
          if ( relState == HIGH &&  !skipMessage )
          {            
            sendDataIfttt(String(temperature, FLOATROUNDING),String(humidity, FLOATROUNDING), String(getCurDp(), FLOATROUNDING) + "("+ String(dpTarget, FLOATROUNDING)+ ")", eventMsg[START]);  
          } 
        }
        else
        {
          if (relState == HIGH)
          {
            //delay(DELAY_TO_SEND_MSG);
            mySensor.sendThermostatModeEvent("HEAT");       
          }
          else
          {
            //delay(DELAY_TO_SEND_MSG);
            mySensor.sendThermostatModeEvent("COOL"); 
            if ( !skipMessage )
            {
              sendDataIfttt(String(temperature, FLOATROUNDING),String(humidity, FLOATROUNDING), String(getCurDp(), FLOATROUNDING) + "("+ String(dpTarget, FLOATROUNDING) + ")", eventMsg[STOP]);
            }            
          }       
        }                        
      }

      dataToUpdate.sensorData = false;
      dataToUpdate.powerState = false;
      dataToUpdate.isAutoMode = false;

      updateState = false;    
  }

  if (skipMessage)
  {
    skipMessage = false;
  }

}
sivar2311 commented 4 years ago

Okay, first things first. buttonState == low If you hold the button pressed this will called few hundred times per second (forget about your delay at the end, this have to be removed - see next section) You should implement another check "when was the button pressed last time?". Please see Blink without delay Tutorial.

Remove the delay in that section. It is blocking the code so that SinricPro.handle() is not called within the delay time. No request will come in, no response and no event will be sent during this period of time!

Last but not least: You have to check if your events have been sent or not (because they might be blocked). Only change device state when event was sent successfully. Every sendEvent() function returns a bool (true if event was sent, falseif event was not send). Events will not be sent when time between last event is lower than 1 second.

I think the mechanism which is used for event blocking got cleared in this post

conuxconux commented 4 years ago

Many thanks for suggestion.

Last but not least: You have to check if your events have been sent or not (because they might be blocked). Only change device state when event was sent successfully. Every sendEvent() function returns a bool (true if event was sent, false if event was not send). Events will not be sent when time between last event is lower than 1 second.

    // event
    void sendTemperatureEvent(float temperature, float humidity = -1, String cause = "PERIODIC_POLL");
    void sendTargetTemperatureEvent(float temperature, String cause = "PHYSICAL_INTERACTION");
    void sendThermostatModeEvent(String thermostatMode, String cause = "PHYSICAL_INTERACTION");

Send event not return a value. Maybe I didn't understand something.

sivar2311 commented 4 years ago

You're right... readme.md is wrong / inconsistent about this. Thanks for pointing me to that (i will change readme with upcomming 2.2.5)

But: every event function returns a bool like described.

Edit: I've updated readme for upcomming 2.2.5: Readme 2.2.5

conuxconux commented 4 years ago

Some *.h files are wrong.

For example: "SinricProThermostat.h", "SinricProWindowAC.h"... return void.

"SinricProTemperaturesensor.h" is right.

I will edit "SinricProThermostat.h" before your update.

sivar2311 commented 4 years ago

Thanks again for "debugging" :)

This will be included in 2.2.5, too See:
SinricProThermostat.h in 2.2.5 SinricProWindowAC.h in 2.2.5

conuxconux commented 4 years ago

I had already changed them that way. Thanks so much. ;-)

conuxconux commented 4 years ago

I have updated the code but the result is the same. I test with 1000-2000 value for DELAY_TO_SEND_MSG. I have added a blink function (on/off led) to see when i press the button how much times is executed.

Are you shure that SendEvent waiting?

void loop() {
  ArduinoOTA.handle();
  server.handleClient();

  SinricPro.handle();

  now = millis();

  buttonState = digitalRead(BUTTON);
  if ((now - lastPushButton) >= 1000 && buttonState == LOW)
  {
    blinkSignal(2, 200); 
    digitalWrite(LED, LOW);
    lastPushButton = now;
    buttonState = !buttonState;
    relState = !relState;
    digitalWrite(RELE, relState);
    writeEpromData(); 
    dataToUpdate.powerState = true; 
    dataToUpdate.isAutoMode = true;
    skipMessage = true;
    updateState = true;
  }

  updateData();

  if ( updateState )
  {
      SinricProThermostat& mySensor = SinricPro[SENSOR_ID];

      if ( dataToUpdate.sensorData )
      {     
        while(!mySensor.sendTemperatureEvent(temperature, humidity))
        {
          delay(DELAY_TO_SEND_MSG);
        }

         while(!mySensor.sendTargetTemperatureEvent(humidity))
         {
            delay(DELAY_TO_SEND_MSG);    
         }
      }
      if ( dataToUpdate.powerState )
      {
        while(!mySensor.sendPowerStateEvent(relState == HIGH ? true : false))
        {
            delay(DELAY_TO_SEND_MSG);    
        }
      }

      if ( dataToUpdate.isAutoMode )
      {
        if ( isAutoMode )
        {
          while(!mySensor.sendThermostatModeEvent("AUTO"))
          {
              delay(DELAY_TO_SEND_MSG);    
          }            
          if ( relState == HIGH &&  !skipMessage )
          {            
            sendDataIfttt(String(temperature, FLOATROUNDING),String(humidity, FLOATROUNDING), String(getCurDp(), FLOATROUNDING) + "("+ String(dpTarget, FLOATROUNDING)+ ")", eventMsg[START]);  
          } 
        }
        else
        {
          if (relState == HIGH)
          {
            while(!mySensor.sendThermostatModeEvent("HEAT"))
            {
                delay(DELAY_TO_SEND_MSG);        
            }
          }
          else
          {
            while(!mySensor.sendThermostatModeEvent("COOL"))
            {
                delay(DELAY_TO_SEND_MSG);        
            }

            if ( !skipMessage )
            {
              sendDataIfttt(String(temperature, FLOATROUNDING),String(humidity, FLOATROUNDING), String(getCurDp(), FLOATROUNDING) + "("+ String(dpTarget, FLOATROUNDING) + ")", eventMsg[STOP]);
            }            
          }       
        }                        
      }

      dataToUpdate.sensorData = false;
      dataToUpdate.powerState = false;
      dataToUpdate.isAutoMode = false;

      updateState = false;    
  }

  if (skipMessage)
  {
    skipMessage = false;
  }

}
sivar2311 commented 4 years ago

Are you shure that SendEvent waiting?

It is not wating..it is returning true if event was sent or false if not Your code above is blocking code again!

Please tell me again what kind of device you're building and what behavior you want to achieve.

conuxconux commented 4 years ago

I realized that I had to try sending it again ... and I waited 1-2 seconds to avoid overloading. If it don’t send the message can I retry without delay? The device is a Sonoff TH10, and I will use to change the humidity of bathroom. In the previous comment I have detailed how it will works.

When I turn it on from the Alexa app or via voice it goes into "AUTO" mode, when I press the physical button in "HEAT" mode, when "COOL" is turned off. In the first case it detects the humidity and temperature data, calculates the dew point and remains on until it is reached. If I activate it manually, it stays on until I turn it off manually. Using the "AUTO" or "HEAT" mode I understand whether it goes to Automatic or Manual from the Alexa app or asking for the set temperature.

sivar2311 commented 4 years ago

Okay, so you have a device which has humidity and temperatrure sensor on board

I didn't understand your posting about what you want to achieve, that's why i asked again. May you explain this in simple words?

What i guess you want to acheive: if humidity / temperature is above some level, turn on a relay if humidity / temperature is below some level, turn off a relay Is this correct?

conuxconux commented 4 years ago

Yes. It is right

sivar2311 commented 4 years ago

So i dont understand why you need to send so much events!?

Device will run for minutes or hours before state changes, right?

conuxconux commented 4 years ago

I have detailed the description to clearly. I’m sorry for my bad English.

You can start it in automatic mode... and in this case I have to set values of current temperature, humidity and mode auto. If I start it in manual mode with push button I should be change powerstate and mode.. For both every 30 sec if changed or themperature or humidity I will update the data...

Device will run for minutes or hours before state changes, right?

But if change idea I will see wrong data.

sivar2311 commented 4 years ago

This is how i would do: Device is in AUTO mode

Device is in COOL mode

Device is set to COOL mode

Device is in HEAT mode

Device is set to HEAT mode

Edit: use the onboard button to toggle through modes AUTO, HEAT, COOL

conuxconux commented 4 years ago

Maybe I explained myself wrong. When I start it by pressing the device button I send the change mode events to set "HEAT" and the powerState change. Then every 30 seconds if the temperature or humidity changes, I send the new events to update only this data. Don't think that I always send unnecessary events. In the first version it was like that but after I modified it. However I thank you I will try to understand where it does not work. I'm sorry I didn't want to annoy you. ;-)

sivar2311 commented 4 years ago

You dont annoy me... but i was not able to understand what you want to achieve and how your device is working.

By sending humidity / temperature every 30 seconds you will run into limit after sending 10 (temperature) events! Why do you need to report humidity / temperature in such short distances to alexa?

Edit: You can check humidity / temperature and do your calculations everytime loop() gets called. Assume you're in AUTO mode

conuxconux commented 4 years ago

I don't like to see outdated data, but if I can't, I'll adapt.

I think that the problem isn't the frequency that I use to send temperature and humidity. At most I can send 4 events per minute related to temperature and humidity: 2 x (sendTemperatureEvent, sendTargetTemperatureEvent). I will try to understand better where it does not work maybe by initially increasing the temperature and humidity update frequency in order to exclude them.

sivar2311 commented 4 years ago

I don't like to see outdated data

Usually temperature and humidity don't change that much in a short period of time.

Since you're using this device in a bathroom i think you can implement another logic to your code: calculate the delta values! (calculate) how much the temperature and humidity have changed since last check)

If this delta value is above a certain level, lets say temperature raised 2°C within a 30 seconds or a minute, you can send an extra event for this, but main loop will continue reporting every 5 minutes.

conuxconux commented 4 years ago

I understand what do you mean. I'm searching for an other soluction. When sendmessage return false I can wait for a minute before try to update the data with event.

You can send 10 powerStates events (with a minimum distance of 1 second) until powerState event will limited to 1 event per minute.

It is like this (each " " representing an event):

How much time to restore the 10 events?

sivar2311 commented 4 years ago

1 minute per one event...so to "refill" all 10 events you have to wait ten minutes (without sending events during this time).

Background: We use Leaky Bucket algorithm for this with one extension: A new "drop" can be added every second

conuxconux commented 4 years ago

Ok. Many thanks.

conuxconux commented 4 years ago

Hi @sivar2311, but

You can send 10 powerStates events (with a minimum distance of 1 second) until powerState event will limited to 1 event per minute.

It is like this (each " " representing an event):

Is it for single device? If I had another device the count of events is separated not global, right?

sivar2311 commented 4 years ago

This limitation is for each event type (powerstate, temperature, powerlevel etc) per device. Did you ran into another problem? Or is it related to your current project?

Please: Do not use multiple devices to send same data in a higher frequency. For your project, just do it as i said before: Sending temperature / humidity in a constant rate about 5 minutes. If temperature / humidity change drasticly, do an intermideate update on this values. Doing so you will have always the actual temperature / humidity from your room.

conuxconux commented 4 years ago

In the current one, I check the temperature and humidity every 3 minutes. I thought I would use another thermostat to view the current dew point and change the dew point target vocally with Alexa.

Many thanks

sivar2311 commented 4 years ago

3 Minutes are good i think. Or if there is a bigger change like about 0.5°C - 1°C which should be reported by sending an additional event.

conuxconux commented 4 years ago

I think that 3 minutes should be fine. Thank you for your suggestions.

Have a nice evening.

kakopappa commented 4 years ago

Hi, thank you for your input.

If you have any suggestions or improvement ideas, most welcome

On Tue, 3 Dec 2019 at 2:49 AM conuxconux notifications@github.com wrote:

I think that 3 minutes should be fine. Thank you for your suggestions.

Have a nice evening.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/sinricpro/esp8266-esp32-sdk/issues/10?email_source=notifications&email_token=ABZAZZX67OUSFWX64FGZ4XDQWVRFVA5CNFSM4JTISTD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFUVXFI#issuecomment-560552853, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZAZZWVBG7YFQAWV4E2DI3QWVRFVANCNFSM4JTISTDQ .