Invoking commands using JSON is painful and error prone (CON-1194)

[jonsmirl]

@wqx6

Converting the request handle data to JSON and then having send_request() convert that JSON to TLV is just painful to do. You are going to have to build custom code for every parameter list. https://github.com/espressif/esp-matter/blob/main/examples/light_switch/main/app_driver.cpp#L211

Isn't the requestData on request_handle_t an EncodableType? So something like my old code should be possible... It doesn't work anymore because Controller has been made internal. Old code.... I got this to compile again with "#include <controller/InvokeInteraction.h>"

    else if (cmd_handle->command_path.mClusterId == LevelControl::Id)
    {
        switch (cmd_handle->command_path.mCommandId)
        {
        case LevelControl::Commands::MoveWithOnOff::Id:
        {
            // level_control::command::group_send_move_with_on_off(fabric_index, cmd_handle->group_id);
            Controller::InvokeGroupCommandRequest(&exchangeMgr, fabric_index, cmd_handle->command_path.mGroupId,
                                                  *((LevelControl::Commands::MoveWithOnOff::Type *)cmd_handle->request_data));
            break;
        };
        case LevelControl::Commands::MoveToLevelWithOnOff::Id:
        {
            // level_control::command::group_send_move_with_on_off(fabric_index, cmd_handle->group_id);
            Controller::InvokeGroupCommandRequest(&exchangeMgr, fabric_index, cmd_handle->command_path.mGroupId,
                                                  *((LevelControl::Commands::MoveToLevelWithOnOff::Type *)cmd_handle->request_data));
            break;
        };
        case LevelControl::Commands::StopWithOnOff::Id:
        {
            // level_control::command::group_send_stop_with_on_off(fabric_index, cmd_handle->group_id);
            Controller::InvokeGroupCommandRequest(&exchangeMgr, fabric_index, cmd_handle->command_path.mGroupId,
                                                  *((LevelControl::Commands::StopWithOnOff::Type *)cmd_handle->request_data));
            break;
        };
        }
    }

It would be much easier to directly construct the TLV in light_switch/main/app_driver.cpp than to use JSON as an intermediary. You might want to leave the JSON version and add a TLV version.

The new code would look something like this

    else if (cmd_handle->command_path.mClusterId == LevelControl::Id)
    {
        switch (cmd_handle->command_path.mCommandId)
        {
        case LevelControl::Commands::MoveWithOnOff::Id:
        {
            // level_control::command::group_send_move_with_on_off(fabric_index, cmd_handle->group_id);
            send_request(fabric_index, cmd_handle->command_path,
                                                  ((LevelControl::Commands::MoveWithOnOff::Type *)cmd_handle->request_data))->Encode());
            break;
        };
        case LevelControl::Commands::MoveToLevelWithOnOff::Id:
        {
            // level_control::command::group_send_move_with_on_off(fabric_index, cmd_handle->group_id);
            send_requestt(fabric_index, cmd_handle->command_path,
                                                  (((LevelControl::Commands::MoveToLevelWithOnOff::Type *)cmd_handle->request_data))->Encode());
            break;
        };
        case LevelControl::Commands::StopWithOnOff::Id:
        {
            // level_control::command::group_send_stop_with_on_off(fabric_index, cmd_handle->group_id);
            send_request(fabric_index, cmd_handle->command_path,
                                                  (((LevelControl::Commands::StopWithOnOff::Type *)cmd_handle->request_data))->Encode());
            break;
        };
        }
    }

Something like that since this stuff is templated instead of using virtual methods.

[wqx6]

The request_data field in request handle doesn't need to be a JSON string. We just provide a API that could accept a JSON string input and use that API in our client examples. This API could be used for any clusters so that it can avoid so many if-elses or switch-cases in our code. And that API could also be used when CONFIG_ENABLE_CHIP_CONTROLLER_BUILD is not set. If you want to call the APIs which uses the Encoded types from Matter in controller/InvokeInteraction.h you can just set the Encodes types as the request data in request handle and do the converting in your command callback.

[jonsmirl]

I agree that I don't like all of the if-else/switches in the command callbacks. There is something wrong with the CHIP architecture which is causing this. I think the design problem is that you should TLV encode the command parameters before the command is submitted instead of after. Then in the command callback the data would already be encoded and there would be no need for the if/else/switch. request_data would be a pointer to the TLV instead of the command structure. This is along similar lines to what you are doing with JSON.

Of course this change would have to be made in the CHIP core.

        LevelControl::Commands::MoveToLevelWithOnOff::Type commandDataT2;
        commandDataT2.level = mBrightness;
        commandDataT2.transitionTime.SetNonNull(0);
        commandDataT2.optionsMask = static_cast<chip::BitMask<chip::app::Clusters::LevelControl::LevelControlOptions>>(0U);
        commandDataT2.optionsOverride = static_cast<chip::BitMask<chip::app::Clusters::LevelControl::LevelControlOptions>>(0U);

        client::request_handle_t cmd_handle2;
        cmd_handle2.command_path.mClusterId = LevelControl::Id;
        cmd_handle2.command_path.mCommandId = LevelControl::Commands::MoveToLevelWithOnOff::Id;

        cmd_handle2.request_data = Encode(&commandDataT2;)
Do the encode here instead of in the command callback  ^^^^^^ 

        // Release moves Position from 1 (press) to 0 (idle)
        lock::chip_stack_lock(portMAX_DELAY);
        client::cluster_update(epLightID, &cmd_handle2);
        lock::chip_stack_unlock();

[jonsmirl]

@wqx6

This is getting fixed inside CHIP and you won't need the JSON. There is a new API, https://github.com/project-chip/connectedhomeip/blob/master/src/app/CommandSender.h#L379-L393

You'll need to do some adjustments to use this, but this allows the parameters to be encoded BEFORE submitting the command. Now when the command ends up in the callback you don't need the giant if/else/switch since you will be able to directly Invoke using the EncodableToTLV thus avoiding all of the templated Invoke commands.

[wqx6]

Thanks! I will update the API to use the virtual class when we update the upstream repo.

[chshu]

@jonsmirl could you please check if https://github.com/espressif/esp-matter/commit/4d617ed2de5a2fbf0c44f03edb2056494c36d0a1 fixed the issue?