sys/cb_mux: Callback multiplexer #8993

ZetaR60 · 2018-04-20T03:15:22Z

This is a rough sketch of a callback multiplexer, intended primarily for use in CPUs that have multiple pin interrupts tied to a single interrupt vector (but could be useful for other things too). It uses a singly linked list to allow callbacks to be dynamically added and deleted as needed so that all interrupts are usable without wasting memory when few are used.

Posted early in the development process to facilitate discussion in #7610.

bergzand

Two initial questions.

Furthermore, I think the name "cbmplex" is not really clear. I don't have a really good suggestion at the moment, maybe somebody else comes along with a good name.

bergzand · 2018-05-01T11:25:33Z

sys/cbmplex/cbmplex.c

+    return entry;
+}
+
+cbmplex_t *cbmplex_find_flags(cbmplex_t *head, uint8_t flags, uint8_t mask)


Is there a use case for having different flags and masks? Why not use flags as a mask?

mask tells the function which bits to search, and flags tells the function what they should be set to. I will put a comment to clarify this.

Of course -_-

bergzand · 2018-05-01T11:26:36Z

sys/include/cbmplex.h

+typedef struct cbmplex {
+    struct cbmplex *next;
+    uint8_t flags;
+    uint8_t cbid;


What is the idea behind the callback ID?

This is whatever the system using the multiplexer uses to identify the callback. In the case of pin interrupts, it would be the pin number.

Not sure I understand what you mean. How does the "system" here know the cbid? It is not passed as argument in the callback.

See example.

jnohlgard · 2018-05-01T11:45:17Z

regarding name, "mux" is a more common shorthand for "multiplexer" (in electronics)

bergzand · 2018-05-01T11:46:17Z

"cb_mux", or "callback_mux" maybe then

ZetaR60 · 2018-05-01T15:41:09Z

Name changed to cb_mux

ZetaR60 · 2018-05-01T16:02:17Z

Here is an example usage.

Initialization:

foo code calls gpio_init_int for a pin change interrupt (PCINT)
gpio.c creates a cb_mux entry and attaches it to its list of PCINTs. It puts the cb and arg into cb_mux's cb and arg, and the flank into flags. It gives it an cbid equal to the pin gpio_t
since PCINTs on this system don't have a flank register and trigger on any change (like on the ATMega), the current state of that pin is also put into flags
gpio.c then enables the PCINT for that pin

Pin change event (on foo's pin):

gpio.c's handler for PCINTs triggers
the handler searches using cb_mux_find_cbid for each entry that had a pin change event
the handler checks the flank in those entries to see if the callback needs to be executed, and executes foo's callback
the handler updates the current state of each pin (option to use cb_mux_update)

ZetaR60 · 2018-05-01T16:38:28Z

Note: There is an issue with the current definition of cbid. Different systems define gpio_t differently, some use up to a uint32_t. Setting cbid to uint32_t seems really big for small systems, and setting it to gpio_t seems wrong if it is not being used to hold pins. Suggestions?

ZetaR60 · 2018-05-01T19:41:29Z

Bergzand and I have been discussing potential issues with the API being too specific to pin interrupts. I have tried to make it more generic:

I have removed flags and flag searches in favor of a generic void *info, so it can be handled however the code using this API wishes.
cbid is now 32 bits, which will support more use cases. I do feel that it would be nice for it to be smaller, but that would require restricting the potential uses.
cb_mux_nextid is replaced by cb_mux_find_hilo_id, which should be more flexible in potential applications.
cb_mux_update is renamed cb_mux_iter, which more generically describes what it does.

I am thinking of another example use case with timers, e.g. allowing multiple callbacks with RTC/RTT implementations:

cbid is the alarm time for the given callback
when the current alarm goes off, it uses cb_mux_find_hilo_id to find the next lowest time and sets that as the next alarm

kYc0o · 2018-05-07T13:04:13Z

Have you measured the overhead in response time of this approach against direct callbacks (which costs memory of course)?

ZetaR60 · 2018-05-07T16:23:58Z

Next on my todo list is to write a test for this code. I can include measuring overhead into that. Finding a callback is O(n), where n is the number of callbacks in the list. I count around 6 instructions per iteration over the list of callbacks.

kYc0o · 2018-05-09T16:35:21Z

Ok, so if we merge this asap you'll be able to provide such a test soon? I want to move forward for interrupt handling on AVR.

ZetaR60 · 2018-05-09T16:40:59Z

@kYc0o I intend to provide the test in this PR.

ZetaR60 · 2018-05-15T07:13:15Z

Notice to interested parties, especially @kYc0o and @roberthartung:

There are some fundamental problems with using cb_mux for its original intended purpose: getting rid of callback limitations in APIs like periph/gpio.h. This is due to the inability for the implementation to allocate memory for new list entries (i.e. no malloc) in a manner transparent to the code using the API. I suspect that an array will be more appropriate for statically allocated memory. For these reasons I will be scrapping cb_mux unless someone really wants it or has a compelling reason to keep it.

Sorry for my oversight in not noticing this, and thanks to @roberthartung for pointing it out.

jnohlgard · 2018-05-15T07:36:22Z

@ZetaR60 you can work around the memory allocation issues using xfa (#7523, #9105)

ZetaR60 · 2018-05-15T19:18:40Z

@gebart If I am understanding xfa correctly, the memory is still statically allocated at compile time (although in a very neat way). The problem is that something like a periph/gpio.h implementation has no way of knowing how many times pin init functions are going to be called, so I don't see how statically allocated memory would work. Of course the API could be changed to pass memory, but then xfa would not be needed. Or you could redefine the function calls in the API to be macros that hide memory allocation, but that could introduce problems when they are assumed to be function calls.

I have been discussing the issue with @kYc0o and the different possible ways around the allocation problem. I think the best compromise so far for use cases like PCINTs is to define an array of structs that contain the callback information. For example, if you had 4 entries, you would be able to use 0-4 PCINTs at a time, but you would get an error if you tried to use 5. You could then allow this number to be changed in periph_conf.h on a board-by-board basis. This would also be much simpler than cb_mux.

kaspar030 · 2018-05-17T21:30:07Z

possible ways around the allocation problem

Maybe if we'd introduce a gpio_int_t type, which any user of a gpio interrupt needs to provide? That struct could contain necessary per-platform fields. It would be an API change, but we could move the allocation to wherever the interrupt is used.

ZetaR60 · 2018-05-17T21:44:03Z

A new type would work. I did interrupts in #9054 by using an array to hold the information, and the max number of entries in the list is defined in board.h. That would be a potential alternative that doesn't require an API change.

ZetaR60 · 2018-05-19T05:54:16Z

Rebased on trunk and added periph/gpio.h support for cb_mux. No CPU actually supports it yet, but everything should silently ignore the passed gpio_int_t entry.

@kaspar030 Is this approximately what you were suggesting?

It is going to be a lot of work implementing cb_mux for all of the CPUs. If we are going with this method (rather than using arrays), how should we proceed?

ZetaR60 · 2018-05-21T00:50:34Z

I have added a comprehensive test routine, and also used it to fix many bugs with the API:

[liveuser@localhost-live cb_mux]$ make BOARD=mega-xplained test
./tests/01-run.py
/home/liveuser/Desktop/RIOT_cbmplex/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "9600"
No handlers could be found for logger "root"
2018-05-20 20:37:18,644 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2018-05-20 20:37:22,646 - INFO # 

2018-05-20 20:37:22,746 - INFO # main(): This is RIOT! (Version: 2018.07-devel-235-gb4a04-localhost-live-RIOT_cbmplex)
2018-05-20 20:37:22,812 - INFO # cb_mux test routine
2018-05-20 20:37:22,814 - INFO # Test list addition, retrieval, execution of 5 CBs
2018-05-20 20:37:22,846 - INFO # Callback 0 executed
2018-05-20 20:37:22,912 - INFO # Callback 1 executed
2018-05-20 20:37:22,914 - INFO # Callback 2 executed
2018-05-20 20:37:22,916 - INFO # Callback 3 executed
2018-05-20 20:37:22,918 - INFO # Callback 4 executed
2018-05-20 20:37:23,014 - INFO # Test list deletion of CB 0, 2, 4, execution of 1, 3
2018-05-20 20:37:23,016 - INFO # Callback 1 executed
2018-05-20 20:37:23,018 - INFO # Callback 3 executed
2018-05-20 20:37:23,112 - INFO # Test execution of CB with lowest ID (1)
2018-05-20 20:37:23,114 - INFO # Callback 1 executed
2018-05-20 20:37:23,146 - INFO # Test execution of CB with highest ID (3)
2018-05-20 20:37:23,147 - INFO # Callback 3 executed
2018-05-20 20:37:23,213 - INFO # Re-adding list entries (0, 2, 4) by finding next free ID
2018-05-20 20:37:23,214 - INFO # Added entry 0
2018-05-20 20:37:23,247 - INFO # Added entry 2
2018-05-20 20:37:23,248 - INFO # Added entry 4
2018-05-20 20:37:23,314 - INFO # Test iteration of a function over list
2018-05-20 20:37:23,317 - INFO # Entry 0 was updated correctly
2018-05-20 20:37:23,347 - INFO # Entry 1 was updated correctly
2018-05-20 20:37:23,415 - INFO # Entry 2 was updated correctly
2018-05-20 20:37:23,418 - INFO # Entry 3 was updated correctly
2018-05-20 20:37:23,448 - INFO # Entry 4 was updated correctly
2018-05-20 20:37:23,547 - INFO # Tests complete!

[liveuser@localhost-live cb_mux]$ echo $?
0

Installed compiler toolchains 
-----------------------------
             native gcc: missing
      arm-none-eabi-gcc: missing
                avr-gcc: avr-gcc (Fedora 7.2.0-1.fc27) 7.2.0
       mips-mti-elf-gcc: missing
             msp430-gcc: missing
   riscv-none-embed-gcc: missing
                  clang: missing

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: missing
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
               avr-libc: "2.0.0" ("20150208")

Installed development tools
---------------------------
                  cmake: cmake version 3.11.0
               cppcheck: missing
                doxygen: missing
                 flake8: missing
                    git: git version 2.14.2
             coccinelle: missing

ZetaR60 · 2018-05-22T07:11:42Z

Rebased to separate the API changes out into #9159

ZetaR60 · 2018-05-22T22:17:06Z

Added a simple benchmark at @kYc0o 's request.

[liveuser@localhost-live cb_mux_bench]$ make BOARD=mega-xplained test
./tests/01-run.py
/home/liveuser/Desktop/RIOT_cbmplex/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "9600"
No handlers could be found for logger "root"
2018-05-22 18:11:45,030 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2018-05-22 18:11:48,732 - INFO # 

2018-05-22 18:11:48,800 - INFO # main(): This is RIOT! (Version: 2018.07-devel-231-g316235-localhost-live-RIOT_cbmplex)
2018-05-22 18:11:48,833 - INFO # cb_mux benchmark application
2018-05-22 18:11:48,899 - INFO # Populating cb_mux list with 20 items
2018-05-22 18:11:48,901 - INFO # Finding the last list entry
2018-05-22 18:11:48,932 - INFO # List walk time: 80 us
2018-05-22 18:11:49,032 - INFO # Walk time less than threshold of 200 us
2018-05-22 18:11:49,033 - INFO # [SUCCESS]

[liveuser@localhost-live cb_mux_bench]$ echo $?
0

kYc0o · 2018-05-24T17:40:45Z

I'd ACK in general this PR, however is failing for samr21-xpro. I ran Murdock and HIL tests to know where else is failing.

kYc0o · 2018-05-24T17:48:19Z

Basically it's a printf problem. Just try to use as much as possible portable data types, so their size depend on the architecture.

ZetaR60 · 2018-05-24T20:27:00Z

printf fixed. Tested again to double check on mega-xplained, and both tests pass. Murdock looks good with the cb_mux tests, but has unrelated failures on ps_schedstatistics and shell board tests.

You might notice the double cast: this is because gcc doesn't like to cast from a pointer to an integer of a different size, even if it is guaranteed to be equal or smaller.

ZetaR60 · 2018-05-28T17:38:18Z

@bergzand Do you think everything here is good to go?

aabadie

looks good codewise, but there's an issue with doxygen grouping. See the comment below

aabadie · 2018-05-28T18:33:05Z

sys/include/cb_mux.h

+ */
+
+/**
+ * @ingroup   sys_cb_mux


this doxygen group is not defined. you can use a @defgroup above this line and maybe also add a bit of documentation.

@defgroup and summary added.

aabadie

ACK, please squash

ZetaR60 · 2018-05-28T20:37:31Z

Unrelated board failures on Murdock again. I am attempting to resubmit the rebased commits to get it to succeed.

ZetaR60 · 2018-05-28T21:27:12Z

I gave Murdock several tries but it is still failing here:

tests/struct_tm_utility/samr21-xpro
tests/ps_schedstatistics/samr21-xpro
tests/shell/samr21-xpro

ZetaR60 · 2018-05-29T06:41:14Z

Created #9220 to try to fix the Murdock issue.

aabadie · 2018-05-29T07:40:50Z

I removed the CI:run tests because of unrelated failures, at least the added cb_mux test script passes. Let's merge when Murdock is green.

aabadie · 2018-05-29T08:06:01Z

All green now, merging. Thanks @ZetaR60 !

ZetaR60 mentioned this pull request Apr 20, 2018

cpu/atmega: WIP implementation of pin change interrupts #7610

Closed

bergzand added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label May 1, 2018

bergzand reviewed May 1, 2018

View reviewed changes

ZetaR60 force-pushed the RIOT_cbmplex branch from d065698 to c35a342 Compare May 1, 2018 15:38

bergzand changed the title ~~sys/cbmplex: Callback multiplexer~~ sys/cb_mux: Callback multiplexer May 1, 2018

ZetaR60 mentioned this pull request May 16, 2018

ACU-AMNx board and MVP checklist #9143

Closed

24 tasks

ZetaR60 force-pushed the RIOT_cbmplex branch from d872957 to 021e077 Compare May 19, 2018 05:45

ZetaR60 mentioned this pull request May 22, 2018

gpio.h/cb_mux API change and interim compatibilty #9159

Closed

ZetaR60 force-pushed the RIOT_cbmplex branch from d52d234 to 316235d Compare May 22, 2018 07:06

kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CI Area: Continuous Integration of RIOT components labels May 24, 2018

kYc0o added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label May 24, 2018

kYc0o added this to the Release 2018.07 milestone May 24, 2018

aabadie reviewed May 28, 2018

View reviewed changes

Matthew Blue added 2 commits May 28, 2018 15:43

sys/cb_mux: initial support

02697fa

tests/cb_mux: initial test routine

c767cdd

aabadie approved these changes May 28, 2018

View reviewed changes

ZetaR60 force-pushed the RIOT_cbmplex branch 2 times, most recently from fa6490b to 588b66a Compare May 28, 2018 20:13

ZetaR60 force-pushed the RIOT_cbmplex branch 2 times, most recently from a9cec66 to 4c3edb2 Compare May 28, 2018 21:02

ZetaR60 force-pushed the RIOT_cbmplex branch from 4c3edb2 to 41591c5 Compare May 29, 2018 00:50

tests/cb_mux: initial benchmark routine

7a95463

ZetaR60 force-pushed the RIOT_cbmplex branch from 41591c5 to 7a95463 Compare May 29, 2018 04:15

roberthartung mentioned this pull request May 29, 2018

cpu/atmega_common: external interrupt fix and refactor #8951

Merged

aabadie merged commit 657bd25 into RIOT-OS:master May 29, 2018

This was referenced Mar 6, 2019

[WIP] cpu/atmega_common: pin change interrupt implementation #11114

Closed

cpu/atmega_common: pseudomodule-based pin change interrupt implementation #11122

Merged

sys/cb_mux: Callback multiplexer #8993

sys/cb_mux: Callback multiplexer #8993

Conversation

ZetaR60 commented Apr 20, 2018

bergzand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnohlgard commented May 1, 2018

bergzand commented May 1, 2018

ZetaR60 commented May 1, 2018

ZetaR60 commented May 1, 2018 • edited Loading

ZetaR60 commented May 1, 2018

ZetaR60 commented May 1, 2018 • edited Loading

kYc0o commented May 7, 2018

ZetaR60 commented May 7, 2018

kYc0o commented May 9, 2018

ZetaR60 commented May 9, 2018

ZetaR60 commented May 15, 2018

jnohlgard commented May 15, 2018

ZetaR60 commented May 15, 2018 • edited Loading

kaspar030 commented May 17, 2018

ZetaR60 commented May 17, 2018

ZetaR60 commented May 19, 2018 • edited Loading

ZetaR60 commented May 21, 2018

ZetaR60 commented May 22, 2018

ZetaR60 commented May 22, 2018

kYc0o commented May 24, 2018

kYc0o commented May 24, 2018

ZetaR60 commented May 24, 2018

ZetaR60 commented May 28, 2018

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

ZetaR60 commented May 28, 2018

ZetaR60 commented May 28, 2018

ZetaR60 commented May 29, 2018

aabadie commented May 29, 2018

aabadie commented May 29, 2018

ZetaR60 commented May 1, 2018 •

edited

Loading

ZetaR60 commented May 1, 2018 •

edited

Loading

ZetaR60 commented May 15, 2018 •

edited

Loading

ZetaR60 commented May 19, 2018 •

edited

Loading