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

Get peer's node ID from session instead of packet header for response handling #9287

Merged
merged 2 commits into from
Aug 28, 2021

Conversation

pan-apple
Copy link
Contributor

Problem

The reports from peer node are getting dropped when received on the controller.

CHIP: [CTL] OnMessageReceived was called for unknown source node

Change overview

The unicast encrypted messages do not carry node IDs in packet headers. The controller code was using the source node ID to identify which device sent the message. This PR updates it to use session information to identify the device object.

Testing

A new test (testSendClusterTestCluster_Reporting_0002_ReportOnOff_Test) has been added to ensure that reports are received and processed on the controller.

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.

@msandstedt msandstedt self-requested a review August 26, 2021 23:56
@kghost
Copy link
Contributor

kghost commented Aug 27, 2021

I think this is part of PR #8726

Would you please help me review that PR, it fixes lots of incorrect usage of packet header like this one.

@pan-apple pan-apple force-pushed the get-peer-id-from-session branch from dea3bf4 to 9eec07e Compare August 27, 2021 16:34
@andy31415
Copy link
Contributor

For my own knowledge: how is reporting used today? I am looking through the spec and see reporting mentioned as a transaction for subscriptions, however I also am aware there is a lot of back and forth regarding how subscriptions are implemented.

How does reporting work right now, how much is based on spec and what do the tests validate?

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.

Any chance to add tests beyond end-to-end in darwin?

End to end tests are slower and darwin in particular is limited to only 5 CI runners so it is slower than any linux tests.

@andy31415
Copy link
Contributor

  • note: will not block PR on spec or testing - just please update the typo and we can merge.

I would like to express some concern about darwin-only tests because slow and ideally maybe some way to record what is spec and what is temporary until it better evolves.

@sagar-apple
Copy link
Contributor

sagar-apple commented Aug 27, 2021

For my own knowledge: how is reporting used today? I am looking through the spec and see reporting mentioned as a transaction for subscriptions, however I also am aware there is a lot of back and forth regarding how subscriptions are implemented.

How does reporting work right now, how much is based on spec and what do the tests validate?

I can describe what reporting does today. Currently some clusters support configuring reports on a specific attribute. When reports are configured the server sends a report of the attributes state out to each "bound" peer.
I'm not sure if the current implementation is spec compliant but reports have been functioning as described until recently.

The added Darwin tests are just setting up these reports on the OnOff Cluster and making sure at least one report is sent out.
In sequence they are testing that the

  • the controller can bind to the OnOff Cluster on accessory
  • the controller can request to configure a reporting interval for the OnOff attribute
  • the controller is able to receive at least 1 report
  • the controller is able to disable reporting.

@github-actions
Copy link

Size increase report for "esp32-example-build" from f4c307f

File Section File VM
chip-temperature-measurement-app.elf .flash.text -60 -60
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

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-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-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

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

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


@mspang
Copy link
Contributor

mspang commented Aug 27, 2021

For my own knowledge: how is reporting used today? I am looking through the spec and see reporting mentioned as a transaction for subscriptions, however I also am aware there is a lot of back and forth regarding how subscriptions are implemented.
How does reporting work right now, how much is based on spec and what do the tests validate?

I can describe what reporting does today. Currently some clusters support configuring reports on a specific attribute. When reports are configured the server sends a report of the attributes state out to each "bound" peer.
I'm not sure if the current implementation is spec compliant but reports have been functioning as described until recently.

The added Darwin tests are just setting up these reports on the OnOff Cluster and making sure at least one report is sent out.
In sequence they are testing that the

  • the controller can bind to the OnOff Cluster on accessory
  • the controller can request to configure a reporting interval for the OnOff attribute
  • the controller is able to receive at least 1 report
  • the controller is able to disable reporting.

This is so different from spec that we didn't even think it was worth enabling it in the Android controller.

See #8757

@woody-apple woody-apple merged commit b64ee77 into project-chip:master Aug 28, 2021
@pan-apple pan-apple deleted the get-peer-id-from-session branch August 30, 2021 14:37
mkardous-silabs pushed a commit to mkardous-silabs/connectedhomeip that referenced this pull request Sep 24, 2021
… handling (project-chip#9287)

* Get peer's node ID from session instead of packet header for response handling

* Fix typo
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.

8 participants