espressif / esp-matter

Espressif's SDK for Matter
Apache License 2.0
677 stars 155 forks source link

Sending a response back to Matter client from a command (CON-1276) #1026

Open pavel808 opened 2 months ago

pavel808 commented 2 months ago

I'm working with the esp32-c6. I have a customized cluster server in my application. I have successfully implemented commands which update my own attributes.

One thing i'm struggling with is how to send a response back to a client when it sends a command. What i'm attempting to do is have a command which simply requests the temperature of the device.

I have used other commands and callbacks already implemented such as esp_matter_command_callback_get_holiday_schedule as references to help write my own . Here is my callback and data :

namespace GetTemperature {

struct Type
{
public:
    // Use GetCommandId instead of commandId directly to avoid naming conflict with CommandIdentification in ExecutionOfACommand
    static constexpr chip::CommandId GetCommandId() { return get_temperature_id; }
    static constexpr chip::CommandId GetClusterId() { return ir_cluster_id; }

    uint16_t temperature = static_cast<uint16_t>(0);

    CHIP_ERROR Encode(chip::TLV::TLVWriter & aWriter, chip::TLV::Tag aTag) const;

    using ResponseType = chip::app::DataModel::NullObjectType;

    static constexpr bool MustUseTimedInvoke() { return false; }
};

struct DecodableType
{
public:
    static constexpr chip::CommandId GetCommandId() { return get_temperature_id; }
    static constexpr chip::CommandId GetClusterId() { return ir_cluster_id; }

    uint16_t temperature = static_cast<uint16_t>(0);

    CHIP_ERROR Decode(TLVReader & reader);
};

}; // namespace GetTemperature

// This gets invoked when my "Get_Temperature" command is sent from a Matter client
static esp_err_t get_temperature_callback(const ConcreteCommandPath &command_path, TLVReader &tlv_data,
                                                           void *opaque_ptr)
{
    /* Get ids */
    uint16_t endpoint_id = command_path.mEndpointId;
    uint32_t cluster_id = command_path.mClusterId;
    uint32_t command_id = command_path.mCommandId;
    //uint32_t attribute_id = temperature_attribute_id;
    float tsens_value;

    /* Return if this is not the set_command command */
    if (endpoint_id != light_endpoint_id || cluster_id != ir_cluster_id || command_id != get_temperature_id) {
        ESP_LOGE(TAG, "ERROR! get_temperature_callback has incorrect ids for endpoint, cluster or command");
        return ESP_FAIL;
    }

    ESP_LOGI(TAG, "Reading temperature");
    ESP_ERROR_CHECK(temperature_sensor_get_celsius(temp_sensor, &tsens_value));
    ESP_LOGI(TAG, "Temperature value %.02f ℃", tsens_value);

    chip::app::CommandHandler * commandObj = (chip::app::CommandHandler *)opaque_ptr;

    GetTemperature::Type response;
    response.temperature = tsens_value;

    commandObj->AddResponse(command_path, response);

    return ESP_OK;
}

Of course this is failing with the below linker error.

Is there a more straight forward way to send a response back to the client? Am I going the right way about this here? How could I possibly solve this? Thanks in advance.

undefined reference to `GetTemperature::Type::Encode(chip::TLV::TLVWriter&, chip::TLV::Tag) const'
collect2: error: ld returned 1 exit status
jonsmirl commented 2 months ago

Another way to implement this is composed devices with child endpoints. image So instead of implementing a command to read the temperature, instead you use the existing temp sensor code to make a child endpoint.

jonsmirl commented 2 months ago

Another option is to add the temp sensor cluster to your device. That's probably the easiest.

pavel808 commented 2 months ago

Another option is to add the temp sensor cluster to your device. That's probably the easiest.

Hi @jonsmirl Thanks for your replies and suggestions. I think this is the way I would prefer. To add the temp sensor cluster to my device. I would like to have the same endpoint in all cases and not multiple endpoints.

Basically I am working on my own modified version of the Light example. Creating a new cluster was fine, but how do I add an existing cluster to my endpoint from the application layer? Below is some of the code inside main_app.c

 // node handle can be used to add/modify other endpoints.
    node_t *node = node::create(&node_config, app_attribute_update_cb, app_identification_cb);
    ABORT_APP_ON_FAILURE(node != nullptr, ESP_LOGE(TAG, "Failed to create Matter node"));

    extended_color_light::config_t light_config;
    light_config.on_off.on_off = DEFAULT_POWER;
    light_config.on_off.lighting.start_up_on_off = nullptr;
    light_config.level_control.current_level = DEFAULT_BRIGHTNESS;
    light_config.level_control.lighting.start_up_current_level = DEFAULT_BRIGHTNESS;
    light_config.color_control.color_mode = (uint8_t)ColorControl::ColorMode::kColorTemperature;
    light_config.color_control.enhanced_color_mode = (uint8_t)ColorControl::ColorMode::kColorTemperature;
    light_config.color_control.color_temperature.startup_color_temperature_mireds = nullptr;

    // endpoint handles can be used to add/modify clusters.
    endpoint_t *endpoint = extended_color_light::create(node, &light_config, ENDPOINT_FLAG_NONE, light_handle);
    ABORT_APP_ON_FAILURE(endpoint != nullptr, ESP_LOGE(TAG, "Failed to create extended color light endpoint"));

    light_endpoint_id = endpoint::get_id(endpoint);

   // Creating my own cluster here
    err = ir_cluster_create(endpoint, light_endpoint_id);
    if (err != ESP_OK)
    {
        ESP_LOGI(TAG, "ERROR! Unable to create custom cluster. Error : %d", err);
        return;
    }

extended_color_light::config already has clusters assigned at the components level as below. What would be the best way to add the temp sensor cluster here to my device in this case? Thanks in advance.

namespace extended_color_light {
typedef struct config {
    cluster::descriptor::config_t descriptor;
    cluster::identify::config_t identify;
    cluster::groups::config_t groups;
    cluster::scenes_management::config_t scenes_management;
    cluster::on_off::config_t on_off;

    cluster::level_control::config_t level_control;
    cluster::color_control::config_t color_control;
} config_t;
jonsmirl commented 2 months ago

binding::config_t binding_config; binding::create(ep, &binding_config, CLUSTER_FLAG_SERVER);

cluster_t *onoff_cluster = on_off::create(ep, &ext_config.on_off, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT, on_off::feature::lighting::get_id ());

cluster_t *level_control_cluster = level_control::create(ep, &ext_config. level_control, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT, level_control::feature::on_off::get_id() | level_control::feature::lighting ::get_id());

On Tue, Jul 30, 2024 at 1:40 PM pavel808 @.***> wrote:

Another option is to add the temp sensor cluster to your device. That's probably the easiest.

Hi @jonsmirl https://github.com/jonsmirl Thanks for your replies and suggestions. I think this is the way I would prefer. To add the temp sensor cluster to my device. I would like to have the same endpoint in all cases and not multiple endpoints.

Basically I am working on my own modified version of the Light example. Creating a new cluster was fine, but how do I add an existing cluster to my endpoint from the application layer? Below is some of the code inside main_app.c

// node handle can be used to add/modify other endpoints. node_t *node = node::create(&node_config, app_attribute_update_cb, app_identification_cb); ABORT_APP_ON_FAILURE(node != nullptr, ESP_LOGE(TAG, "Failed to create Matter node"));

extended_color_light::config_t light_config;
light_config.on_off.on_off = DEFAULT_POWER;
light_config.on_off.lighting.start_up_on_off = nullptr;
light_config.level_control.current_level = DEFAULT_BRIGHTNESS;
light_config.level_control.lighting.start_up_current_level = DEFAULT_BRIGHTNESS;
light_config.color_control.color_mode = (uint8_t)ColorControl::ColorMode::kColorTemperature;
light_config.color_control.enhanced_color_mode = (uint8_t)ColorControl::ColorMode::kColorTemperature;
light_config.color_control.color_temperature.startup_color_temperature_mireds = nullptr;

// endpoint handles can be used to add/modify clusters.
endpoint_t *endpoint = extended_color_light::create(node, &light_config, ENDPOINT_FLAG_NONE, light_handle);
ABORT_APP_ON_FAILURE(endpoint != nullptr, ESP_LOGE(TAG, "Failed to create extended color light endpoint"));

light_endpoint_id = endpoint::get_id(endpoint);

// Creating my own cluster here err = ir_cluster_create(endpoint, light_endpoint_id); if (err != ESP_OK) { ESP_LOGI(TAG, "ERROR! Unable to create custom cluster. Error : %d", err); return; }

extended_color_light::config already has clusters assigned at the components level as below. What would be the best way to add the temp sensor cluster here to my device in this case? Thanks in advance.

namespace extended_color_light { typedef struct config { cluster::descriptor::config_t descriptor; cluster::identify::config_t identify; cluster::groups::config_t groups; cluster::scenes_management::config_t scenes_management; cluster::on_off::config_t on_off;

cluster::level_control::config_t level_control;
cluster::color_control::config_t color_control;

} config_t;

— Reply to this email directly, view it on GitHub https://github.com/espressif/esp-matter/issues/1026#issuecomment-2258876068, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADUX4DMGE4SOYS3YBN26LZO7F2VAVCNFSM6AAAAABLPQ6662VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYHA3TMMBWHA . You are receiving this because you were mentioned.Message ID: @.***>

-- Jon Smirl @.***

pavel808 commented 2 months ago

binding::config_t binding_config;

Hi @jonsmirl I'm not sure if I understand this.

In fact it probably makes more sense to add the light sensor as a new endpoint to the same node I have.

endpoint_t *temp_ep = temperature_sensor::create(node, &temp_config, ENDPOINT_FLAG_NONE, NULL);

What I don't get is that there doesn't seem to be an existing command for reading the temperature. I see only this. So it's a bit confusing:

command_t *create_set_temperature(cluster_t *cluster);

jonsmirl commented 2 months ago

You can always put the sensors in new endpoints. But if you do that they will be separate devices in the Apple/Google/Amazon UI. I didn't think you wanted that.

pavel808 commented 2 months ago

You can always put the sensors in new endpoints. But if you do that they will be separate devices in the Apple/Google/Amazon UI. I didn't think you wanted that.

@jonsmirl Ah that's a good point. It's better to have the same endpoint in that case. I'm still struggling though to attach the temperature sensor cluster. Also, there doesn't seem to be existing commands to read the temperature via that cluster?

jonsmirl commented 2 months ago

You don't need a special command to read it, the current temp is an attribute on that cluster.

pavel808 commented 2 months ago

CLUSTER_FLAG_SERVER

Ok. I wasn't aware that the attribute can be read directly from a Matter cluster server from a client without a command. I am new to Matter, so still figuring this out.

In fact, I should be just able to add the temperature_measurement cluster to my endpoint as follows correct? :

temperature_measurement::config_t temp_config;
esp_matter::cluster_t *cluster = temperature_measurement::create(endpoint, &temp_config, CLUSTER_FLAG_SERVER);
jonsmirl commented 2 months ago

Yes, and then the cluster and the attribute will appear on the endpoint. When you subscribe to the end point you will receive temp updates.

pavel808 commented 2 months ago

Yes, and then the cluster and the attribute will appear on the endpoint. When you subscribe to the end point you will receive temp updates.

Thanks.

I am using chip-tool to try and read the measured temperature value as follows :

chip-tool temperaturemeasurement read measured-value 0x7394 0x1

The measured value is NULL as below. Is there some other initialization procedure required? Thanks for all your help.

[1722440375.419419][33114:33116] CHIP:TOO: Endpoint: 1 Cluster: 0x0000_0402 Attribute 0x0000_0000 DataVersion: 2333107726
[1722440375.419490][33114:33116] CHIP:TOO:   MeasuredValue: null
jonsmirl commented 2 months ago

you have to set the attribute in the device

        esp_matter_attr_val_t val;
        val = esp_matter_int16(event.sensors.temperature);
        attribute::update(endpoint::get_id(getEndpoint()),
                          chip::app::Clusters::TemperatureMeasurement::Id,
                          chip::app::Clusters::TemperatureMeasurement::Attributes::MeasuredValue::Id, &val);
pavel808 commented 2 months ago

chip::app::Clusters::TemperatureMeasurement::Id

Is there some example code which I can use as a reference to solve my issue? I'm not following this unfortunatley. Where about should this attribute::update be called? Do I need to call this within some callback? I called it in my initialization procedure but it's obviously not enough. Also event.sensors.temperature I can't find anywhere :-/ Thanks

jonsmirl commented 2 months ago

You read your temperature sensor locally and then do then update the attribute locally. You can do that as much as you want. If the temperature changes it will send a message out on the subscription. You can also remote read the attribute whenever you want too.

pavel808 commented 2 months ago

You read your temperature sensor locally and then do then update the attribute locally. You can do that as much as you want. If the temperature changes it will send a message out on the subscription. You can also remote read the attribute whenever you want too.

Ah ok. Now it makes more sense. That's why I was trying to implement a new command that only reads the temperature from the sensor when it is requested remotely.

At this stage there doesn't seem to be no way to do this. The measured value attribute must be periodically updated right? Then read it remotely for example using : chip-tool temperaturemeasurement read measured-value 0x7394 0x1

jonsmirl commented 2 months ago

chip-tool temperaturemeasurement read measured-value 0x7394 0x1 Reads it once on-demand

If you set up a subscription it will send a message whenever it changes

jonsmirl commented 2 months ago

Don't sit in a polling loop on your client doing: chip-tool temperaturemeasurement read measured-value 0x7394 0x1

That is what subscriptions are for.

jonsmirl commented 2 months ago

Read about chip-tool interactive mode. https://docs.espressif.com/projects/esp-matter/en/latest/esp32/developing.html#commissioning-and-control

pavel808 commented 2 months ago

chip-tool temperaturemeasurement read measured-value 0x7394 0x1 Reads it once on-demand

If you set up a subscription it will send a message whenever it changes

My use-case is different. My client may want to request the temperature once per hour, once per two hours etc. Whenever the user requests it on the client side.

So if I am reading the attribute directly remotely, am I right in that the attribute value must be kept updated periodically ? I mean the attribute value must be kept refreshed with the actual sensor value? So I need to poll it on the device side in some thread?

How I was attempting to do this initially was with my own cluster and command. For example, the user sends a "get temperature" command remotely, then a callback gets invoked on the device which calls temperature_sensor_get_celsius(temp_sensor, &tsens_value); as per the temperature example.

jonsmirl commented 2 months ago

We use sensor chips which generate an interrupt when the temp changes. TI HDC2010 family. Get the interrupt, read the sensor chip, update the attribute.

You can also capture the read request for the attribute, but then subscriptions won't work. This an advanced usage, you need to implement an AttributeAccessInterface. https://github.com/espressif/esp-matter/blob/main/components/esp_matter_rainmaker/esp_matter_rainmaker.cpp#L404

If you are doing this on batteries you will need to read about 'sleepy devices'.