search

project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.55k stars 2.04k forks source link

Follow up for PR https://github.com/project-chip/connectedhomeip/pull/29364 #36526

Open nivi-apple opened 1 week ago

nivi-apple commented 1 week ago

see - https://github.com/project-chip/connectedhomeip/pull/29364/files#r1826271763

nivi-apple commented 1 week ago

also address add tests with multiple controllers

https://github.com/project-chip/connectedhomeip/pull/29364/files#r1826432261

nivi-apple commented 1 week ago

Followup: instead of having logic in the caller that has to match this logic in terms of which types of messages expect responses, we should have either sendFlags or just the boolean of whether we set kExpectResponse be an outparam of this method. But please don't make that change in this PR; do it separately.

https://github.com/project-chip/connectedhomeip/pull/29364/files#r1826256649

nivi-apple commented 1 week ago

So... this is not responding with a BUSY status on the exchange or anything. It's going to just drop everything on the floor. Is this actually the behavior we want? And if it is, why do we have the higher-level "busy" handling above that sends nice cluster response commands? This seems pretty broken to me....

Member Author nivi-apple

hmm let me figure out how to pipe the busy correctly. the tests pass where we test busy so either the tests don't test the right thing or something is sending the right response but cleaning up the class which it shouldn't.

Member Author nivi-apple

do a follow up. create multiple handlers.

Member Author nivi-apple

36526. combined in the follow up issue

https://github.com/project-chip/connectedhomeip/pull/29364/files#r1826433255

nivi-apple commented 1 week ago

Fix So... this part is wrong. If mOTAImageTransferHandler is already not null, we will clobber the pointer and then things will go very badly wrong on shutdown. The fact that we apparently have no tests exercising this case is very bad and we need to fix that.

We need to do one of two things:

Actually support multiple instances of MTROTAImageTransferHandler being live, and then we can create a new one here, or: Have a check for IsInAnOngoingTransfer() and fail out if it's true here.

Note that this is for the "we got an unsolicited BDX message" case. The reason our tests work is that we do respond BUSY from the QueryImage command if we are in a transfer, which would be before the other side tries to start BDX, so in "normal" operation we never reach this code with IsInAnOngoingTransfer() true.

So long story short, for now

Put a IsInAnOngoingTransfer() check at the top of this method and error out if it's true. Do not set mOTAImageTransferHandler here. It gets set in OnTransferHandlerCreated. As a followup, we start supporting multiple MTROTAImageTransferHandler being live.