Window Covering command stop indistinguishable (CON-799)

[svenehlers]

I am thinking about how to distinguish the window covering stop command from other drive commands. The chip layer is changing the target position and tilt upon receiving a stop command to the current position and tilt but in the attribute update callback i can not detect if the chip layer received a drive command with the current position or tilt as parameter or if it was caused by a stop command. This could be handled by updating the current position in 'real time' but this takes too much computation time and produces too much traffic. Currently the current position and tilt is updated after the window covering stopped only. While driving the current position and tilt is always given from the previous stopping event.

At this moment i dont see any solution but setting an extra flag when the stop command callback is called in the esp-matter layer or chip layer. Does anybody knows another solution or has any concerns about manipulating the esp-matter source file.

[jonsmirl]

Have you looked at the internals of the window covering cluster API? https://github.com/project-chip/connectedhomeip/tree/master/src/app/clusters/window-covering-server

[svenehlers]

I have checked it and there is happening what i described. The target position/tilt is set to the current position/tilt which is executing the callback for attribute changed. For me this looks like the implementation of the chip layer is not thought on a practical basis or i am missing something obvious :D.

[Diegorro98]

Back on the day I suggested in a PR the ability to override command callback (#35) because I needed to stop (and move) the Window Covering device when Position Aware features weren't enabled so that there is no way to use position to tell esp-matter to move the device. As the PR hasn't been merged yet, I step in and did some "not-cool" coding in order to be able to override the command without modifying esp-matter (so the project could be build with docker images and similar):

First, you need to copy this piece of code wherever you want to modify the command : https://github.com/espressif/esp-matter/blob/1cacb8b2fcd42c465e38317d3a63d0169460b397/components/esp_matter/esp_matter_core.cpp#L157-L162 (This is needed because as it is defined at a cpp file, AFAIK there is no way to access it)

Then you define your command (I leave this template here that can help you with getting the handles and setting a response for Matter with commandObj->AddStatus())

esp_err_t stop_movement(const ConcreteCommandPath &command_path, TLVReader &tlv_data, void *opaque_ptr) {
    chip::app::CommandHandler *commandObj = (chip::app::CommandHandler *)opaque_ptr; 
    chip::EndpointId endpoint = command_path.mEndpointId;
    ESP_LOGI(TAG, "StopMotion command received for endpoint %d", endpoint);

    if (endpoint == endpoint_id ) {
        chip::Protocols::InteractionModel::Status status = GetMotionLockStatus(endpoint);
        if (Status::Success != status)
        {
            ESP_LOGE(TAG, "Err device locked");
            commandObj->AddStatus(commandPath, status);
            return ESP_FAIL;
        }
        /* TODO get the handle of your device */
        device_handle* deivce_handle= (device_handle*)get_priv_data(endpoint);
        if (device_handle== NULL) {
            commandObj->AddStatus(command_path, Status::Failure);
            return ESP_FAIL;
        }

        commandObj->AddStatus(command_path, chip::Protocols::InteractionModel::Status::Success);
        /* TODO perform stop logic */
        deivce_handle->stop();
        return ESP_OK;
    } else {
        ESP_LOGE(TAG, "Stop command sent to an unknown endpoint");
        commandObj->AddStatus(command_path, chip::Protocols::InteractionModel::Status::NotFound);
        return ESP_FAIL;
    }
}

commandObj->AddStatus() should be called ASAP to notify that the device has correctly received the command, everything is ok, and the device can perform the required action.

Lastly, override the command:

((_command_t *)esp_matter::command::get(cluster_t_handle, WindowCovering::Commands::StopMotion::Id, COMMAND_FLAG_ACCEPTED))->callback = stop_movement;

Maybe there is another way to do this such as modifying the delegate that is at app/clusters/window-covering-server/window-covering-server.cpp#L741 but I didn't explore more as the way I explained worked for me.

[svenehlers]

@Diegorro98 Thank you very much! I guess i will go on with this approach.

[jonsmirl]

set_plugin_server_init_callback(cluster, MatterWindowCoveringPluginServerInitCallback);

You can intercept the call by writing your own plugin sever which then chains on into the Matter one.

set_plugin_server_init_callback(cluster, myModifiedMatterWindowCoveringPluginServerInitCallback);

[Diegorro98]

@svenehlers glad to help! 😊 I was thinking that maybe you should leave this issue open so that way maybe the PR I made or any other solution to change the callback is pushed, because there should be an official and clean way to set the callback. I mean, how would espressif or any member from the Matter working group develop a Window Covering device that is not position aware then target position cannot be set.

[jmeg-sfy]

Hello here, you could provide a PR to the main window covering server if needed. The current implementation is far from being optimal and has been done to accommodate the examples and CI. A real product would handles or customize the server following its own need.

The most important thing to keep in mind for a product Position Aware is after a Stop Command the attributes Current and Target must be equal, that it !

I am not an ESP user , so i don't know how your examples or SDK are working TBH

[jmeg-sfy]

set_plugin_server_init_callback(cluster, MatterWindowCoveringPluginServerInitCallback);

You can intercept the call by writing your own plugin sever which then chains on into the Matter one.

set_plugin_server_init_callback(cluster, myModifiedMatterWindowCoveringPluginServerInitCallback);

does set_plugin_server_init_callback is from Matter's SDK or this a thing from ESP's SDK ?

[svenehlers]

I think now i implemented it in a way how it was intended by CHIP and how you suggested in a previous comment @Diegorro98 . I am defining a window covering delegate and inject it into the window covering endpoint. This was not possible for CHIP v1.0 but since v1.1 it is.

class CoveringDelegate : public chip::app::Clusters::WindowCovering::Delegate{
    public:

        CHIP_ERROR HandleMovement(chip::app::Clusters::WindowCovering::WindowCoveringType type){
            return CHIP_NO_ERROR;
        }

        CHIP_ERROR HandleStopMotion(){
            //Handle stopping of covering here
            return CHIP_ERROR_IN_PROGRESS;
        }

        ~CoveringDelegate() = default;
};

By returning CHIP_ERROR_IN_PROGRESS the stop command willl not update the target lift/tilt by its own. I do it on my own after the covering really stopped.

covering_delegate = CoveringDelegate();
covering_delegate.SetEndpoint(window_covering_endpoint_id);
chip::app::Clusters::WindowCovering::SetDefaultDelegate((chip::EndpointId) window_covering_endpoint_id, &covering_delegate);

where covering_delegate is declared in a global scope.

I had to add this line before calling SetEndpoint:

esp_matter::endpoint::enable(endpoint_window_covering_device);