CHIP missing support in app/cluster/thermostat/thermostat-server.c for scheduling commands (CON-1146)

[jonsmirl]

Chip code is missing the weak modifiers allowing the default do-nothing implementations to be overridden. thermo.diff.txt

[jadhavrohit924]

@jonsmirl Can you please explain a bit more, what are you trying to achieve?

[jonsmirl]

I am working on implementing the scheduling feature for my thermostat. To do that you need to implement those commands in your own code. Section 4.3.10 of Cluster spec. After I implemented those functions I couldn't link because they were already defined. Adding the _weak attribute lets my implementation replace the do-nothing one. If you look at doorlock you can see _weak was already added there... https://github.com/project-chip/connectedhomeip/blob/master/src/app/clusters/door-lock-server/door-lock-server-callback.cpp#L47

[jonsmirl]

Hmm... there probably should be a ThermostatDataProvider() object and then these commands would call into that provider. Then I would override the ThermostatDataProvider implementation. https://github.com/project-chip/connectedhomeip/issues/33406

[jonsmirl]

The implementation in app/clusters/thermostat-server does not follow the spec. Many of those command stubs are commands which don't exist.

[jonsmirl]

Implementing this is some good value add code for esp-matter since every thermostat has to implement it exactly the same. I implemented some of the methods to demonstrate what is needed.

There is a generic ThermostatDataProvider which gets added to app/src/clusters/thermostat-server.cpp. It has all virtual methods which return unimplemented.

The emberAf... commands are also implemented in app/src/clusters/thermostat-server.cpp. I made some skeletons, but GetWeeklySchedule is not building the response object.

Finally you would add EspThermostatDataProvider in the esp-matter code. At boot it would load this data structure from flash. EspThermostatDataProvider then implements the SetWeeklySchedule, GetWeeklySchedule and ClearWeeklySchedule commands. Those implementations update the data structure in memory while also keeping it in sync on flash. Then you need to schedule these events and generate a callback as they occur, finally my code implements that callback. You can assume the ESP clock has been correctly set.

class ThermostatDataProvider
{
public:
    virtual CHIP_ERROR SetWeeklySchedule(uint8_t numberOfTransitionsForSequence,
                                         chip::BitMask<chip::app::Clusters::Thermostat::ScheduleDayOfWeekBitmap> dayOfWeekForSequence,
                                         chip::BitMask<chip::app::Clusters::Thermostat::ScheduleModeBitmap> modeForSequence,
                                         chip::app::DataModel::DecodableList<chip::app::Clusters::Thermostat::Structs::WeeklyScheduleTransitionStruct::DecodableType> transitions) { return CHIP_ERROR_NOT_IMPLEMENTED; };
    virtual CHIP_ERROR GetWeeklySchedule(chip::BitMask<chip::app::Clusters::Thermostat::ScheduleDayOfWeekBitmap> daysToReturn,
                                         chip::BitMask<chip::app::Clusters::Thermostat::ScheduleModeBitmap> modeToReturn) { return CHIP_ERROR_NOT_IMPLEMENTED; };
    virtual CHIP_ERROR ClearWeeklySchedule() { return CHIP_ERROR_NOT_IMPLEMENTED; };
};

class EspThermostatDataProvider : public ThermostatDataProvider
{
public:
    CHIP_ERROR SetWeeklySchedule(uint8_t numberOfTransitionsForSequence,
                                 chip::BitMask<chip::app::Clusters::Thermostat::ScheduleDayOfWeekBitmap> dayOfWeekForSequence,
                                 chip::BitMask<chip::app::Clusters::Thermostat::ScheduleModeBitmap> modeForSequence,
                                 chip::app::DataModel::DecodableList<chip::app::Clusters::Thermostat::Structs::WeeklyScheduleTransitionStruct::DecodableType> transitions);
};

CHIP_ERROR EspThermostatDataProvider::SetWeeklySchedule(uint8_t numberOfTransitionsForSequence,
                                                       chip::BitMask<chip::app::Clusters::Thermostat::ScheduleDayOfWeekBitmap> dayOfWeekForSequence,
                                                       chip::BitMask<chip::app::Clusters::Thermostat::ScheduleModeBitmap> modeForSequence,
                                                       chip::app::DataModel::DecodableList<chip::app::Clusters::Thermostat::Structs::WeeklyScheduleTransitionStruct::DecodableType> transitions)
{
    auto iterator = transitions.begin();
    while (iterator.Next())
    {
        auto &item = iterator.GetValue();

        // store this stuff in flash
        ESP_LOGI(TAG, "time %d heat %d cool %d", item.transitionTime, item.heatSetpoint, item.coolSetpoint);
    }
    return CHIP_ERROR_NOT_IMPLEMENTED;
}

ThermostatDataProvider *provider = new EspThermostatDataProvider();

bool emberAfThermostatClusterClearWeeklyScheduleCallback(app::CommandHandler *commandObj,
                                                         const app::ConcreteCommandPath &commandPath,
                                                         const Commands::ClearWeeklySchedule::DecodableType &commandData)
{
    ESP_LOGI(TAG, "emberAfThermostatClusterClearWeeklyScheduleCallback");

    CHIP_ERROR err = provider->ClearWeeklySchedule();

    Status status = Status::Success;
    if (CHIP_ERROR_NOT_FOUND == err || CHIP_ERROR_KEY_NOT_FOUND == err)
    {
        status = Status::NotFound;
    }
    else if (CHIP_NO_ERROR != err)
    {
        status = Status::Failure;
    }

    // Send status response.
    commandObj->AddStatus(commandPath, status, "ClearWeeklySchedule failed");
    return false;
}

bool emberAfThermostatClusterGetWeeklyScheduleCallback(app::CommandHandler *commandObj,
                                                       const app::ConcreteCommandPath &commandPath,
                                                       const Commands::GetWeeklySchedule::DecodableType &commandData)
{
    ESP_LOGI(TAG, "emberAfThermostatClusterGetWeeklyScheduleCallback");
    CHIP_ERROR err = provider->GetWeeklySchedule(commandData.daysToReturn,
                                                 commandData.modeToReturn);

    Status status = Status::Success;
    if (CHIP_ERROR_NOT_FOUND == err || CHIP_ERROR_KEY_NOT_FOUND == err)
    {
        status = Status::NotFound;
    }
    else if (CHIP_NO_ERROR != err)
    {
        status = Status::Failure;
    }

    // Send status response.
    commandObj->AddStatus(commandPath, status, "GetWeeklySchedule failed");
    return false;
}

bool emberAfThermostatClusterSetWeeklyScheduleCallback(app::CommandHandler *commandObj,
                                                       const app::ConcreteCommandPath &commandPath,
                                                       const Commands::SetWeeklySchedule::DecodableType &commandData)
{
    ESP_LOGI(TAG, "emberAfThermostatClusterSetWeeklyScheduleCallback");
    CHIP_ERROR err = provider->SetWeeklySchedule(commandData.numberOfTransitionsForSequence,
                                                 commandData.dayOfWeekForSequence,
                                                 commandData.modeForSequence,
                                                 commandData.transitions);

    Status status = Status::Success;
    if (CHIP_ERROR_NOT_FOUND == err || CHIP_ERROR_KEY_NOT_FOUND == err)
    {
        status = Status::NotFound;
    }
    else if (CHIP_NO_ERROR != err)
    {
        status = Status::Failure;
    }

    // Send status response.
    commandObj->AddStatus(commandPath, status, "SetWeeklySchedule failed");
    return false;
}

[jonsmirl]

There is a problem in the spec here too. Thermostats also control the HVAC fan. That should have another row for fan on/off.

Example from US existing US thermostat

Note on the front page how I have fan control. I have same controls on the actual thermostat too.

Also note the event alarms at the bottom. When the Zigbee alarms were removed from the thermostat cluster they were not replaced with Matter events.

[jonsmirl]

Thermostat cluster is also missing Heat Pump control with stages.

[jonsmirl]

These should be settable via cluster::thermostat::config_t, right? Then then the OTA image can set them.

[jonsmirl]

Has scheduling been removed from the thermostat?

[jadhavrohit924]

Hi @jonsmirl, if you think there is issue with spec and CHIP sdk can you please file those in https://github.com/CHIP-Specifications/connectedhomeip-spec/issues and https://github.com/project-chip/connectedhomeip/issues?