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

Support for multi-endpoint for the color control server #9469

Merged

Conversation

mkardous-silabs
Copy link
Contributor

@mkardous-silabs mkardous-silabs commented Sep 3, 2021

Problem

  • Color control server did not support multi-endpoints. Current implementation would cause current color execution to be overwritten when command for a new endpoint was received.
  • Color Control server was implemented with a C style
  • To use ember events (emberEventControlSetDelayMS) to launch system timer, old generated code that is not currently generated needed to be manually changed to support multi-endpoint. This forces the number of endpoints to be known at compile time and does not support dynamic endpoints.
  • A few color control functions did not use accessor functions to read/write attributes

Change overview

  • Refactor to replace all read/write calls weith accessor functions
  • Refactor of color-control-server to be in line with C++ standards
  • Added support for multi-endpoints for color control server
  • Modified ember events (emberEventControlSetDelayMS) to allow callbacks to be set at runtime instead of compile time.

Testing

  • Used Automated tests to validate that the refactor did not break previously implemented features
  • Used Automated tests to validate that multi-endpoint was functional

Copy link
Contributor

@mrjerryjohns mrjerryjohns left a comment

Choose a reason for hiding this comment

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

Need a solution that does not utilize STL maps.

@mrjerryjohns
Copy link
Contributor

Have been discussing with @bzbarsky-apple strategies for how we could solve the problem in this PR.

@mkardous-silabs
Copy link
Contributor Author

Have been discussing with @bzbarsky-apple strategies for how we could solve the problem in this PR.

I agree that a conversation might be useful just to agree on cluster design since dynamic clusters make everything slightly more complicated.

@bzbarsky-apple @jmartinez-silabs @mrjerryjohns

@mrjerryjohns
Copy link
Contributor

Had a discussion with @mkardous-silabs and discussed a pathway to proceed for now given the lack of true support for dynamic endpoints in the code today.

For now, he'll pivot the PR to just supporting fixed endpoints, and at a later time, we'll tackle dynamic.

@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 84e6e1a

File Section File VM
chip-qpg6100-lighting-example.out .text 1552 1552
chip-qpg6100-lighting-example.out .heap 0 40
chip-qpg6100-lighting-example.out .data -40 -40
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,30151
.debug_loc,0,9707
.debug_line,0,6518
.debug_str,0,3325
.debug_abbrev,0,2671
.strtab,0,1836
.debug_ranges,0,1664
.text,1552,1552
.debug_frame,0,1008
.symtab,0,848
.debug_aranges,0,312
.heap,40,0
.data,-40,-40
[Unmapped],0,-1552

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 84e6e1a

File Section File VM
chip-lock.elf text 232 232
chip-lock.elf rodata 40 40
chip-lock.elf device_handles 8 8
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.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,20959
.debug_line,0,2952
.debug_abbrev,0,2697
.debug_loc,0,761
.debug_str,0,539
.strtab,0,322
text,232,232
.debug_frame,0,156
.symtab,0,128
.debug_aranges,0,64
rodata,40,40
device_handles,8,8
.shstrtab,0,-2


@github-actions
Copy link

Size increase report for "esp32-example-build" from 84e6e1a

File Section File VM
chip-all-clusters-app.elf .dram0.heap_start 0 8
chip-all-clusters-app.elf .dram0.bss 0 -8
chip-all-clusters-app.elf .dram0.data -96 -96
chip-all-clusters-app.elf .flash.text -448 -448
chip-all-clusters-app.elf .flash.rodata -616 -616
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,61843
.debug_line,0,9433
.debug_loc,0,8420
.debug_abbrev,0,3673
.debug_str,0,3332
.debug_ranges,0,1768
[Unmapped],0,1160
.debug_frame,0,1092
.strtab,0,576
.debug_aranges,0,312
.dram0.heap_start,8,0
.riscv.attributes,0,-1
.dram0.bss,-8,0
.dram0.data,-96,-96
.symtab,0,-112
.flash.text,-448,-448
.flash.rodata,-616,-616


@woody-apple woody-apple merged commit 3178a47 into project-chip:master Sep 16, 2021
mleisner pushed a commit to mleisner/connectedhomeip that referenced this pull request Sep 20, 2021
…#9469)

* Color Control server refactor to use accessors functions for attributes

* Added support for static multi-end point for color-control

* Refactor color-control-server to c++ standards

* Ember event refactor

* Removed includes that were moved to another directory

* Removed static attributes

* Removed unordered map
Damian-Nordic added a commit to Damian-Nordic/connectedhomeip that referenced this pull request Sep 27, 2021
PR project-chip#9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
Damian-Nordic added a commit to Damian-Nordic/connectedhomeip that referenced this pull request Sep 27, 2021
PR project-chip#9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
woody-apple pushed a commit that referenced this pull request Sep 30, 2021
PR #9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
andy31415 pushed a commit that referenced this pull request Oct 7, 2021
PR #9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
jimlyall-q pushed a commit to jimlyall-q/connectedhomeip that referenced this pull request Oct 11, 2021
PR project-chip#9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
hank820 pushed a commit to hank820/connectedhomeip that referenced this pull request Oct 12, 2021
PR project-chip#9469 changed the implementation of
EventControlHandler function from af-event.cpp, but only
updated Color Control Cluster code to use the new approach.
As a result, all other clusters using scheduled actions,
such as LevelControl.MoveToLevel, got broken.

Make sure both methods of initializing cluster events are
supported until all clusters are aligned one way or another.
Also, add a cirque test for LevelControl.MoveToLevel.
@mkardous-silabs mkardous-silabs deleted the feature/multi_endpoint branch October 19, 2021 12:46
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.

4 participants