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

OpCreds Fabric Management functionality #6233

Merged

Conversation

shana-apple
Copy link
Contributor

@shana-apple shana-apple commented Apr 22, 2021

  • Adds functionality for reading the fabrics list attribute which gets updated after any updates to the admin pairing table.
  • Adds functionary for removeFabric.
  • Adds a setFabric command as a temporary command before AddOptCert is implemented. This allows the commissioner to set their vendorId for their fabric and returns to them their fabricId
  • Remove the label field of FabricDescriptor struct because a bug in the generated code makes it that the whole list is corrupted if if is present (we need to fix whatever is wrong with OCTET_STRING in lists). This means that the UpdateFabricLabel implementation is not included in this patch.
  • UI updates in the iOS demo app

src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
@shana-apple shana-apple force-pushed the sazria/fabricLogicNoLabel branch from 29b6e7f to b3e6b5f Compare April 22, 2021 22:28
src/messaging/ExchangeMessageDispatch.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/app/util/basic-types.h Outdated Show resolved Hide resolved
src/darwin/CHIPTool/CHIPTool.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
@mspang mspang marked this pull request as draft April 27, 2021 23:51
@mspang
Copy link
Contributor

mspang commented Apr 27, 2021

This needs restyling so it doesn't change every single generated file.

@shana-apple shana-apple force-pushed the sazria/fabricLogicNoLabel branch 2 times, most recently from 2ed665e to a304781 Compare April 28, 2021 18:19
@bzbarsky-apple bzbarsky-apple marked this pull request as ready for review April 28, 2021 18:39
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Adding changes requested for the storage delegate coupling - it seems we are trying to simplify ember callbacks however this introduces extra globals inside the transport layer.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Approving, however please address other comments from mspang@ and resolve conflicts.

I would also note that PRs involving generated code are very hard to review properly.

@shana-apple shana-apple force-pushed the sazria/fabricLogicNoLabel branch from 0bc73a1 to 9e29cb3 Compare May 4, 2021 16:29
@shana-apple
Copy link
Contributor Author

Approving, however please address other comments from mspang@ and resolve conflicts.

I would also note that PRs involving generated code are very hard to review properly.

Agreed, I tend to add the regenerated files in a separate commit but we should have a script that regenerates them on push on build or something.

@shana-apple shana-apple requested a review from mspang May 4, 2021 16:54
@shana-apple shana-apple force-pushed the sazria/fabricLogicNoLabel branch from 41edb69 to 3bf2ced Compare May 4, 2021 17:33
@woody-apple
Copy link
Contributor

@saurabhst ?

    Adds functionality for reading the fabrics list attribute which gets updated after any updates to the admin pairing table.
    Adds functionary for removeFabric.
    Adds a setFabric command as a temporary command before AddOptCert is implemented. This allows the commissioner to set their vendorId for their fabric and returns to them their fabricId
    Remove the label field of FabricDescriptor struct because a bug in the generated code makes it that the whole list is corrupted if if is present (we need to fix whatever is wrong with OCTET_STRING in lists). This means that the UpdateFabricLabel implementation is not included in this patch.
    UI updates in the iOS demo app
@shana-apple shana-apple force-pushed the sazria/fabricLogicNoLabel branch from 3bf2ced to aeeb165 Compare May 4, 2021 19:50
@woody-apple
Copy link
Contributor

@msandstedt ?

@woody-apple woody-apple merged commit 3907f38 into project-chip:master May 5, 2021
@github-actions
Copy link

github-actions bot commented May 5, 2021

Size increase report for "nrfconnect-example-build" from 0f443b8

File Section File VM
chip-shell.elf text 428 428
chip-shell.elf rodata 352 356
chip-shell.elf device_handles 4 4
chip-lock.elf text 696 696
chip-lock.elf rodata 680 680
chip-lock.elf bss 0 275
chip-lock.elf device_handles 8 8
chip-lock.elf [LOAD #3 [RW]] 0 -19
chip-lighting.elf rodata 608 612
chip-lighting.elf text 608 608
chip-lighting.elf bss 0 256
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,10622
.debug_loc,0,2063
.debug_line,0,1326
.debug_str,0,1293
text,428,428
rodata,356,352
.debug_abbrev,0,340
.strtab,0,242
.symtab,0,224
.debug_frame,0,196
.debug_aranges,0,48
.debug_ranges,0,16
device_handles,4,4
.shstrtab,0,2

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,13762
.debug_loc,0,2561
.debug_line,0,1571
.debug_str,0,1307
text,696,696
rodata,680,680
.symtab,0,400
.debug_abbrev,0,335
.strtab,0,298
bss,275,0
.debug_frame,0,264
.debug_ranges,0,232
.debug_aranges,0,56
device_handles,8,8
.shstrtab,0,-2
[LOAD #3 [RW]],-19,0

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,13797
.debug_loc,0,2553
.debug_line,0,1568
.debug_str,0,1307
rodata,612,608
text,608,608
.symtab,0,400
.debug_abbrev,0,311
.strtab,0,298
.debug_frame,0,264
bss,256,0
.debug_ranges,0,232
.debug_aranges,0,56
.shstrtab,0,-2


@github-actions
Copy link

github-actions bot commented May 5, 2021

Size increase report for "esp32-example-build" from 0f443b8

File Section File VM
chip-all-clusters-app.elf .flash.rodata 1832 1832
chip-all-clusters-app.elf .flash.text 1700 1700
chip-all-clusters-app.elf .dram0.bss 0 264
chip-all-clusters-app.elf [6 Others] 234 8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,44640
.debug_line,0,7915
.debug_loc,0,3222
.debug_str,0,2511
[Unmapped],0,2256
.debug_abbrev,0,1942
.flash.rodata,1832,1832
.flash.text,1700,1700
.strtab,0,1088
.shstrtab,0,848
.debug_ranges,0,768
.symtab,0,496
.debug_frame,0,432
.dram0.bss,264,0
[6 Others],8,234
.debug_aranges,0,144
.xt.prop._ZN32OpCredsAdminPairingTableDelegate25OnAdminPersistedToStorageEPN4chip9Transport16AdminPairingInfoE,0,128
.xt.lit._ZN32OpCredsAdminPairingTableDelegate25OnAdminPersistedToStorageEPN4chip9Transport16AdminPairingInfoE,0,88
.xt.prop._ZN32OpCredsAdminPairingTableDelegate25OnAdminDeletedFromStorageEt,0,88
.xt.prop._ZN32OpCredsAdminPairingTableDelegate27OnAdminRetrievedFromStorageEPN4chip9Transport16AdminPairingInfoE,0,88
.xt.prop._ZN32OpCredsAdminPairingTableDelegateD0Ev,0,76

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize


bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this pull request May 5, 2021
This is due to a merge conflict between PR project-chip#6233 nd PR project-chip#6442.  The
completion handlers got renamed to response handlers in the latter,
but the former is still calling one of them completionHandler.
andy31415 pushed a commit that referenced this pull request May 6, 2021
This is due to a merge conflict between PR #6233 nd PR #6442.  The
completion handlers got renamed to response handlers in the latter,
but the former is still calling one of them completionHandler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants