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.43k stars 1.99k forks source link

PayloadHeader does not have a single source of truth for "is this an ack?" state #4083

Open bzbarsky-apple opened 3 years ago

bzbarsky-apple commented 3 years ago

Problem

PayloadHeader has two different ways to represent whether it's an ack: the kExchangeFlag_AckMsg flag in mExchangeFlags and mAckId.HasValue(). This is confusing some people who think they need to change both places to be sure they have an ack id.

PayloadHeader::Decode ensures the two are in sync, and the SetAckId setters ensure they stay in sync, but it might be better if there were only one source of truth.

Proposed Solution

Have a single source of truth, probably mAckId.HasValue(). Encode could then reconstitute the flag byte based on that, and we wouldn't need the synchronization code in the SetAckId setters.

I'm a little torn on whether to leave the IsAckMsg API, since that might still confuse people, but probably best to leave it for now.

@andy31415 @pan-apple

woody-apple commented 2 years ago

SW Dev Bug Review: Closing this, as doesn't appear needed anymore and/or is complete, please reopen if needed.

bzbarsky-apple commented 2 years ago

This is still an issue, though mAckId is now named mAckMessageCounter.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.