-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ReadClient and ReaderHandler and interactionModelDelegate #5198
Add ReadClient and ReaderHandler and interactionModelDelegate #5198
Conversation
Add status report support and correspond handler in ReadHandler, particular for situation when thereconnectedhomeip/src/app/ReadClient.cpp Lines 233 to 243 in 5d72c9e
This comment was generated by todo based on a
|
5d72c9e
to
2463cd5
Compare
2463cd5
to
a8efb0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about unit tests for this code?
a8efb0f
to
b8daefd
Compare
Done, thanks |
5e20828
to
476df57
Compare
1054770
to
3e2dc7c
Compare
Fix the below check after the above status report is implemented.connectedhomeip/src/app/InteractionModelEngine.cpp Lines 141 to 151 in 3e2dc7c
This comment was generated by todo based on a
|
46470ea
to
3a9ced4
Compare
Once status report support is added, we would shutdown only when there is errorconnectedhomeip/src/app/ReadHandler.cpp Lines 96 to 106 in 3a9ced4
This comment was generated by todo based on a
|
3a9ced4
to
eea899c
Compare
Once status report support is added, we would shutdown only when there is error or when this is last chunk of reportconnectedhomeip/src/app/ReadHandler.cpp Lines 96 to 106 in eea899c
This comment was generated by todo based on a
|
70c9662
to
5d4e632
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I think this hangs together now, but I've been thinking about this and I have two architectural concerns:
- 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.
- 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.
5d4e632
to
3991b7f
Compare
3991b7f
to
5e33b73
Compare
|
5e33b73
to
66be4f4
Compare
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.
66be4f4
to
84001c7
Compare
{ | ||
Uninitialized = 0, //< The handler has not been initialized | ||
Initialized, //< The handler has been initialized and is ready | ||
Reportable, //< The handler has received read request and is waiting for the data to send to be available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
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