[EFR32] Fix 2 door lock edge cases #20446
Merged
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
selissia
reviewed
Jul 7, 2022
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda and
mlepage-google
pullapprove
bot
requested review from
tecimovic,
tehampson,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
selissia
approved these changes
Jul 7, 2022
jmartinez-silabs
approved these changes
Jul 7, 2022
|
Only problem is there's already a different function that checks the upper bound of the credential index. |
andy31415
reviewed
Jul 7, 2022
Can the function check both lower and upper bounds? or is there a reason only one bound needs to be checked? I would merge as a first choice, but if that does not work use 'UpperBound' and 'LowerBound' suffixes or find a different name based on why we have 2 different methods. |
|
I ended up just removed the index > 0 check. This should be taken care in IsValidCredentialIndex, because the indices are unsigned. The index > 0 check should be encapsulated in |
|
PR #20446: Size comparison from 4f7d5cf to 91f9d02 Increases (5 builds for cc13x2_26x2, cyw30739, mbed, telink)
Decreases (2 builds for cc13x2_26x2, efr32)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #20446: Size comparison from 4f7d5cf to 4d5e760 Increases (5 builds for cc13x2_26x2, cyw30739, mbed, telink)
Decreases (4 builds for cc13x2_26x2, efr32, esp32)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
woody-apple
approved these changes
Jul 8, 2022
github-actions bot
pushed a commit
that referenced
this pull request
Jul 8, 2022
* Fix 2 door lock edge cases * remove check of lower bound for credential index * remove unused function declaration Co-authored-by: Restyled.io <[email protected]>
andy31415
pushed a commit
that referenced
this pull request
Jul 9, 2022
* Fix 2 door lock edge cases * remove check of lower bound for credential index * remove unused function declaration Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Michael Rupp <[email protected]> Co-authored-by: Restyled.io <[email protected]>
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.
Problem
When an invalid pinCode is sent with a lock command, and DUT is already locked, a SUCCESS is returned. Expected outcome is that failure is returned because the pinCode is invalid.
When clear fabric from credentials is called with the ProgrammingPin credential, an error is returned. Expected outcome is that success is returned.
Change overview
Remove any checking of lockState in the setLockState function, because the SUCCESS/FAILURE of the function does not depend on the lockState. Also removed a
return truestatement in the case where the current state is the same as the new state command received.Removed the check for the lower bound of the credentialIndex. This is encapsulated in IsValidCredentialIndex, because credentialIndex is unsigned. All door-lock indices are one-indexed, except for ProgrammingPin.. I wonder if this is overlooked in the spec.
Testing
Ran through DRLK TC 2.2 on EFR32, thread network.