SystemLayerImplSelect: add libev support; CHIP_SYSTEM_CONFIG_USE_LIBEV #24232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR #24232: Size comparison from 9a1c31c to 8c122eb Increases (8 builds for bl602, bl702, cc13x2_26x2, psoc6, telink)
Decreases (9 builds for bl702, cc13x2_26x2, esp32, nrfconnect, psoc6, qpg, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
8c122eb to
1e1d426
Compare
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda and
mlepage-google
|
Writing because stalebot threatens to close the issue... What are the actual roadblocks here? While I agree with @andy31415 that separating darwin/posix/libev would be the mid term goal in this area, that should not really kill the libev integration in the current state. The libev #ifdefs are not uglier than the Darwin ones are. The code itself is running fine and runs fine on top of recent master. Would it help to rebase the PR to current master? |
|
@plan44 Looks like the main blockers are dealing with the above comments (e.g. filing issues to track them, since they did not seem to block the review) and getting a non-Google review (which I can work on). |
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
2b9c9de to
87aba88
Compare
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
87aba88 to
467b86b
Compare
|
PR #24232: Size comparison from bd43948 to 467b86b Increases (1 build for cc32xx)
Decreases (2 builds for cc32xx, nrfconnect)
Full report (5 builds for cc32xx, mbed, nrfconnect)
|
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
467b86b to
80ada61
Compare
|
PR #24232: Size comparison from bd43948 to 80ada61 Increases (9 builds for bl602, cc32xx, esp32, k32w, psoc6, telink)
Decreases (16 builds for bl702, cc13x4_26x4, cc32xx, efr32, psoc6, telink)
Full report (66 builds for bl602, bl702, cc13x2_26x2, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
approved these changes
May 31, 2023
src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp
Outdated
Show resolved
Hide resolved
src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp
Outdated
Show resolved
Hide resolved
When CHIP_SYSTEM_CONFIG_USE_LIBEV is set, SystemLayerImplSelect expects a *libev* mainloop to be present to schedule timers and socket watches (similar to CHIP_SYSTEM_CONFIG_USE_DISPATCH for Darwin). A libev mainloop must be passed to SystemLayer using `SetLibEvLoop()` before any timers or socket watches are used - otherwise, `chipDie()` is invoked. Platform manager events are also dispatched via the libev mainloop, using ScheduleWork(). This eliminates the need for a separate thread for dispatching events, and makes sure event handlers cannot run in parallel with timers or I/O handlers. In consequence PostEvent() may not be called without holding the chip lock. The reason for running *matter* under libev in the first place is to allow creating single-threaded apps including all of *matter*. So disallowing PostEvent() "from any thread" is a design choice for the libev case. # Usage The entire project needs to be build with `CHIP_SYSTEM_CONFIG_USE_LIBEV=1` (this can be done via invoking a project-specific extra config via the `default_configs_extra` argument in args.gni) Setting up the libev mainloop and handing it over to SystemLayer must be done in application specific code, outside the code provided by chip examples. Also adding libev as a dependency must be done in the application's BUILD.gn. # Background *libev* is a multi-platform event library often used in embedded linux context to handle events, and builds the basis for various libraries with non-blocking APIs. This changeset allows using the *matter* stack with libev based applications. # Example The opensource bridge project p44mbrd (https://github.com/plan44/p44mbrd) is based on libev and makes use of this changeset.
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
80ada61 to
b9ffb72
Compare
|
PR #24232: Size comparison from 77b1596 to b9ffb72 Increases (10 builds for bl602, bl702, cc32xx, cyw30739, psoc6, telink)
Decreases (13 builds for cc32xx, k32w, nrfconnect, psoc6, telink)
Full report (58 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
@plan44 Should #24232 (comment) just get spun off into a separate issue? |
Yes, please - as said I'd need some guidance to even understand what a "variant" would mean in that context. It is not possible to just enable CHIP_SYSTEM_CONFIG_USE_LIBEV and compile an example that way - the libev based main app would be missing. |
|
@plan44 Please file that issue? |
|
@bzbarsky-apple done -> #27022 (sorry for being a bit slow at times finding the right path to do things here 🤯 ) |
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Aug 22, 2023
libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] project-chip#24232 (review)
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Aug 22, 2023
libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] project-chip#24232 (review)
plan44
added a commit
to plan44/connectedhomeip
that referenced
this pull request
Aug 29, 2023
libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] project-chip#24232 (review)
mergify bot
pushed a commit
that referenced
this pull request
Aug 29, 2023
) * SystemLayerImplSelect: libev: avoid timers firing slightly too early (#28434) This fixes a ReportEngine problem that was caused by libev based timers firing slightly (in the range of 20mS) too early, because libev by default uses the time when events started processing as the "now" reference for relative timers for efficiency reasons. To ensure timers cannot fire early, timer setup must compensate for any difference between ev_now() which is the time events started processing and ev_time(), which is the actual current time (but is a bit less efficient to obtain). # Conflicts: # src/system/SystemLayerImplSelect.cpp * SystemLayerImplSelect: libev: eliminate I/O wakeup thread libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] #24232 (review) * SystemLayerImplSelect: reword comment: early firing timers can happen - the fix prevents them in normal libev case - however the caller MUST NOT rely on timers *never* firing a bit early
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…#28434) (project-chip#28740) * SystemLayerImplSelect: libev: avoid timers firing slightly too early (project-chip#28434) This fixes a ReportEngine problem that was caused by libev based timers firing slightly (in the range of 20mS) too early, because libev by default uses the time when events started processing as the "now" reference for relative timers for efficiency reasons. To ensure timers cannot fire early, timer setup must compensate for any difference between ev_now() which is the time events started processing and ev_time(), which is the actual current time (but is a bit less efficient to obtain). # Conflicts: # src/system/SystemLayerImplSelect.cpp * SystemLayerImplSelect: libev: eliminate I/O wakeup thread libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] project-chip#24232 (review) * SystemLayerImplSelect: reword comment: early firing timers can happen - the fix prevents them in normal libev case - however the caller MUST NOT rely on timers *never* firing a bit early
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Note: this is a rebased version of #22043, which was closed by stalebot despite ongoing discussion. Details were discussed there already and led to this cleaned up and now 3month field tested version]
When CHIP_SYSTEM_CONFIG_USE_LIBEV is set, SystemLayerImplSelect expects a libev [1] mainloop to be present to schedule timers and socket watches (similar to CHIP_SYSTEM_CONFIG_USE_DISPATCH for Darwin).
A libev mainloop must be passed to SystemLayer using
SetLibEvLoop()before any timers or socket watches are used - otherwise,chipDie()is invoked.Usage
The entire project needs to be built with
CHIP_SYSTEM_CONFIG_USE_LIBEV=1(this can be done via invoking a project-specific extra config via thedefault_configs_extraargument in args.gni)Setting up the libev mainloop and handing it over to SystemLayer must be done in application specific code, outside the code provided by chip examples. Also adding libev as a dependency must be done in the application's BUILD.gn.
Background
libev is a multi-platform event library often used in embedded linux context to handle events, and builds the basis for various libraries with non-blocking APIs. This changeset allows using the connectedhomeip stack with libev based applications.
In particular, CHIP_SYSTEM_CONFIG_USE_LIBEV=1 is in active use and real-world testing by the p44mbrd [2] opensource matter bridge project running mainly on openwrt linux devices.
Testing
CHIP_SYSTEM_CONFIG_USE_LIBEVnot set.CHIP_SYSTEM_CONFIG_USE_LIBEV=1: chip timer and socket watches work the same way as with select or dispatch based applications.