Add ReadClient and ReaderHandler and interactionModelDelegate

[yunhanw-google]

Problem:
Need read client to send IM read request and process report and read handler to process
read request, need interaction model delegate to notify zcl the actions
in corresponse for particular IM callback.

Summary of Changes:
-- Add read client to send IM read request and process report.
-- Add read handler to process read request
-- Add interaction model delgate to notify when each IM event stream is received and
all reports have been received and so on.

Note: This is PR splited from large PR #4800

[todo]

Add status report support and correspond handler in ReadHandler, particular for situation when there

connectedhomeip/src/app/ReadClient.cpp

Lines 233 to 243 in 5d72c9e

	// TODO: Add status report support and correspond handler in ReadHandler, particular for situation when there
	// are multiple reports
	}
	if (nullptr != mpDelegate && !moreChunkedMessages)
	{
	inParam.Clear();
	outParam.Clear();
	mpDelegate->HandleIMCallBack(chip::app::InteractionModelDelegate::CallbackId::kReportProcessed, inParam, outParam);
	}
	exit:

---

This comment was generated by todo based on a TODO comment in 5d72c9e in #5198. cc @yunhanw-google.

[woody-apple]

@vivien-apple ?

[mspang]

What about unit tests for this code?

[yunhanw-google]

What about unit tests for this code?

Done, thanks

[todo]

Fix the below check after the above status report is implemented.

connectedhomeip/src/app/InteractionModelEngine.cpp

Lines 141 to 151 in 3e2dc7c

	// Todo: Fix the below check after the above status report is implemented.
	if (nullptr != apExchangeContext)
	{
	apExchangeContext->Abort();
	}
	}
	void InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
	const PacketHeader & aPacketHeader, const PayloadHeader & aPayloadHeader,
	System::PacketBufferHandle aPayload)
	{

---

This comment was generated by todo based on a Todo comment in 3e2dc7c in #5198. cc @yunhanw-google.

[todo]

Once status report support is added, we would shutdown only when there is error

connectedhomeip/src/app/ReadHandler.cpp

Lines 96 to 106 in 3a9ced4

	//Todo: Once status report support is added, we would shutdown only when there is error
	exit:
	Shutdown();
	return err;
	}
	CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle aPayload)
	{
	CHIP_ERROR err = CHIP_NO_ERROR;
	System::PacketBufferTLVReader reader;

---

This comment was generated by todo based on a Todo comment in 3a9ced4 in #5198. cc @yunhanw-google.

[todo]

Once status report support is added, we would shutdown only when there is error or when this is last chunk of report

connectedhomeip/src/app/ReadHandler.cpp

Lines 96 to 106 in eea899c

	//Todo: Once status report support is added, we would shutdown only when there is error or when this is last chunk of report
	exit:
	Shutdown();
	return err;
	}
	CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle aPayload)
	{
	CHIP_ERROR err = CHIP_NO_ERROR;
	System::PacketBufferTLVReader reader;

---

This comment was generated by todo based on a Todo comment in eea899c in #5198. cc @yunhanw-google.

[bzbarsky-apple]

OK. I think this hangs together now, but I've been thinking about this and I have two architectural concerns:

1. I question the desirability of a reusable ReadClient and the attendant lifetime-management complexity being foisted on the SDK consumer. There are various things we could do to make this better, and some of them can probably be done on top of this PR, so I'm not going to block it, but I think we should very seriously think about something that would be both safer and easier to use.

For example, we could have a ReadClientHandle that allows move-construction and move-assignment only, except from inside the IM engine. The handle would point to a client and vice versa, with destructors cleaning up the references, and the move stuff on handle updating them. You could compare a handle to a client pointer, and you could shut the client down via the handle, but that's all you could do with a handle. When the client is done, it would shut itself down and null out the handle. This would guarantee that (a) you don't have to worry about shutting down the read client unless you want to terminate an in-progress read and (b) you can't mess up and shut down a different read from the one you meant to shut down because you kept a pointer to something around past a certain point. Please strongly consider filing a followup bug on this.

2. I am very worried about exposing the raw TLV to SDK consumers. The TLV reader API is not particularly easy to use, unfortunately. Again, I guess we can do this for the moment, but it seems to me that we're eventually going to want a nicer API. What the size cost of that would be is a hard question, but I would really like something safer to use than "here, read some tlv; good luck", especially when the rules around reading TLV are not well-internalized even by CHIP developers and are not well-described in the spec.

[yunhanw-google]

#5446
#5447

OK. I think this hangs together now, but I've been thinking about this and I have two architectural concerns:

1. I question the desirability of a reusable ReadClient and the attendant lifetime-management complexity being foisted on the SDK consumer. There are various things we could do to make this better, and some of them can probably be done on top of this PR, so I'm not going to block it, but I think we should very seriously think about something that would be both safer and easier to use.

For example, we could have a ReadClientHandle that allows move-construction and move-assignment only, except from inside the IM engine. The handle would point to a client and vice versa, with destructors cleaning up the references, and the move stuff on handle updating them. You could compare a handle to a client pointer, and you could shut the client down via the handle, but that's all you could do with a handle. When the client is done, it would shut itself down and null out the handle. This would guarantee that (a) you don't have to worry about shutting down the read client unless you want to terminate an in-progress read and (b) you can't mess up and shut down a different read from the one you meant to shut down because you kept a pointer to something around past a certain point. Please strongly consider filing a followup bug on this.

1. I am very worried about exposing the raw TLV to SDK consumers. The TLV reader API is not particularly easy to use, unfortunately. Again, I guess we can do this for the moment, but it seems to me that we're eventually going to want a nicer API. What the size cost of that would be is a hard question, but I would really like something safer to use than "here, read some tlv; good luck", especially when the rules around reading TLV are not well-internalized even by CHIP developers and are not well-described in the spec.

[andy31415]

Where is this used?