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

[controller] reset Device state for usage in accessories #9386

Merged
merged 2 commits into from
Sep 9, 2021

Conversation

gjc13
Copy link
Contributor

@gjc13 gjc13 commented Sep 1, 2021

Problem

The controller::Device class is currently tightly assuming the device state and forcing a commssion flow starting from PASE.

Change overview

Reset the state in OperationalCertProvisioned so that we can call this function to start from CASE.

Testing

How was this tested?

  • CI build and unit tests.

@gjc13
Copy link
Contributor Author

gjc13 commented Sep 1, 2021

@msandstedt The change follows our discussion in #9103. Though it's kind of hack it unblocks us.

@mspang @andy31415 PTAL.

@gjc13
Copy link
Contributor Author

gjc13 commented Sep 1, 2021

I had offline discussion with Tennessee and here is his comments:

I think you misunderstood Michael's comment
My point in the original comment is that reimplementing Device with subset functionality for each example is problematic
Separate from that I agree with Michael's points but I would say that just hacking the PASE state machine is not quite right. I agree with Michael that changes are few. Since I do not have much time this week, I suggest you work with Spang and Michael on resolution. I think the interaction peer change also was helpful, and overall, we will need to incrementally "separate" device/controller but we have to unblock device to device. I just want to make sure we dont make too many "compromise" changes that make the final solution hard to maintain (that is, as hard or harder to maintain than the problematic code we have now)

Personally I believe the delegate class #9103 is not an elegant solution.
The correct way out is to remove Device class from our controller generated code and use InteractionModelEngine as the only entrypoint to send ZCL or BDX messages. We'll add another class to manage the pairing process and setup the symmetric credentials.

This approach requires rewriting a large amount of code. As an unblocker, this hack looks quite acceptable compared to #9103. If we are using a temporary solution anyway, I'll prefer a smaller change.

@tcarmelveilleux
Copy link
Contributor

Personally I believe the delegate class #9103 is not an elegant solution.
The correct way out is to remove Device class from our controller generated code and use InteractionModelEngine as the only entrypoint to send ZCL or BDX messages. We'll add another class to manage the pairing process and setup the symmetric credentials.

InteractionModelEngine is for the IM, which is a layer above Message Layer. BDX is a protocol handler that does not relate at all to IM. I disagree that InteractionModelEngine should be the entrypoint for everything.

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.

While a 1 line change, what test failed? What is this fixing? What new test is added to confirm that this doesn't regress?

@gjc13
Copy link
Contributor Author

gjc13 commented Sep 2, 2021

While a 1 line change, what test failed? What is this fixing? What new test is added to confirm that this doesn't regress?

Unit test added. It's testing calling EstablishConnectivity after manually calling OperationalCertProvisioned. We'd expect a CASE session to be setup in this case. Before the change it will report ERROR_INCORRECT_STATE.

@gjc13 gjc13 requested a review from woody-apple September 2, 2021 06:47
@gjc13 gjc13 force-pushed the device-state-reset branch from 3a68687 to 4067fb5 Compare September 3, 2021 03:22
@gjc13 gjc13 force-pushed the device-state-reset branch from 4067fb5 to 5d635e6 Compare September 6, 2021 03:51
@github-actions
Copy link

github-actions bot commented Sep 6, 2021

Size increase report for "esp32-example-build" from 92309bc

File Section File VM
chip-shell.elf .flash.text 48 48
chip-ipv6only-app.elf .flash.text 172 172
chip-lock-app.elf .flash.text 64 64
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.flash.text,48,48
[Unmapped],0,-48

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize
[Unmapped],0,3924
.flash.text,172,172

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.flash.text,64,64
.debug_loc,0,4
.xt.lit._ZNK4chip4SpanIhE7SubSpanEjj,0,-8
.xt.prop._ZNK4chip4SpanIhE7SubSpanEjj,0,-12
[Unmapped],0,-64


@woody-apple woody-apple merged commit 741ab0a into project-chip:master Sep 9, 2021
@gjc13 gjc13 mentioned this pull request Sep 9, 2021
kpschoedel pushed a commit to kpschoedel/connectedhomeip that referenced this pull request Sep 9, 2021
…p#9386)

* [controller] reset Device state for usage in accessories

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

Successfully merging this pull request may close these issues.

5 participants