Initial implementation of PushAV Stream Transport Cluster #37787

sayondeep · 2025-02-26T11:34:03Z

This is the initial implementation of Push AV Stream Transport cluster

Delegate definition
Attributes support
Command Handling

Testing

Addition of delegate implementation in the example app is pending.

semanticdiff-com · 2025-02-26T11:34:05Z

Review changes with

Changed Files

File	Status
src/app/zap_cluster_list.json	75% smaller
src/app/chip_data_model.gni	Unsupported file format
src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp	Unsupported file format
src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h	Unsupported file format
src/app/common/templates/config-data.yaml	0% smaller
zzz_generated/app-common/app-common/zap-generated/callback.h	Unsupported file format

CLAassistant · 2025-02-26T11:34:13Z

All committers have signed the CLA.

github-actions · 2025-02-27T09:32:23Z

PR #37787: Size comparison from 73fe30c to 543b7e1

Full report (3 builds for cc32xx, stm32)

platform	target	config	section	`73fe30c`	`543b7e1`
cc32xx	air-purifier	CC3235SF_LAUNCHXL	FLASH	540646	540646
			RAM	205128	205128
	lock	CC3235SF_LAUNCHXL	FLASH	574794	574794
			RAM	205376	205376
stm32	light	STM32WB5MM-DK	FLASH	459840	459840
			RAM	141472	141472

src/app/chip_data_model.gni

yufengwangca · 2025-02-27T17:56:29Z

you need to enable this cluster in camera example app to include it into the build system

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

yufengwangca · 2025-02-27T18:14:37Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+     *   produced; otherwise, the command SHALL be rejected with an appropriate
+     *   error.
+     */
+    virtual Protocols::InteractionModel::Status AllocatePushTransport(uint16_t & connectionID,


Protocols::InteractionModel::Status is SDK internal, suggest to return general CHIP_ERROR in application code, same for other APIs bellow.

Protocols::InteractionModel::Status is no more internal than CHIP_ERROR. It's fine for delegates to return it.

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

yufengwangca · 2025-02-27T18:20:36Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+    friend class PushAvStreamTransportServer;
+
+    PushAvStreamTransportServer * mPushAvStreamTransportServer = nullptr;
+
+    /**
+     * This method is used by the SDK to set the PushAvStreamTransportServer pointer member in the delegate.
+     * This is done in the constructor during the instantiation of the PushAvStreamTransportServer object.
+     *
+     * @param aPushAvStreamTransportServer A pointer to the PushAvStreamTransportServer object related to this delegate object.
+     */
+    void SetPushAvStreamTransportServer(PushAvStreamTransportServer * aPushAvStreamTransportServer)
+    {
+        mPushAvStreamTransportServer = aPushAvStreamTransportServer;
+    }


In other cluster implementations, the cluster server is instantiated directly from the application rather than through its delegate. This approach avoids circular dependencies, since the cluster server itself contains the delegate.

yufengwangca · 2025-02-27T18:21:23Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+    }
+
+protected:
+    PushAvStreamTransportServer * GetPushAvStreamTransportServer() const { return mPushAvStreamTransportServer; }


The app should instantiate and hold the Server instance by itself

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

yufengwangca · 2025-02-27T18:25:48Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+    AttributeAccessInterface(MakeOptional(aEndpointId), CameraAvStreamManagement::Id), mDelegate(aDelegate),
+    mEndpointId(aEndpointId), mFeature(aFeature)
+{
+    mDelegate.SetPushAvStreamTransportServer(this);


This create circular dependency, we should have PushAvStreamTransportServer contains the reference to PushAvStreamTransportDelegate, not the other way around

Seems fine for both to refer to each other... Why is this a problem?

yufengwangca · 2025-02-27T18:27:27Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+    // Explicitly set the PushAvStreamTransportServer pointer in the Delegate to null.
+
+    mDelegate.SetPushAvStreamTransportServer(nullptr);
+
+    // Unregister command handler and attribute access interfaces
+    CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this);
+    AttributeAccessInterfaceRegistry::Instance().Unregister(this);


mDelegate.SetPushAvStreamTransportServer(nullptr); is not needed, and move others to Shutdown API and call Shutdown here

mDelegate.SetPushAvStreamTransportServer(nullptr); is not needed

It's absolutely needed to avoid a dangling pointer, if the delegate holds a pointer to this object,

yufengwangca · 2025-02-27T20:14:20Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+        ReturnErrorOnFailure(aEncoder.Encode(mFeature));
+        break;
+
+    case SupportedContainerFormats::Id:


Why do we need to intercept and override the default attributes in the attribute store? We only need to modify these default fields if we require the values to come from the app, since the default storage doesn't meet our requirements. However, it appears that these attributes aren't being handled specially.

yufengwangca · 2025-02-27T20:15:59Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+private:
+    PushAvStreamTransportDelegate & mDelegate;
+    EndpointId mEndpointId;
+    const BitFlags<Feature> mFeature;


Is the default storage provided by the attribute store insufficient? Why must we maintain these attributes within the cluster server itself?

bzbarsky-apple · 2025-02-28T01:15:26Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+     *   produced; otherwise, the command SHALL be rejected with an appropriate
+     *   error.
+     */
+    virtual Protocols::InteractionModel::Status AllocatePushTransport(uint16_t & connectionID,


Protocols::InteractionModel::Status is no more internal than CHIP_ERROR. It's fine for delegates to return it.

bzbarsky-apple · 2025-02-28T01:18:08Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+     */
+    virtual Protocols::InteractionModel::Status
+    FindTransport(const Optional<DataModel::Nullable<uint16_t>> & connectionID,
+                  DataModel::List<TransportConfigurationStruct> & outtransportConfigurations) = 0;


What is the lifetime of the memory the List points to? How is that lifetime supposed to be managed by the callee?

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

bzbarsky-apple · 2025-02-28T01:19:15Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.h

+    // Helper functions
+    bool FindStreamTransportConnection(const uint16_t connectionID);
+    uint16_t GenerateConnectionID();


Are these meant to be public?

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

bzbarsky-apple · 2025-02-28T01:33:46Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+    uint16_t connectionID   = commandData.connectionID;
+    auto & transportOptions = commandData.transportOptions;
+
+    // Call the delegate


Need to validate the constraints first.

bzbarsky-apple · 2025-02-28T01:35:21Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+    auto & activationReason = commandData.activationReason;
+    auto & timeControl      = commandData.timeControl;
+
+    // Call the delegate


Needs constraint validation.

bzbarsky-apple · 2025-02-28T01:35:45Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+    auto & activationReason = commandData.activationReason;
+    auto & timeControl      = commandData.timeControl;
+
+    // Call the delegate


Needs connection id validation per spec.

bzbarsky-apple · 2025-02-28T01:36:26Z

src/app/clusters/push-av-stream-transport-server/push-av-stream-transport-server.cpp

+
+    DataModel::List<TransportConfigurationStruct> outStreamConfigurations;
+
+    // Call the delegate


Needs spec-required validation for a non-null connection id.

github-actions bot added the app label Feb 26, 2025

sayondeep force-pushed the pr/camera/pushav branch 3 times, most recently from 48ff8e0 to 9d1591f Compare February 27, 2025 09:00

sayondeep changed the title ~~initial implementation of pushav server~~ Initial implementation of PushAV Stream Transport Cluster Feb 27, 2025

initial implementation of pushav server

79c2dbd

sayondeep force-pushed the pr/camera/pushav branch from 9d1591f to 79c2dbd Compare February 27, 2025 09:03

Restyled by prettier-json

beda7c9

sayondeep marked this pull request as ready for review February 27, 2025 09:20

pullapprove bot added the review - pending label Feb 27, 2025