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.

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

Improve documentation and member names for ChipZclRawBuffer_t. #857

Merged
merged 1 commit into from
May 27, 2020
Merged

Improve documentation and member names for ChipZclRawBuffer_t. #857

merged 1 commit into from
May 27, 2020

Conversation

bzbarsky-apple
Copy link
Contributor

Also fixes a memory leak in chipZclProcessIncoming that became more obvious
with the new documentation.

Problem

It wasn't immediately clear to me what some of the ChipZclRawBuffer_t APIs did.

Summary of Changes

Documentation improvements, memory leak fix.

fixes #856

Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

Can we doxygen headers?

@bzbarsky-apple
Copy link
Contributor Author

Can we doxygen headers?

We can, and this header already uses comments in the general format doxygen recognizes. @woody-apple are you just looking for @param bits in function documentation, or something else?

Copy link
Contributor

@bhaskar-apple bhaskar-apple left a comment

Choose a reason for hiding this comment

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

LGTM

Also fixes a memory leak in chipZclProcessIncoming that became more obvious
with the new documentation.
@woody-apple woody-apple self-requested a review May 27, 2020 18:32
@woody-apple
Copy link
Contributor

@gerickson @jelderton ?

@bzbarsky-apple
Copy link
Contributor Author

@turon The test failure is:

Failed assert: Tdelta < (Tdelay + margin) in ../../../../../src/platform/tests/TestPlatformTime.cpp:116

That doesn't seem related to these changes. Is that test known to intermittently fail?

@woody-apple
Copy link
Contributor

@turon The test failure is:

Failed assert: Tdelta < (Tdelay + margin) in ../../../../../src/platform/tests/TestPlatformTime.cpp:116

That doesn't seem related to these changes. Is that test known to intermittently fail?

#861

@woody-apple woody-apple merged commit 9f4e81b into project-chip:master May 27, 2020
@gerickson
Copy link
Contributor

Thanks for doing this. The only additional request for a future pass would be decorating the @param declarations with [in], [out], or [inout] to document their directionality. Clearly, if they aren't pointers or references they must by definition be [in]; however, I think it'd be good for our Doxygen to be consistent across the board on that decoration.

@bzbarsky-apple bzbarsky-apple deleted the zcl-buffer-better-docs branch May 27, 2020 20:11
@@ -97,5 +105,8 @@ ChipZclStatus_t chipZclProcessIncoming(uint8_t * rawBuffer, uint16_t rawBufferLe
chipZclDecodeZclHeader(buffer, context);
chipZclClusterCommandParse(context);

chipZclRawFree(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for these to be allocated instead of just placed on the stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good question; I mostly kept the existing setup there, but I agree that in general it would make a lot of sense to me to put the context and the buffer on the stack...

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed in #898

@bzbarsky-apple
Copy link
Contributor Author

The only additional request for a future pass would be decorating the @param declarations with [in], [out], or [inout]

#897 does that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve documentation for ChipZclRawBuffer_t
7 participants