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

SystemLayer::PostEvent add API to post a lambda event #9678

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

kghost
Copy link
Contributor

@kghost kghost commented Sep 14, 2021

Problem

Provides a most flexible API to run a deferred function.

This will decouple system object from network endpoints (TCPEndpoint, UDPEndpoint). It helps implementing the system object pool in PR #9590

Change overview

Add a PostLambda to post a generic lambda into system event queue.

There is several limitation of the lambda

  • The lambda must be trivially copyable. Because the lambda is memcpy into the queue. It means that the lambda can only capture pod data (Including pointers).
  • The size of the lambda must less than the queue element size. The size of the lambda is the sum of all captured fields. The body of the lambda is encoded in the type info of the lambda, which is captured by a static function LambdaProxy<Lambda>::Call, and resolved into a function pointer at compile time, assigned to LambdaBridge::LambdaProxy into the queue element.

Testing

Verified by unit tests.

src/system/SystemLayerImplLwIP.cpp Outdated Show resolved Hide resolved
src/system/SystemLayer.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Looking good!

src/platform/LwIPEventSupport.cpp Outdated Show resolved Hide resolved
src/system/SystemLayer.h Outdated Show resolved Hide resolved
@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 8e86c9e

File Section File VM
chip-qpg6100-lighting-example.out .text 132 132
chip-qpg6100-lighting-example.out .heap 0 8
chip-qpg6100-lighting-example.out .bss 0 -8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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'

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

sections,vmsize,filesize
.debug_info,0,3885
.debug_abbrev,0,1870
.debug_str,0,389
.strtab,0,332
.debug_line,0,148
.symtab,0,144
.text,132,132
.debug_frame,0,80
.debug_loc,0,64
.debug_ranges,0,48
.debug_aranges,0,24
.heap,8,0
.bss,-8,0
[Unmapped],0,-132


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 8e86c9e

File Section File VM
chip-lock.elf text 8 8
chip-lock.elf device_handles -8 -8
chip-shell.elf text 8 8
chip-shell.elf device_handles -8 -8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_abbrev,0,2044
.debug_info,0,1702
.debug_str,0,34
.symtab,0,32
.debug_loc,0,10
text,8,8
.debug_line,0,-6
device_handles,-8,-8

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

sections,vmsize,filesize
.debug_abbrev,0,1286
.debug_info,0,1038
.debug_loc,0,162
.debug_str,0,34
.symtab,0,32
text,8,8
.debug_line,0,-8
device_handles,-8,-8


@github-actions
Copy link

Size increase report for "esp32-example-build" from 8e86c9e

File Section File VM
chip-all-clusters-app.elf .flash.text 64 64
chip-all-clusters-app.elf .flash.rodata 56 56
chip-all-clusters-app.elf .dram0.heap_start 0 8
chip-all-clusters-app.elf .dram0.bss 0 -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,5374
.debug_abbrev,0,2591
.debug_loc,0,563
.debug_str,0,375
.strtab,0,332
.debug_line,0,214
.debug_ranges,0,72
.flash.text,64,64
.flash.rodata,56,56
.debug_frame,0,44
.symtab,0,32
.debug_aranges,0,24
.dram0.heap_start,8,0
.riscv.attributes,0,-1
.dram0.bss,-8,0
[Unmapped],0,-120


@woody-apple woody-apple merged commit 747b485 into project-chip:master Sep 17, 2021
mleisner pushed a commit to mleisner/connectedhomeip that referenced this pull request Sep 20, 2021
@mspang
Copy link
Contributor

mspang commented Sep 22, 2021

Here's my belated feedback

  • Is it correct? It seems like it may not be [1]
  • Do we really need it? (would a single function ptr and user_data do?)
  • Do people understand the limitations?
  • Why mix the call-once nature of lambdas with the broadcast nature of other 'events' into a single API ?
  • If we have lambdas which is undoubtedly a more flexible API, why do we even need to be able to store events in the queue?
    (to put it another way, why 'waste' space prefixing the lambda with an enum discriminator if you could instead always express 'broadcast this event' as a lambda and only support queuing lambdas? In for a penny, in for a pound)

[1] https://answers.launchpad.net/gcc-arm-embedded/+question/686870/+index

@kghost
Copy link
Contributor Author

kghost commented Sep 22, 2021

@mspang

  • Is it correct? It seems like it may not be [1]

It is not same as [1], his usage is obviously wrong, he called the constructor twice.

  • Do we really need it? (would a single function ptr and user_data do?)

user_data doesn't contain type information, using lambda, the type information of the posted data will be encoded into the handler function.

  • Do people understand the limitations?

There are static_assert that make sure people do not make mistakes even if doesn't understand the limitations

  • Why mix the call-once nature of lambdas with the broadcast nature of other 'events' into a single API ?

I'm not quite get this one, lambdas can be called multiple times, we can broadcast lambda object if we want.

  • If we have lambdas which is undoubtedly a more flexible API, why do we even need to be able to store events in the queue?

it is a limitation of RToS xQueue API, The API can only post fixed size memory region.

https://www.freertos.org/a00117.html

 BaseType_t xQueueSend(
                            QueueHandle_t xQueue,
                            const void * pvItemToQueue,
                            TickType_t xTicksToWait
                         );

(to put it another way, why 'waste' space prefixing the lambda with an enum discriminator if you could instead always express 'broadcast this event' as a lambda and only support queuing lambdas? In for a penny, in for a pound)

This PR provides a better interface than previous, more flexible and type-safe. It will still take time to migrate all legacy code into the new API. Once all usage are migrated, we can safely remove the enum

[1] https://answers.launchpad.net/gcc-arm-embedded/+question/686870/+index

@mspang
Copy link
Contributor

mspang commented Sep 22, 2021

@mspang

  • Is it correct? It seems like it may not be [1]

It is not same as [1], his usage is obviously wrong, he called the constructor twice.

  • Do we really need it? (would a single function ptr and user_data do?)

user_data doesn't contain type information, using lambda, the type information of the posted data will be encoded into the handler function.

  • Do people understand the limitations?

There are static_assert that make sure people do not make mistakes even if doesn't understand the limitations

  • Why mix the call-once nature of lambdas with the broadcast nature of other 'events' into a single API ?

I'm not quite get this one, lambdas can be called multiple times, we can broadcast lambda object if we want.

  • If we have lambdas which is undoubtedly a more flexible API, why do we even need to be able to store events in the queue?

it is a limitation of RToS xQueue API, The API can only post fixed size memory region.

https://www.freertos.org/a00117.html

 BaseType_t xQueueSend(
                            QueueHandle_t xQueue,
                            const void * pvItemToQueue,
                            TickType_t xTicksToWait
                         );

(to put it another way, why 'waste' space prefixing the lambda with an enum discriminator if you could instead always express 'broadcast this event' as a lambda and only support queuing lambdas? In for a penny, in for a pound)

This PR provides a better interface than previous, more flexible and type-safe. It will still take time to migrate all legacy code into the new API. Once all usage are migrated, we can safely remove the enum

You don't need to migrate the call sites of PostEvent, you can keep those as is and make PostEvent convert the event to a lambda.

All you need to do it is change the array of discriminated union of events and lambdas to an array only of lambdas and implement "dispatch event" as a lambda.

The reason to do it is to make more efficient use of the space in the queue.

[1] https://answers.launchpad.net/gcc-arm-embedded/+question/686870/+index

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.

6 participants