Skip to content
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.

Sign up for GitHub

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

Clearly document lifetime expectations for CommandSender. #7067

Merged
merged 1 commit into from
May 25, 2021
Merged

Clearly document lifetime expectations for CommandSender. #7067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/app/CommandSender.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,22 @@ class CommandSender : public Command, public Messaging::ExchangeDelegate
{
public:
// TODO: issue #6792 - the secure session parameter should be made non-optional and passed by reference.
// Once SendCommandRequest returns successfully, the CommandSender will
// handle calling Shutdown on itself once it decides it's done with waiting
// for a response (i.e. times out or gets a response).
//
// If SendCommandRequest is never called, or the call fails, the API
// consumer is responsible for calling Shutdown on the CommandSender.
CHIP_ERROR SendCommandRequest(NodeId aNodeId, Transport::AdminId aAdminId, SecureSessionHandle * secureSession = nullptr);

private:
// ExchangeDelegate interface implementation. Private so people won't
// accidentally call it on us when we're not being treated as an actual
// ExchangeDelegate.
void OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override;
void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;

private:
CHIP_ERROR ProcessCommandDataElement(CommandDataElement::Parser & aCommandElement) override;
};

Expand Down
2 changes: 1 addition & 1 deletion src/app/InteractionModelEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate

/**
* Retrieve a CommandSender that the SDK consumer can use to send a set of commands. If the call succeeds,
* the consumer is responsible for calling Shutdown() on the CommandSender once it's done using it.
* see CommandSender documentation for lifetime handling.
Copy link
Contributor

@andy31415 andy31415 May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: redirecting from a doc to another doc is generally somewhat inconvenient for reads of code. Don't have a better suggestion though beside some more invasive changes that would not require documentation to figure out lifetime (smart-pointer/handle usage).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was not too happy about this either, not least because it's not pointing directly at the relevant docs over there, but if I did that I'd be pointing to a function name that can also change... I couldn't find a nicer solution just with documentation, unfortunately. :(

*
* @param[out] apCommandSender A pointer to the CommandSender object.
*
Expand Down