Feature/enhanced setup flow feature #34065
Open
+1,464
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harimau-qirex,
harsha-rajendran and
hawk248
swan-amazon
force-pushed
the
feature/enhanced-setup-flow-feature
branch
from
66ddf83
to
d9e1799
Compare
|
PR #34065: Size comparison from 1000d7e to d9e1799 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Oct 17, 2024
This commit introduces the initial logic for handling Terms and Conditions (TC) acknowledgements in the General Commissioning cluster. The logic includes support for setting and checking TC acknowledgements and versions during the commissioning process. Changes include: - Handling TC acknowledgements and TC acknowledgement version in the pairing command. - Logic to read TC attributes and check TC acceptance in the General Commissioning server. - Introduction of classes to manage TC acceptance logic. - Initialization and use of TC providers in the server setup. - Addition of a new commissioning stage for TC acknowledgements in the commissioning flow. The feature logic is currently disabled and will be enabled in an example in a subsequent commit.
swan-amazon
force-pushed
the
feature/enhanced-setup-flow-feature
branch
from
d9e1799
to
a7ea749
Compare
|
PR #34065: Size comparison from 5fe4409 to a7ea749 Full report (79 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
125
to
135
| if (nullptr == termsAndConditionsProvider) | ||
| { | ||
| return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; | ||
| } | ||
|
|
||
| err = termsAndConditionsProvider->GetRequirements(outTermsAndConditions); | ||
| if (CHIP_NO_ERROR != err) | ||
| { | ||
| return CHIP_ERROR_INTERNAL; | ||
| } | ||
|
|
There was a problem hiding this comment.
For these returns on conditions not met or err != CHIP_NO_ERROR, we use VerifyOrReturnError(cond, error) and ReturnErrorOnFailure(err)
It would make the overall vertical space in here much tighter.
Comment on lines
121
to
137
| TermsAndConditionsProvider * const termsAndConditionsProvider = Server::GetInstance().GetTermsAndConditionsProvider(); | ||
| Optional<TermsAndConditions> outTermsAndConditions; | ||
| CHIP_ERROR err; | ||
|
|
||
| if (nullptr == termsAndConditionsProvider) | ||
| { | ||
| return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; | ||
| } | ||
|
|
||
| err = termsAndConditionsProvider->GetRequirements(outTermsAndConditions); | ||
| if (CHIP_NO_ERROR != err) | ||
| { | ||
| return CHIP_ERROR_INTERNAL; | ||
| } | ||
|
|
||
| return !outTermsAndConditions.HasValue() ? aEncoder.Encode(static_cast<uint16_t>(0)) | ||
| : aEncoder.Encode(outTermsAndConditions.Value().version); |
There was a problem hiding this comment.
Example:
Suggested change
| TermsAndConditionsProvider * const termsAndConditionsProvider = Server::GetInstance().GetTermsAndConditionsProvider(); | |
| Optional<TermsAndConditions> outTermsAndConditions; | |
| CHIP_ERROR err; | |
| if (nullptr == termsAndConditionsProvider) | |
| { | |
| return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; | |
| } | |
| err = termsAndConditionsProvider->GetRequirements(outTermsAndConditions); | |
| if (CHIP_NO_ERROR != err) | |
| { | |
| return CHIP_ERROR_INTERNAL; | |
| } | |
| return !outTermsAndConditions.HasValue() ? aEncoder.Encode(static_cast<uint16_t>(0)) | |
| : aEncoder.Encode(outTermsAndConditions.Value().version); | |
| TermsAndConditionsProvider * const termsAndConditionsProvider = Server::GetInstance().GetTermsAndConditionsProvider(); | |
| VerifyOrReturnError(nullptr != termsAndConditionsProvider, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE); | |
| Optional<TermsAndConditions> outTermsAndConditions; | |
| ReturnErrorOnFailure(termsAndConditionsProvider->GetRequirements(outTermsAndConditions)); | |
| return !outTermsAndConditions.HasValue() ? aEncoder.Encode(static_cast<uint16_t>(0)) | |
| : aEncoder.Encode(outTermsAndConditions.Value().version); |
The re-writing of errors to CHIP_ERROR_INTERNAL is frowned-upon because the underlying error under the hood will be lost to upstream logging, and furthermore, so errors properly lead to different IM StatusCode for some cases.
src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
Outdated
Show resolved
Hide resolved
beckerzito
approved these changes
Oct 22, 2024
| } | ||
| else | ||
| { | ||
| CheckSuccess(termsAndConditionsProvider->CommitAcceptance(), Failure); |
There was a problem hiding this comment.
Why is this allowed to be called (and immediately committed) outside failsafe?
There was a problem hiding this comment.
Outside of the failsafe context, I believe we should be able to accept updated terms and conditions. I thought the intention of failsafe context was to establish a failsafe context where you could change things and they are not committed until the failsafe is closed or commissioning complete. I don't believe we made any statements that this command may only be received while in the failsafe context.
There was a problem hiding this comment.
Every command of General Commissioning cluster can only be handled during fail-same. But overall, it's odd whenever there is any state that can be reverted by fail-safe under some situations, but not others.
src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+29
to
+36
| #define VerifyNoErrorOrReturnValue(expr, code, ...) VerifyOrReturnValue(CHIP_NO_ERROR == expr, code, ##__VA_ARGS__) | ||
| #define VerifyNoErrorOrReturnInternal(expr, ...) VerifyNoErrorOrReturnValue(expr, CHIP_ERROR_INTERNAL, ##__VA_ARGS__) | ||
| #define VerifyOrReturnInvalidArgument(expr, ...) VerifyOrReturnValue(expr, CHIP_ERROR_INVALID_ARGUMENT, ##__VA_ARGS__) | ||
| #define VerifyOrReturnUninitialized(expr, ...) VerifyOrReturnValue(expr, CHIP_ERROR_UNINITIALIZED, ##__VA_ARGS__) | ||
| #define VerifyOrReturnInternal(expr, ...) VerifyOrReturnValue(expr, CHIP_ERROR_INTERNAL, ##__VA_ARGS__) | ||
| #define VerifyNotNullOrReturnInvalidArgument(expr, ...) \ | ||
| VerifyOrReturnValue(nullptr != expr, CHIP_ERROR_INVALID_ARGUMENT, ##__VA_ARGS__) | ||
| #define VerifyNotNullOrReturnUninitialized(expr, ...) VerifyOrReturnValue(nullptr != expr, CHIP_ERROR_UNINITIALIZED, ##__VA_ARGS__) |
There was a problem hiding this comment.
Why all these different ones rather than following the usual patterns?
| 8 * sizeof(uint16_t); // Extra space for rollback compatibility | ||
| } // namespace | ||
|
|
||
| CHIP_ERROR chip::app::DefaultTermsAndConditionsStorageDelegate::Init(PersistentStorageDelegate * const inPersistentStorageDelegate) |
There was a problem hiding this comment.
the * const here and many other places is superfluous.
There was a problem hiding this comment.
I had ChatGPT write this up... I can remove them, but they do provide some purpose, but if the codebase avoids it, I can remove them.
The value of marking a pointer as const to prevent reassignment is mostly contextual and subjective, depending on factors like coding standards, project size, and developer preferences. Let's explore its potential value in terms of code safety and clarity:
Value of the const Safeguard
-
Preventing Accidental Reassignment:
- If your function doesn’t intend to reassign the pointer, marking it
constcan prevent potential errors or bugs. While it’s uncommon to accidentally reassign a pointer, this safeguard ensures that such mistakes (whether by you or others maintaining the code) can’t happen. - Example: If the function signature makes it clear that the pointer shouldn’t change, the
constmakes it explicit, and the compiler will catch any unintentional reassignment attempts. In large codebases, or when refactoring, this can help avoid subtle issues.
- If your function doesn’t intend to reassign the pointer, marking it
-
Intentionality and Code Self-Documentation:
- Using
constclarifies the function’s contract. It signals to anyone reading or maintaining the code that the function will not modify the pointer itself, reinforcing the idea that the pointer is only used to access the object it points to, and its address is fixed inside the function. - Code readability: Some developers find that adding
constimproves readability by making the contract of the function more explicit: "This function guarantees it won’t reassign the pointer."
- Using
-
Consistency Across a Codebase:
- In projects where
constcorrectness is emphasized, consistently applyingconstto pointers and references can enforce a mindset of immutability, leading to fewer bugs, especially in larger teams or systems with complex pointer manipulation. - Code standards: In some codebases (particularly in critical systems, embedded systems, or safety-critical applications), strict
constcorrectness is a common practice to ensure immutability where appropriate.
- In projects where
-
Compiler Optimizations:
- In some cases, marking pointers
constmight enable the compiler to perform certain optimizations, although this is more relevant when usingconston the object the pointer points to, rather than the pointer itself. Nonetheless, usingconstconsistently can lead to subtle performance improvements.
- In some cases, marking pointers
Is the Safeguard Always Valuable?
While these benefits exist, the actual value of marking the pointer const in a specific function can vary:
-
For small functions or simple codebases, this safeguard might not be crucial. It might even be seen as unnecessary verbosity if it’s obvious from the function’s implementation that the pointer won’t be reassigned.
-
For large or complex codebases where multiple developers interact with the code, or where functions are long and intricate, the
constqualifier can be more valuable as it contributes to readability and avoids unintended side effects.
Code Clarity in Practice
- Minimal clarity improvement: In simple functions, marking the pointer as
constmay provide minimal clarity because it’s already clear from context that the pointer won't be reassigned. - Enhanced clarity in larger functions: In larger or more complex functions, where the pointer is used in multiple places, adding
constcan act as a reminder that the pointer shouldn’t be modified.
Conclusion: How Valuable Is It?
- In smaller or less critical functions: The value might be negligible, as it doesn’t fundamentally change behavior.
- In larger or more complex functions, or in codebases that emphasize immutability and safety: It adds clarity and helps prevent potential bugs. In such cases, the
constsafeguard can be useful, even if only to communicate intent clearly to other developers.
Whether this safeguard adds value depends on your team's coding practices and the complexity of your system. If you prioritize strict immutability and preventing subtle bugs, using const is beneficial. If simplicity and brevity are more important in your specific context, it may seem superfluous.
There was a problem hiding this comment.
Whether this safeguard adds value depends on your team's coding practices and the complexity of your system
const int * == int const * --> this the common understanding in practice of single-const either side. Our codebase strongly prefers the const at the start.
| VerifyNoErrorOrReturnInternal(tlvReader.ExitContainer(tlvContainer)); | ||
|
|
||
| // Only v1 serialization format is supported | ||
| VerifyOrReturnInternal(kSerializationVersion == serializationVersion); |
There was a problem hiding this comment.
Why not tolerate v > 1? Will this clean-up as intended if this ever hits, or will the caller exit elsewhere and we will get in a messed-up place which can't go on?
Comment on lines
+273
to
+274
| ChipLogError(AppServer, "Failed storage delegate Delete(): %" CHIP_ERROR_FORMAT, err.Format()); | ||
| return CHIP_ERROR_INTERNAL; |
There was a problem hiding this comment.
Suggest nor returning an error here, as the client can't do anything with it.
| @@ -222,6 +222,8 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) | |||
|
|
|||
| mReportScheduler = initParams.reportScheduler; | |||
|
|
|||
| mTermsAndConditionsProvider = initParams.termsAndConditionsProvider; | |||
There was a problem hiding this comment.
This should be conditional T&Cs enabled, otherwise we have to pay for some provider flash space even if not used.
Comment on lines
+316
to
+328
| #if CHIP_CONFIG_TC_REQUIRED | ||
| static app::DefaultTermsAndConditionsProvider sDefaultTermsAndConditionsProviderInstance; | ||
| static app::DefaultTermsAndConditionsStorageDelegate sDefaultTermsAndConditionsStorageDelegateInstance; | ||
|
|
||
| Optional<app::TermsAndConditions> termsAndConditions = Optional<app::TermsAndConditions>((app::TermsAndConditions){ | ||
| .value = CHIP_CONFIG_TC_REQUIRED_ACKNOWLEDGEMENTS, | ||
| .version = CHIP_CONFIG_TC_REQUIRED_ACKNOWLEDGEMENTS_VERSION, | ||
| }); | ||
|
|
||
| ReturnErrorOnFailure(sDefaultTermsAndConditionsStorageDelegateInstance.Init(this->persistentStorageDelegate)); | ||
| ReturnErrorOnFailure(sDefaultTermsAndConditionsProviderInstance.Init(&sDefaultTermsAndConditionsStorageDelegateInstance, | ||
| termsAndConditions)); | ||
| this->termsAndConditionsProvider = &sDefaultTermsAndConditionsProviderInstance; |
There was a problem hiding this comment.
This means that in this case (which most products will inherit blindly) will use the CHIP_CONFIG_XXX per-firmware version. Would recommend not doing any magic init of this here, and making every user of T&C provider setup explicit init of the T&C when needed.
|
|
||
| SessionHandle handle = commandObj->GetExchangeContext()->GetSessionHandle(); | ||
|
|
||
| // Ensure it's a valid CASE session |
There was a problem hiding this comment.
Per (now updated) spec, this check happens after the terms and conditions check, so this is not matching the spec.
| if (handle->GetSessionType() != Session::SessionType::kSecure || | ||
| handle->AsSecureSession()->GetSecureSessionType() != SecureSession::Type::kCASE || | ||
| !failSafe.MatchesFabricIndex(commandObj->GetAccessingFabricIndex())) | ||
| response.errorCode = CheckTermsAndConditionsAcknowledgements(termsAndConditionsProvider); |
There was a problem hiding this comment.
So that can return one of three values: NotMet, NotReceived, NotAccepted. The spec allows NotReceived and NotMet here; it does not have any provisions for NotAccepted.
Is NotAccepted not a possible return value in this situation? If so that should probably be documented here with an explanation of why not.
If it's a possible return value, this does not match the spec.
| } | ||
| else | ||
|
|
||
| if (failSafe.UpdateTermsAndConditionsHasBeenInvoked()) |
There was a problem hiding this comment.
I don't see anything in the spec that says that terms and conditions bits are rolled back by fail-safe expiration, or indeed any interaction between fail-safe and terms and conditions bits. Where is this coming from?
There was a problem hiding this comment.
Let's fix the spec. It was the intent that there would be fail-safe handling during commissioning fail-safe, AND it's implemented. I would be OK letting it stay like this until the spec is updated, assuming it's already the assumption.
There was a problem hiding this comment.
This has been discussed offline, but to add clarification here as well this was following the recommendation here: https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/69f5bba9e65c83fec4b886ebe509f3d01f37d09f/src/service_device_management/GeneralCommissioningCluster.adoc?plain=1#L360
Comment on lines
+571
to
+572
| // Clear terms and conditions acceptance on failsafe timer expiration | ||
| termsAndConditionsProvider->RevertAcceptance(); |
There was a problem hiding this comment.
This is not in the spec. Where is this coming from?
There was a problem hiding this comment.
See my comment above.
There was a problem hiding this comment.
PR for adding this requirement: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10607
|
|
||
| VerifyNotNullOrReturnUninitialized(mStorageDelegate); | ||
|
|
||
| tlvWriter.Init(buffer, sizeof(buffer)); |
There was a problem hiding this comment.
Suggested change
| tlvWriter.Init(buffer, sizeof(buffer)); | |
| tlvWriter.Init(buffer); |
|
|
||
| uint32_t lengthWritten = tlvWriter.GetLengthWritten(); | ||
| VerifyOrReturnInternal(CanCastTo<uint16_t>(lengthWritten)); | ||
| VerifyOrReturnInternal(lengthWritten <= kEstimatedTlvBufferSize); |
There was a problem hiding this comment.
I mean, if this is false you already had a buffer overrun and smashed your stack and probably can't trust anything.... So not sure what this check is really getting you.
| * @param[in] inStorageDelegate Storage delegate dependency. | ||
| */ | ||
| CHIP_ERROR Init(TermsAndConditionsStorageDelegate * const inStorageDelegate, | ||
| const chip::Optional<chip::app::TermsAndConditions> & inRequiredTermsAndConditions); |
There was a problem hiding this comment.
Suggested change
| const chip::Optional<chip::app::TermsAndConditions> & inRequiredTermsAndConditions); | |
| const Optional<TermsAndConditions> & inRequiredTermsAndConditions); |
|
|
||
| bool setTermsAndConditionsCallRequiredBeforeCommissioningCompleteSuccess = | ||
| termsAndConditionsState != TermsAndConditionsState::OK; | ||
| return aEncoder.Encode(setTermsAndConditionsCallRequiredBeforeCommissioningCompleteSuccess); |
There was a problem hiding this comment.
I understand the value read from this attribute change according to the user's status of acceptance.
However, it seems that there is no implementation to report the value when this attribute is subscribed.
| return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; | ||
| } | ||
|
|
||
| return aEncoder.EncodeNull(); |
There was a problem hiding this comment.
Is this feature currently under development?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Integrate Terms and Conditions Acknowledgements in Commissioning Process
Fixes #34064
Add support for setting Terms and Conditions acknowledgements
acknowledgment version in the General Commissioning cluster.
acknowledgements.
Enhance setup flow handling
Conditions acknowledgements.
Conditions acknowledgements.
Handle setting Terms and Conditions acknowledgements
commissioning process.
Zap regen