Switch power state persist (MEGH-2324)

[adukale]

I have modified esp_rmaker_standard_params.c with following code to add persist flag to power parameter:

esp_rmaker_param_t *esp_rmaker_power_param_create(const char *param_name, bool val)
{
    esp_rmaker_param_t *param = esp_rmaker_param_create(param_name, ESP_RMAKER_PARAM_POWER,
            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE | PROP_FLAG_PERSIST);
    if (param) {
        esp_rmaker_param_add_ui_type(param, ESP_RMAKER_UI_TOGGLE);
    }
    return param;
}

But the same is not working. I have attached a relay to switch example GPIO, but after reboot power state is not retained.

Am I missing someadditional code?

Thanks is advance.

[solroshan]

Hey @adukale,

I did not work on this yet but i have similar trouble with power states after reboots. I'll back soon with my own solution if will not shared by rainmaker angels👾
UP

[shahpiyushv]

@adukale , I tried this out with the switch example and this indeed works for me. Here is the patch, which is similar to yours

diff --git a/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c b/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
index c8af62b..f919f9a 100644
--- a/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
+++ b/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
@@ -27,7 +27,7 @@ esp_rmaker_param_t *esp_rmaker_name_param_create(const char *param_name, const c
 esp_rmaker_param_t *esp_rmaker_power_param_create(const char *param_name, bool val)
 {
     esp_rmaker_param_t *param = esp_rmaker_param_create(param_name, ESP_RMAKER_PARAM_POWER,
-            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE);
+            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE | PROP_FLAG_PERSIST);
     if (param) {
         esp_rmaker_param_add_ui_type(param, ESP_RMAKER_UI_TOGGLE);
     }

The log shows me this message when I add the power parameter to switch. "Init" basically means that the callback was called because of a value available in persistent storage (nvs).

I (692) app_main: Received write request via : Init
I (692) app_main: Received value = false for Switch - Power

However, I found that there is a bug which causes the callback to be missed if you use the standard device APIs. The switch example uses the low level APIs as below, which work

    switch_device = esp_rmaker_device_create("Switch", ESP_RMAKER_DEVICE_SWITCH, NULL);
    esp_rmaker_device_add_cb(switch_device, write_cb, NULL);
    esp_rmaker_param_t *power_param = esp_rmaker_power_param_create(ESP_RMAKER_DEF_POWER_NAME, DEFAULT_POWER);

However, it does not work for the below code (which is used in multi device)

    switch_device = esp_rmaker_switch_device_create("Switch", NULL, DEFAULT_SWITCH_POWER);
    esp_rmaker_device_add_cb(switch_device, write_cb, NULL);

The reason for this is that here, the parameter gets added before the callback gets registered. I will check how this can be fixed.,

Can you confirm if your observation matches with this?

[solroshan]

@shahpiyushv @adukale Hello guys,

This is my win10 based working tags;
esp32-wroom-32
python 3.8.7
git 2.30.0
idf 4.2

My idf prompt warned me for *power_param

 unused variable 'power_param' [-Wunused-variable]
 esp_rmaker_param_t *power_param = esp_rmaker_power_param_create(ESP_RMAKER_DEF_POWER_NAME, 
 DEFAULT_WATER_POWER);

and this is esp_rmaker_standard_params.c

esp_rmaker_param_t *esp_rmaker_power_param_create(const char *param_name, bool val)
{
    esp_rmaker_param_t *param = esp_rmaker_param_create(param_name, ESP_RMAKER_PARAM_POWER,
            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE | PROP_FLAG_PERSIST);
    if (param) {
        esp_rmaker_param_add_ui_type(param, ESP_RMAKER_UI_TOGGLE);
    }
    return param;
}

and my esp info lines after flashed with this line;

I (16955) app_main: Received write request via : Local
I (16955) app_main: Received value = false for Light - Power
...
I (19245) app_main: Received write request via : Cloud
I (19255) app_main: Received value = false for Light - Power

Need to add esp_rmaker_param_get_stored_value function for get value from nvs? 🤔

[nirmeshru]

Hi @shahpiyushv,

I tried your patch to persist gipo pin status and made the change in the switch example.
I am getting the Init log but it is not reflecting on ESP-wroom-32 Dev Kit V1.
I am using GPIO 2 onboard LED to test gpio persist code.

I (1066) app_main: Received write request via : Init
I (1076) app_main: Received value = true for Switch - Power

any other change is required so please point to us. I am using the latest IDF version and mobile applications

Thanks,
Nirmesh

[adukale]

@adukale , I tried this out with the switch example and this indeed works for me. Here is the patch, which is similar to yours

diff --git a/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c b/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
index c8af62b..f919f9a 100644
--- a/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
+++ b/components/esp_rainmaker/src/standard_types/esp_rmaker_standard_params.c
@@ -27,7 +27,7 @@ esp_rmaker_param_t *esp_rmaker_name_param_create(const char *param_name, const c
 esp_rmaker_param_t *esp_rmaker_power_param_create(const char *param_name, bool val)
 {
     esp_rmaker_param_t *param = esp_rmaker_param_create(param_name, ESP_RMAKER_PARAM_POWER,
-            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE);
+            esp_rmaker_bool(val), PROP_FLAG_READ | PROP_FLAG_WRITE | PROP_FLAG_PERSIST);
     if (param) {
         esp_rmaker_param_add_ui_type(param, ESP_RMAKER_UI_TOGGLE);
     }

The log shows me this message when I add the power parameter to switch. "Init" basically means that the callback was called because of a value available in persistent storage (nvs).

I (692) app_main: Received write request via : Init
I (692) app_main: Received value = false for Switch - Power

However, I found that there is a bug which causes the callback to be missed if you use the standard device APIs. The switch example uses the low level APIs as below, which work

    switch_device = esp_rmaker_device_create("Switch", ESP_RMAKER_DEVICE_SWITCH, NULL);
    esp_rmaker_device_add_cb(switch_device, write_cb, NULL);
    esp_rmaker_param_t *power_param = esp_rmaker_power_param_create(ESP_RMAKER_DEF_POWER_NAME, DEFAULT_POWER);

However, it does not work for the below code (which is used in multi device)

    switch_device = esp_rmaker_switch_device_create("Switch", NULL, DEFAULT_SWITCH_POWER);
    esp_rmaker_device_add_cb(switch_device, write_cb, NULL);

The reason for this is that here, the parameter gets added before the callback gets registered. I will check how this can be fixed.,

Can you confirm if your observation matches with this?

Correct. This doesn't work in multi device example.
I am trying to add multiple devices into the the switch example to bypass this issue.

[shahpiyushv]

@nirmeshru , have you changed the output gpio number here?

[nirmeshru]

@shahpiyushv yes, I changed the gpio number to gpio 2.

[adukale]

@shahpiyushv I can confirm the behaviour reported by @nirmeshru
Serial monitor shows below:

I (1316) app_main: Received write request via : Init
I (1316) app_main: Received value = false for Switch1 - Power
I (1326) app_main: Received write request via : Init
I (1326) app_main: Received value = false for Switch2 - Power
I (1336) app_main: Received write request via : Init
I (1336) app_main: Received value = true for Switch3 - Power
I (1346) app_main: Received write request via : Init
I (1356) app_main: Received value = false for Switch4 - Power

But the state does not get updated.

[adukale]

I think I found out an issue:
To give a background - I was configuring 4 Channel relay with switch example. So I added 2 switches first and then 2 more.
So the initial 2 switches had: app_driver_set_state(0); & app_driver_set_state2(0); methods and then at the time of next 2 switches I got lazy and directly added gpio_set_level(21, 0); method in write_cb callback.
This is when I discovered the behaviour, the 2 switches where app_driver_set_state(0) & app_driver_set_state2(0) methods was used was not updating the state reported by init but at the same time the rest 2 switches where gpio_set_level(21, 0); was used, was restoring state from init. Now I have added gpio_set_level(21, 0); in all4 switches callbacks, init state is being set at startup.
This is just a workaround / dirty hack to circumvent the bug. So I think when app_driver_init(); gets called, it reports the last state but somehow it fails to set the state of GPIO.

I'll try to find more onto this.

[shahpiyushv]

@adukale , @nirmeshru once you get the callback, the rest of the logic is in the example code itself, and not the core rainmaker functionality. Since you are getting the above prints, the "persistency" itself is working fine and clearly the issue is with the driver. I found that the culprit is this line which was causing set_power_state(g_power_state) to get missed. This problem is independent of the "persistency". Just removing that line should solve the issue for you. Can you check?

[adukale]

@shahpiyushv Correct. Removed the line and now switch state is getting restored.

Closing the issue as the same is resolved.

[adukale]

I have noticed one bug lately but didn't pay attention to. The switch state persists after reboot and the state gets updated correctly in ESP Rainmaker app however the state doesn't get updated in HomeKit app. The state is always true or false which is defined here: https://github.com/espressif/esp-rainmaker/blob/master/examples/homekit_switch/main/app_main.c#L222

Is there any way to update switch status from NVS in HomeKit?

[shahpiyushv]

In RainMaker, for persistent params, the write callback gets invoked. The callback not only updates the actual state of the switch and data model in RainMaker, but also updates the HomeKit data. However, during initialisation, the HomeKit update gets ignored because it is initialised after RainMaker. I think you can capture the value of power received in write_cb for ctx->src = ESP_RMAKER_REQ_SRC_INIT and pass it to app_homekit_start(). We will make similar changes in the example or you may raise a PR for that.

[adukale]

Thank you for your response, however I am getting little bit confused. Can you share an exmaple?
Also, the solution you are proposing will work even if there are multiple devices, right?

[shahpiyushv]

Here is a quick (but untested) patch. Please try and let us know if it works

diff --git a/examples/homekit_switch/main/app_main.c b/examples/homekit_switch/main/app_main.c
index 4d473f5..fb26dd4 100644
--- a/examples/homekit_switch/main/app_main.c
+++ b/examples/homekit_switch/main/app_main.c
@@ -30,6 +30,7 @@
 
 static const char *TAG = "app_main";
 esp_rmaker_device_t *switch_device;
+bool switch_init_state = DEFAULT_POWER;
 
 /* Callback to handle commands received from the RainMaker cloud */
 static esp_err_t write_cb(const esp_rmaker_device_t *device, const esp_rmaker_param_t *param,
@@ -45,6 +46,9 @@ static esp_err_t write_cb(const esp_rmaker_device_t *device, const esp_rmaker_pa
         app_driver_set_state(val.val.b);
         esp_rmaker_param_update(param, val);
         app_homekit_update_state(val.val.b);
+        if (ctx->src == ESP_RMAKER_REQ_SRC_INIT) {
+            switch_init_state = val.val.b;
+        }
     }
     return ESP_OK;
 }
@@ -219,7 +223,7 @@ void app_main()
     esp_rmaker_start();
 
     /* Start the HomeKit module */
-    app_homekit_start(DEFAULT_POWER);
+    app_homekit_start(switch_init_state);
 
     /* Start the Wi-Fi.
      * If the node is provisioned, it will start connection attempts,

[adukale]

Thanks for your quick response. I got your logic now but won't this apply "switch_init_state" to all switches added in this example?
I have added 6 switches in homekit_example and here is my write_cb function for reference:

static esp_err_t write_cb(const esp_rmaker_device_t *device, const esp_rmaker_param_t *param,
            const esp_rmaker_param_val_t val, void *priv_data, esp_rmaker_write_ctx_t *ctx)
{
    if (ctx) {
        ESP_LOGI(TAG, "Received write request via : %s", esp_rmaker_device_cb_src_to_str(ctx->src));
    }
    if (strcmp(esp_rmaker_param_get_name(param), ESP_RMAKER_DEF_POWER_NAME) == 0) {
        app_homekit_start(false);
        ESP_LOGI(TAG, "Received value = %s for %s - %s",
                val.val.b? "true" : "false", esp_rmaker_device_get_name(device),
                esp_rmaker_param_get_name(param));
        set_power_state(device,val.val.b);
        esp_rmaker_param_update(param, val);
        app_homekit_update_state(device,val.val.b);
    }
    return ESP_OK;
}

I have modified set_power_state() and app_homekit_update_state() functions to accept device as parameter.

With your example code the switch_init_state will be applicable for all 5 devices.

[shahpiyushv]

Assuming that you have modified app_homekit_start() to initialise your HomeKit switches, you can also try calling this BEFORE you add the devices in RainMaker. This way, since the HomeKit data model will already be initialised, the app_homekit_update_state() in write_cb will ensure that the switch states are correctly updated in HomeKit,

[adukale]

Actually due to oversight I had missed modifying app_homekit_start(), thanks to you I was able to find it and fix.
So as per your inputs I have added below in write_cb

if (ctx->src == ESP_RMAKER_REQ_SRC_INIT) {
       
           if (device == switch1_device)
           switch1_init_state = val.val.b;
           
           if (device == switch2_device)
           switch2_init_state = val.val.b;

           if (device == switch3_device)
           switch3_init_state = val.val.b;

           if (device == switch4_device)
           switch4_init_state = val.val.b;

           if (device == switch5_device)
           switch5_init_state = val.val.b;

           if (device == switch6_device)
           switch6_init_state = val.val.b;
           
           }

Then I am executing below

app_homekit_start(switch1_init_state, switch2_init_state, switch3_init_state, switch4_init_state, switch5_init_state, switch6_init_state);

And finally, initializing the accessories in homekit

 service1 = hap_serv_switch_create(init_state1);
 service2 = hap_serv_switch_create(init_state2);
 service3 = hap_serv_switch_create(init_state3);
 service4 = hap_serv_switch_create(init_state4);
 service5 = hap_serv_switch_create(init_state5);
 service6 = hap_serv_switch_create(init_state6);

This seems to be working, I'll test for some time.

Also, I tried executing app_homekit_start() before esp_rmaker_node_add_device() but this didn't result in expected behaviour. I will try again and see if this gives results.

Thanks for your kind support.