Topics

[RFCv2 1/2] misc: Add timer API


Luiz Augusto von Dentz
 

From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>

This adds a timer fiber which can be used track timeouts removing the
need to use one delayed fiber per timeout.

Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
---
include/misc/timer.h | 106 ++++++++++++++++++++++++
misc/Kconfig | 37 +++++++++
misc/Makefile | 1 +
misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/Kconfig | 1 +
5 files changed, 369 insertions(+)
create mode 100644 include/misc/timer.h
create mode 100644 misc/timer.c

diff --git a/include/misc/timer.h b/include/misc/timer.h
new file mode 100644
index 0000000..d3ce695
--- /dev/null
+++ b/include/misc/timer.h
@@ -0,0 +1,106 @@
+/** @file
+ * @brief System timer APIs.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+extern struct sys_timer_queue *__global_tq;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
+};
+
+/**
+ * @brief Init timer queue
+ *
+ * @param tq Timer queue object that should be initialized
+ * @param stack_size The stack size in bytes
+ */
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size);
+
+/**
+ * @brief Add timer to a timer queue
+ *
+ * Both timer and timer queue must be initialized.
+ *
+ * @param tq Timer queue object to add the timer
+ * @param t Timer object to be added
+ * @param ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t,
+ int ticks);
+
+/**
+ * @brief Cancel timer
+ *
+ * Remove timer from timer queue and stop it so its callback is not called.
+ *
+ * @param tq Timer queue object where the timer was added
+ * @param t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t);
+
+/**
+ * @brief Init timer
+ *
+ * @param t Timer object to be initialized
+ * @param func Function callback to be called when timer expires
+ * @param user_data Data to be passed in the callback
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data);
+
+/**
+ * @brief Add timer to the global timer queue
+ *
+ * Timer must be initialized.
+ *
+ * @param _t Timer object to be added
+ * @param _ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_add(_t, _ticks) sys_timer_queue_add(__global_tq, _t, _ticks)
+
+/**
+ * @brief Cancel global timer
+ *
+ * Remove timer from the global timer queue and stop it so its callback is not
+ * called.
+ *
+ * @param _t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_cancel(_t) sys_timer_queue_cancel(__global_tq, _t)
diff --git a/misc/Kconfig b/misc/Kconfig
index 4c05169..a9bd80f 100644
--- a/misc/Kconfig
+++ b/misc/Kconfig
@@ -374,3 +374,40 @@ config REBOOT
needed to perform a "safe" reboot (e.g. SYSTEM_CLOCK_DISABLE, to stop the
system clock before issuing a reset).
endmenu
+
+menu "Timer Options"
+
+config SYS_TIMER
+ bool
+ prompt "Enable System Timer"
+ default n
+ select NANO_TIMEOUTS
+ select NANO_TIMERS
+ help
+ Global switch for enabling sys_timer API.
+
+config SYS_TIMER_STACK_SIZE
+ int "Size of the global timer fiber stack"
+ depends on SYS_TIMER
+ default 512
+ range 512 65536
+ help
+ Size of the global timer fiber stack. This is the context from
+ which all global timer callbacks occur.
+
+config SYS_LOG_TIMER_LEVEL
+ int
+ prompt "System Timer Log level"
+ depends on SYS_LOG && SYS_TIMER
+ default 0
+ range 0 4
+ help
+ Sets log level for System Timers.
+ Levels are:
+ 0 OFF, do not write
+ 1 ERROR, only write SYS_LOG_ERR
+ 2 WARNING, write SYS_LOG_WRN in adition to previous level
+ 3 INFO, write SYS_LOG_INF in adition to previous levels
+ 4 DEBUG, write SYS_LOG_DBG in adition to previous levels
+
+endmenu
diff --git a/misc/Makefile b/misc/Makefile
index cb9a718..d57b8f2 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_CPLUSPLUS) += cpp_virtual.o cpp_vtable.o \
cpp_init_array.o cpp_ctors.o cpp_dtors.o
obj-$(CONFIG_PRINTK) += printk.o
obj-$(CONFIG_REBOOT) += reboot.o
+obj-$(CONFIG_SYS_TIMER) += timer.o
obj-y += generated/
obj-y += debug/
diff --git a/misc/timer.c b/misc/timer.c
new file mode 100644
index 0000000..90bf196
--- /dev/null
+++ b/misc/timer.c
@@ -0,0 +1,224 @@
+/** @file
+ * @brief Internal APIs for Bluetooth timer handling.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <nanokernel.h>
+#include <toolchain.h>
+#include <sections.h>
+#include <string.h>
+#include <errno.h>
+#include <misc/stack.h>
+#include <misc/timer.h>
+
+#define SYS_LOG_DOMAIN "System Timer"
+#define SYS_LOG_LEVEL CONFIG_SYS_LOG_TIMER_LEVEL
+#include <misc/sys_log.h>
+
+#if CONFIG_SYS_LOG_TIMER_LEVEL > 0
+/* Enabling debug increases stack size requirement considerably */
+#define STACK_DEBUG_EXTRA 512
+#else
+#define STACK_DEBUG_EXTRA 0
+#endif
+
+#define STACK(name, size) \
+ char __stack name[(size) + STACK_DEBUG_EXTRA]
+
+struct timer_queue {
+ struct sys_timer_queue tq;
+
+ STACK(stack, CONFIG_SYS_TIMER_STACK_SIZE);
+};
+
+static struct timer_queue global_tq = {
+ .tq.stack_size = CONFIG_SYS_TIMER_STACK_SIZE,
+};
+
+struct sys_timer_queue *__global_tq = &global_tq.tq;
+
+static void timer_expired(struct sys_timer *t)
+{
+ SYS_LOG_DBG("t %p", t);
+
+ /* Just to be safe stop the timer */
+ nano_fiber_timer_stop(&t->__nano_timer);
+
+ t->func(t->user_data);
+}
+
+static int32_t timer_queue_ticks_remains(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ stack_analyze("timer queue stack", tq->stack, tq->stack_size);
+
+ while (tq->__head) {
+ struct sys_timer *cur;
+
+ /* If head is not expired sleep the remaining ticks */
+ ticks = nano_timer_ticks_remain(&tq->__head->__nano_timer);
+ if (ticks > 0) {
+ goto done;
+ }
+
+ /* Remove head since it has expired */
+ cur = tq->__head;
+ tq->__head = tq->__head->__next;
+
+ /* Execute callback */
+ timer_expired(cur);
+ }
+
+ /* No timers left, abort fiber */
+ ticks = 0;
+
+done:
+ SYS_LOG_DBG("tq %p ticks %d", tq, ticks);
+
+ return ticks;
+}
+
+static void timer_queue_fiber(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id);
+
+ while ((ticks = timer_queue_ticks_remains(tq))) {
+ fiber_sleep(ticks);
+ }
+
+ SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id);
+
+ tq->__thread_id = 0;
+ fiber_abort();
+}
+
+static int timer_queue_start(struct sys_timer_queue *tq)
+{
+ if (tq->__thread_id) {
+ return 0;
+ }
+
+ tq->__thread_id = fiber_start(tq->stack, tq->stack_size,
+ (nano_fiber_entry_t)timer_queue_fiber,
+ (int) tq, 0, 7, 0);
+
+ return 0;
+}
+
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size)
+{
+ if (!tq || !stack_size) {
+ return -EINVAL;
+ }
+
+ tq->stack_size = stack_size;
+
+ return 0;
+}
+
+static inline void timer_queue_wakeup(struct sys_timer_queue *tq)
+{
+ if (sys_thread_self_get() != tq->__thread_id) {
+ fiber_wakeup(tq->__thread_id);
+ }
+}
+
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks);
+
+ if (!t || !t->func)
+ return -EINVAL;
+
+ timer_queue_start(tq);
+ nano_timer_start(&t->__nano_timer, ticks);
+
+ /* Sort the list of timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) {
+ break;
+ }
+ }
+
+ t->__next = cur;
+
+ if (prev) {
+ prev->__next = t;
+ } else {
+ tq->__head = t;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p", tq, t);
+
+ if (!t) {
+ return -EINVAL;
+ }
+
+ /* Lookup existing timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (cur == t) {
+ break;
+ }
+ }
+
+ if (!cur) {
+ return -ENOENT;
+ }
+
+ nano_timer_stop(&t->__nano_timer);
+
+ /* Remove timer for the queue */
+ if (prev) {
+ prev->__next = t->__next;
+ } else {
+ tq->__head = t->__next;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
+{
+ if (!t || !func) {
+ return -EINVAL;
+ }
+
+ t->func = func;
+ t->user_data = user_data;
+ nano_timer_init(&t->__nano_timer, t);
+
+ return 0;
+}
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 2ffb0e0..aa2712b 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL

config BLUETOOTH_CONN
bool
+ select SYS_TIMER
default n

if BLUETOOTH_CONN
--
2.5.5


Tomasz Bursztyka
 

Hi Luiz,



+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
Giving the pointer on current sys_timer instead you could:

+
+struct sys_timer {
+ struct nano_timer __nano_timer;
+ sys_timer_func func;
+ void *user_data;
remove that pointer, and up to user to use CONTAINER_OF() if he wants to.
(yes I am reusing ideas from Johan ;) )

+ struct sys_timer *__next;
+};
+
+/**
+ * @brief Init timer queue
+ *
+ * @param tq Timer queue object that should be initialized
+ * @param stack_size The stack size in bytes
+ */
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size);
Are you ever going to use another queue besides the system's one (the
global one)?
It could simplify a lot the API then. Unless I miss to see some use case
for custom queue

+
+/**
+ * @brief Add timer to a timer queue
+ *
+ * Both timer and timer queue must be initialized.
+ *
+ * @param tq Timer queue object to add the timer
And continuing on that idea, is it then worth asking to which queue you
want to add a timer?

+ * @param t Timer object to be added
+ * @param ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t,
+ int ticks);
so you could remove tq parameter.

+
+/**
+ * @brief Cancel timer
+ *
+ * Remove timer from timer queue and stop it so its callback is not called.
+ *
+ * @param tq Timer queue object where the timer was added
+ * @param t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t);
here as well.

+
+/**
+ * @brief Init timer
+ *
+ * @param t Timer object to be initialized
+ * @param func Function callback to be called when timer expires
+ * @param user_data Data to be passed in the callback
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data);
+
+/**
+ * @brief Add timer to the global timer queue
+ *
+ * Timer must be initialized.
+ *
+ * @param _t Timer object to be added
+ * @param _ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_add(_t, _ticks) sys_timer_queue_add(__global_tq, _t, _ticks)
+
+/**
+ * @brief Cancel global timer
+ *
+ * Remove timer from the global timer queue and stop it so its callback is not
+ * called.
+ *
+ * @param _t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_cancel(_t) sys_timer_queue_cancel(__global_tq, _t)
And these 2 defines would also go away.

And struct sys_timer_queue could also be private.

Tomasz


Luiz Augusto von Dentz
 

Hi Tomasz,

On Wed, Apr 20, 2016 at 12:05 PM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:
Hi Luiz,



+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);

Giving the pointer on current sys_timer instead you could:

+
+struct sys_timer {
+ struct nano_timer __nano_timer;
+ sys_timer_func func;
+ void *user_data;

remove that pointer, and up to user to use CONTAINER_OF() if he wants to.
(yes I am reusing ideas from Johan ;) )
Yep, this actually saves a pointer and in fact is something I thought
about but for some reason did not implement, anyway I will change to
make use of CONTAINER_OF.

+ struct sys_timer *__next;
+};
+
+/**
+ * @brief Init timer queue
+ *
+ * @param tq Timer queue object that should be initialized
+ * @param stack_size The stack size in bytes
+ */
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size);

Are you ever going to use another queue besides the system's one (the global
one)?
It could simplify a lot the API then. Unless I miss to see some use case for
custom queue
Well a private/custom one could be useful in case the stack needs to
different, and I was actually thinking to include a priority later on.
But yes it could be simplified in the first version so later on we can
check for actual use cases.

+
+/**
+ * @brief Add timer to a timer queue
+ *
+ * Both timer and timer queue must be initialized.
+ *
+ * @param tq Timer queue object to add the timer

And continuing on that idea, is it then worth asking to which queue you want
to add a timer?

+ * @param t Timer object to be added
+ * @param ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t,
+ int ticks);

so you could remove tq parameter.

+
+/**
+ * @brief Cancel timer
+ *
+ * Remove timer from timer queue and stop it so its callback is not
called.
+ *
+ * @param tq Timer queue object where the timer was added
+ * @param t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer
*t);

here as well.


+
+/**
+ * @brief Init timer
+ *
+ * @param t Timer object to be initialized
+ * @param func Function callback to be called when timer expires
+ * @param user_data Data to be passed in the callback
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void
*user_data);
+
+/**
+ * @brief Add timer to the global timer queue
+ *
+ * Timer must be initialized.
+ *
+ * @param _t Timer object to be added
+ * @param _ticks Amount of time in ticks to wait
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_add(_t, _ticks) sys_timer_queue_add(__global_tq, _t,
_ticks)
+
+/**
+ * @brief Cancel global timer
+ *
+ * Remove timer from the global timer queue and stop it so its callback
is not
+ * called.
+ *
+ * @param _t Timer object to be canceled
+ *
+ * @return Zero on success or (negative) error code otherwise.
+ */
+#define sys_timer_cancel(_t) sys_timer_queue_cancel(__global_tq, _t)

And these 2 defines would also go away.

And struct sys_timer_queue could also be private.

Tomasz


--
Luiz Augusto von Dentz


Benjamin Walsh <benjamin.walsh@...>
 

On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>

This adds a timer fiber which can be used track timeouts removing the
need to use one delayed fiber per timeout.

Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
---
include/misc/timer.h | 106 ++++++++++++++++++++++++
misc/Kconfig | 37 +++++++++
misc/Makefile | 1 +
misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/Kconfig | 1 +
5 files changed, 369 insertions(+)
create mode 100644 include/misc/timer.h
create mode 100644 misc/timer.c
This is a good functionality to add to the kernel I think. Two things
though:

1. It should probably end up in the kernel directory: I don't think this
is a 'misc' functionality.

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?


diff --git a/include/misc/timer.h b/include/misc/timer.h
new file mode 100644
index 0000000..d3ce695
--- /dev/null
+++ b/include/misc/timer.h
@@ -0,0 +1,106 @@
+/** @file
+ * @brief System timer APIs.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+extern struct sys_timer_queue *__global_tq;
^^
We try not to use double-underscores. Single one is enough, but since
this is a global symbol, it should have a more descriptive name.

+ size_t stack_size;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
^^^
I'm not sure why you use double-underscores in structure field names...?

+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
^^
also here

+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
^^
and here

[..snip..]

+static void timer_queue_fiber(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id);
+
+ while ((ticks = timer_queue_ticks_remains(tq))) {
+ fiber_sleep(ticks);
+ }
+
+ SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id);
+
+ tq->__thread_id = 0;
+ fiber_abort();
^^^^^^^^^^^^^^

Don't do this: it's done automatically by the kernel when the fiber
entry point runs to completion.

I think it looks good, but I should not be doing a code review on the
mailing list. I'll do the full review in gerrit when it ends up there.

Cheers,
Ben

+}
+
+static int timer_queue_start(struct sys_timer_queue *tq)
+{
+ if (tq->__thread_id) {
+ return 0;
+ }
+
+ tq->__thread_id = fiber_start(tq->stack, tq->stack_size,
+ (nano_fiber_entry_t)timer_queue_fiber,
+ (int) tq, 0, 7, 0);
+
+ return 0;
+}
+
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size)
+{
+ if (!tq || !stack_size) {
+ return -EINVAL;
+ }
+
+ tq->stack_size = stack_size;
+
+ return 0;
+}
+
+static inline void timer_queue_wakeup(struct sys_timer_queue *tq)
+{
+ if (sys_thread_self_get() != tq->__thread_id) {
+ fiber_wakeup(tq->__thread_id);
+ }
+}
+
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks);
+
+ if (!t || !t->func)
+ return -EINVAL;
+
+ timer_queue_start(tq);
+ nano_timer_start(&t->__nano_timer, ticks);
+
+ /* Sort the list of timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) {
+ break;
+ }
+ }
+
+ t->__next = cur;
+
+ if (prev) {
+ prev->__next = t;
+ } else {
+ tq->__head = t;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p", tq, t);
+
+ if (!t) {
+ return -EINVAL;
+ }
+
+ /* Lookup existing timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (cur == t) {
+ break;
+ }
+ }
+
+ if (!cur) {
+ return -ENOENT;
+ }
+
+ nano_timer_stop(&t->__nano_timer);
+
+ /* Remove timer for the queue */
+ if (prev) {
+ prev->__next = t->__next;
+ } else {
+ tq->__head = t->__next;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
+{
+ if (!t || !func) {
+ return -EINVAL;
+ }
+
+ t->func = func;
+ t->user_data = user_data;
+ nano_timer_init(&t->__nano_timer, t);
+
+ return 0;
+}
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 2ffb0e0..aa2712b 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL

config BLUETOOTH_CONN
bool
+ select SYS_TIMER
default n

if BLUETOOTH_CONN
--
2.5.5


Luiz Augusto von Dentz
 

Hi Benjamin,

On Thu, Apr 21, 2016 at 5:32 PM, Benjamin Walsh
<benjamin.walsh(a)windriver.com> wrote:
On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>

This adds a timer fiber which can be used track timeouts removing the
need to use one delayed fiber per timeout.

Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
---
include/misc/timer.h | 106 ++++++++++++++++++++++++
misc/Kconfig | 37 +++++++++
misc/Makefile | 1 +
misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/Kconfig | 1 +
5 files changed, 369 insertions(+)
create mode 100644 include/misc/timer.h
create mode 100644 misc/timer.c
This is a good functionality to add to the kernel I think. Two things
though:

1. It should probably end up in the kernel directory: I don't think this
is a 'misc' functionality.
Yep, I asked about this but nobody seems to be interested at that
point so I used misc to start with.

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.


diff --git a/include/misc/timer.h b/include/misc/timer.h
new file mode 100644
index 0000000..d3ce695
--- /dev/null
+++ b/include/misc/timer.h
@@ -0,0 +1,106 @@
+/** @file
+ * @brief System timer APIs.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+extern struct sys_timer_queue *__global_tq;
^^
We try not to use double-underscores. Single one is enough, but since
this is a global symbol, it should have a more descriptive name.
I understood from the code that __ are used for internal/private
stuch, such as in uint64_t __noinit __start_tsc;, well without this
being stated in the coding style it is kind hard to to guess.

+ size_t stack_size;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
^^^
I'm not sure why you use double-underscores in structure field names...?

+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
^^
also here

+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
^^
and here

[..snip..]

+static void timer_queue_fiber(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id);
+
+ while ((ticks = timer_queue_ticks_remains(tq))) {
+ fiber_sleep(ticks);
+ }
+
+ SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id);
+
+ tq->__thread_id = 0;
+ fiber_abort();
^^^^^^^^^^^^^^

Don't do this: it's done automatically by the kernel when the fiber
entry point runs to completion.
Okay, I will update it shortly.

I think it looks good, but I should not be doing a code review on the
mailing list. I'll do the full review in gerrit when it ends up there.
Actually I find these type of review much more convenient than gerrit,
anyway you can find them there as well:

https://gerrit.zephyrproject.org/r/#/c/1593/

Cheers,
Ben

+}
+
+static int timer_queue_start(struct sys_timer_queue *tq)
+{
+ if (tq->__thread_id) {
+ return 0;
+ }
+
+ tq->__thread_id = fiber_start(tq->stack, tq->stack_size,
+ (nano_fiber_entry_t)timer_queue_fiber,
+ (int) tq, 0, 7, 0);
+
+ return 0;
+}
+
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size)
+{
+ if (!tq || !stack_size) {
+ return -EINVAL;
+ }
+
+ tq->stack_size = stack_size;
+
+ return 0;
+}
+
+static inline void timer_queue_wakeup(struct sys_timer_queue *tq)
+{
+ if (sys_thread_self_get() != tq->__thread_id) {
+ fiber_wakeup(tq->__thread_id);
+ }
+}
+
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks);
+
+ if (!t || !t->func)
+ return -EINVAL;
+
+ timer_queue_start(tq);
+ nano_timer_start(&t->__nano_timer, ticks);
+
+ /* Sort the list of timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) {
+ break;
+ }
+ }
+
+ t->__next = cur;
+
+ if (prev) {
+ prev->__next = t;
+ } else {
+ tq->__head = t;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p", tq, t);
+
+ if (!t) {
+ return -EINVAL;
+ }
+
+ /* Lookup existing timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (cur == t) {
+ break;
+ }
+ }
+
+ if (!cur) {
+ return -ENOENT;
+ }
+
+ nano_timer_stop(&t->__nano_timer);
+
+ /* Remove timer for the queue */
+ if (prev) {
+ prev->__next = t->__next;
+ } else {
+ tq->__head = t->__next;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
+{
+ if (!t || !func) {
+ return -EINVAL;
+ }
+
+ t->func = func;
+ t->user_data = user_data;
+ nano_timer_init(&t->__nano_timer, t);
+
+ return 0;
+}
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 2ffb0e0..aa2712b 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL

config BLUETOOTH_CONN
bool
+ select SYS_TIMER
default n

if BLUETOOTH_CONN
--
2.5.5


--
Luiz Augusto von Dentz


Luiz Augusto von Dentz
 

Hi,

On Thu, Apr 21, 2016 at 6:51 PM, Luiz Augusto von Dentz
<luiz.dentz(a)gmail.com> wrote:
Hi Benjamin,

On Thu, Apr 21, 2016 at 5:32 PM, Benjamin Walsh
<benjamin.walsh(a)windriver.com> wrote:
On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>

This adds a timer fiber which can be used track timeouts removing the
need to use one delayed fiber per timeout.

Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
---
include/misc/timer.h | 106 ++++++++++++++++++++++++
misc/Kconfig | 37 +++++++++
misc/Makefile | 1 +
misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/Kconfig | 1 +
5 files changed, 369 insertions(+)
create mode 100644 include/misc/timer.h
create mode 100644 misc/timer.c
This is a good functionality to add to the kernel I think. Two things
though:

1. It should probably end up in the kernel directory: I don't think this
is a 'misc' functionality.
Yep, I asked about this but nobody seems to be interested at that
point so I used misc to start with.
Under kernel there is either nanokernel or microkernel, not sure where
a sys_* API should go? It is actually really messy to have directly in
nanokernel.h since it is all missed up with #ifdef in between, perhaps
we should have a system.h for sys_ API?

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.
Since nobody is suggesting anything here are some alternatives:

sys_timer_callback*: quite long imo
sys_callback: doesn't say much
sys_timeout: I would favor this one


diff --git a/include/misc/timer.h b/include/misc/timer.h
new file mode 100644
index 0000000..d3ce695
--- /dev/null
+++ b/include/misc/timer.h
@@ -0,0 +1,106 @@
+/** @file
+ * @brief System timer APIs.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+extern struct sys_timer_queue *__global_tq;
^^
We try not to use double-underscores. Single one is enough, but since
this is a global symbol, it should have a more descriptive name.
I understood from the code that __ are used for internal/private
stuch, such as in uint64_t __noinit __start_tsc;, well without this
being stated in the coding style it is kind hard to to guess.

+ size_t stack_size;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
^^^
I'm not sure why you use double-underscores in structure field names...?

+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
^^
also here

+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
^^
and here

[..snip..]

+static void timer_queue_fiber(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id);
+
+ while ((ticks = timer_queue_ticks_remains(tq))) {
+ fiber_sleep(ticks);
+ }
+
+ SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id);
+
+ tq->__thread_id = 0;
+ fiber_abort();
^^^^^^^^^^^^^^

Don't do this: it's done automatically by the kernel when the fiber
entry point runs to completion.
Okay, I will update it shortly.

I think it looks good, but I should not be doing a code review on the
mailing list. I'll do the full review in gerrit when it ends up there.
Actually I find these type of review much more convenient than gerrit,
anyway you can find them there as well:

https://gerrit.zephyrproject.org/r/#/c/1593/

Cheers,
Ben

+}
+
+static int timer_queue_start(struct sys_timer_queue *tq)
+{
+ if (tq->__thread_id) {
+ return 0;
+ }
+
+ tq->__thread_id = fiber_start(tq->stack, tq->stack_size,
+ (nano_fiber_entry_t)timer_queue_fiber,
+ (int) tq, 0, 7, 0);
+
+ return 0;
+}
+
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size)
+{
+ if (!tq || !stack_size) {
+ return -EINVAL;
+ }
+
+ tq->stack_size = stack_size;
+
+ return 0;
+}
+
+static inline void timer_queue_wakeup(struct sys_timer_queue *tq)
+{
+ if (sys_thread_self_get() != tq->__thread_id) {
+ fiber_wakeup(tq->__thread_id);
+ }
+}
+
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks);
+
+ if (!t || !t->func)
+ return -EINVAL;
+
+ timer_queue_start(tq);
+ nano_timer_start(&t->__nano_timer, ticks);
+
+ /* Sort the list of timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) {
+ break;
+ }
+ }
+
+ t->__next = cur;
+
+ if (prev) {
+ prev->__next = t;
+ } else {
+ tq->__head = t;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p", tq, t);
+
+ if (!t) {
+ return -EINVAL;
+ }
+
+ /* Lookup existing timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (cur == t) {
+ break;
+ }
+ }
+
+ if (!cur) {
+ return -ENOENT;
+ }
+
+ nano_timer_stop(&t->__nano_timer);
+
+ /* Remove timer for the queue */
+ if (prev) {
+ prev->__next = t->__next;
+ } else {
+ tq->__head = t->__next;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
+{
+ if (!t || !func) {
+ return -EINVAL;
+ }
+
+ t->func = func;
+ t->user_data = user_data;
+ nano_timer_init(&t->__nano_timer, t);
+
+ return 0;
+}
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 2ffb0e0..aa2712b 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL

config BLUETOOTH_CONN
bool
+ select SYS_TIMER
default n

if BLUETOOTH_CONN
--
2.5.5


--
Luiz Augusto von Dentz


--
Luiz Augusto von Dentz


Benjamin Walsh <benjamin.walsh@...>
 

On Thu, Apr 21, 2016 at 06:51:19PM +0300, Luiz Augusto von Dentz wrote:
+extern struct sys_timer_queue *__global_tq;
^^
We try not to use double-underscores. Single one is enough, but since
this is a global symbol, it should have a more descriptive name.
I understood from the code that __ are used for internal/private
stuch, such as in uint64_t __noinit __start_tsc;, well without this
being stated in the coding style it is kind hard to to guess.
The only things that should be using double-underscore are the
attributes macros (e.g. __weak for __attribute__((__weak)))). There are
other cases, such as symbols from linker scripts, ARM and ARC exception
handlers, maybe others, but these are legacy from the old codebase, and
should be changed when we have the time.


Benjamin Walsh <benjamin.walsh@...>
 

On Fri, Apr 22, 2016 at 03:22:45PM +0300, Luiz Augusto von Dentz wrote:
Hi,

On Thu, Apr 21, 2016 at 6:51 PM, Luiz Augusto von Dentz
<luiz.dentz(a)gmail.com> wrote:
Hi Benjamin,

On Thu, Apr 21, 2016 at 5:32 PM, Benjamin Walsh
<benjamin.walsh(a)windriver.com> wrote:
On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>

This adds a timer fiber which can be used track timeouts removing the
need to use one delayed fiber per timeout.

Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
---
include/misc/timer.h | 106 ++++++++++++++++++++++++
misc/Kconfig | 37 +++++++++
misc/Makefile | 1 +
misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/Kconfig | 1 +
5 files changed, 369 insertions(+)
create mode 100644 include/misc/timer.h
create mode 100644 misc/timer.c
This is a good functionality to add to the kernel I think. Two things
though:

1. It should probably end up in the kernel directory: I don't think this
is a 'misc' functionality.
Yep, I asked about this but nobody seems to be interested at that
point so I used misc to start with.
Under kernel there is either nanokernel or microkernel, not sure where
a sys_* API should go? It is actually really messy to have directly in
nanokernel.h since it is all missed up with #ifdef in between, perhaps
we should have a system.h for sys_ API?
It uses nanokernel timers, so I would put it under nanokernel.

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.
Since nobody is suggesting anything here are some alternatives:

sys_timer_callback*: quite long imo
sys_callback: doesn't say much
sys_timeout: I would favor this one
I would prefer sys_callback I think: you add a callback with a timeout
shows that there is a timeout/timer IMHO.

int sys_callback_queue_init(struct sys_timer_queue *tq, size_t stack_size)
int sys_callback_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
int sys_callback_cancel(struct sys_timer_queue *tq, struct sys_timer *t)

int sys_callback_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
^^^^^^^^^^^^^^^^^^

Not sure why this is not part of sys_callback_queue_init though...

Or, how about 'sys_callout' ?


diff --git a/include/misc/timer.h b/include/misc/timer.h
new file mode 100644
index 0000000..d3ce695
--- /dev/null
+++ b/include/misc/timer.h
@@ -0,0 +1,106 @@
+/** @file
+ * @brief System timer APIs.
+ */
+
+/*
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+extern struct sys_timer_queue *__global_tq;
^^
We try not to use double-underscores. Single one is enough, but since
this is a global symbol, it should have a more descriptive name.
I understood from the code that __ are used for internal/private
stuch, such as in uint64_t __noinit __start_tsc;, well without this
being stated in the coding style it is kind hard to to guess.

+ size_t stack_size;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
^^^
I'm not sure why you use double-underscores in structure field names...?

+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
^^
also here

+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
^^
and here

[..snip..]

+static void timer_queue_fiber(struct sys_timer_queue *tq)
+{
+ int32_t ticks;
+
+ SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id);
+
+ while ((ticks = timer_queue_ticks_remains(tq))) {
+ fiber_sleep(ticks);
+ }
+
+ SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id);
+
+ tq->__thread_id = 0;
+ fiber_abort();
^^^^^^^^^^^^^^

Don't do this: it's done automatically by the kernel when the fiber
entry point runs to completion.
Okay, I will update it shortly.

I think it looks good, but I should not be doing a code review on the
mailing list. I'll do the full review in gerrit when it ends up there.
Actually I find these type of review much more convenient than gerrit,
anyway you can find them there as well:

https://gerrit.zephyrproject.org/r/#/c/1593/

Cheers,
Ben

+}
+
+static int timer_queue_start(struct sys_timer_queue *tq)
+{
+ if (tq->__thread_id) {
+ return 0;
+ }
+
+ tq->__thread_id = fiber_start(tq->stack, tq->stack_size,
+ (nano_fiber_entry_t)timer_queue_fiber,
+ (int) tq, 0, 7, 0);
+
+ return 0;
+}
+
+int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size)
+{
+ if (!tq || !stack_size) {
+ return -EINVAL;
+ }
+
+ tq->stack_size = stack_size;
+
+ return 0;
+}
+
+static inline void timer_queue_wakeup(struct sys_timer_queue *tq)
+{
+ if (sys_thread_self_get() != tq->__thread_id) {
+ fiber_wakeup(tq->__thread_id);
+ }
+}
+
+int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks);
+
+ if (!t || !t->func)
+ return -EINVAL;
+
+ timer_queue_start(tq);
+ nano_timer_start(&t->__nano_timer, ticks);
+
+ /* Sort the list of timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) {
+ break;
+ }
+ }
+
+ t->__next = cur;
+
+ if (prev) {
+ prev->__next = t;
+ } else {
+ tq->__head = t;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t)
+{
+ struct sys_timer *cur, *prev;
+
+ SYS_LOG_DBG("tq %p t %p", tq, t);
+
+ if (!t) {
+ return -EINVAL;
+ }
+
+ /* Lookup existing timers */
+ for (cur = tq->__head, prev = NULL; cur;
+ prev = cur, cur = cur->__next) {
+ if (cur == t) {
+ break;
+ }
+ }
+
+ if (!cur) {
+ return -ENOENT;
+ }
+
+ nano_timer_stop(&t->__nano_timer);
+
+ /* Remove timer for the queue */
+ if (prev) {
+ prev->__next = t->__next;
+ } else {
+ tq->__head = t->__next;
+ /* Wakeup timer queue fiber since there is a new head */
+ timer_queue_wakeup(tq);
+ }
+
+ return 0;
+}
+
+int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data)
+{
+ if (!t || !func) {
+ return -EINVAL;
+ }
+
+ t->func = func;
+ t->user_data = user_data;
+ nano_timer_init(&t->__nano_timer, t);
+
+ return 0;
+}
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 2ffb0e0..aa2712b 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL

config BLUETOOTH_CONN
bool
+ select SYS_TIMER
default n

if BLUETOOTH_CONN
--
2.5.5


--
Luiz Augusto von Dentz


--
Luiz Augusto von Dentz
--
Benjamin Walsh, SMTS
Wind River Rocket
www.windriver.com
Zephyr kernel maintainer
www.zephyrproject.org


Tomasz Bursztyka
 

Hi Benjamin,

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.
Since nobody is suggesting anything here are some alternatives:

sys_timer_callback*: quite long imo
sys_callback: doesn't say much
sys_timeout: I would favor this one
Me too.

I would prefer sys_callback I think: you add a callback with a timeout
shows that there is a timeout/timer IMHO.
A timeout says all without people to read parameters. At a timeout
usually, it means
you want to do something, thus the callback. It's not suprizing timeout
keyword is usually
preferred in some user-land libraries on Linux or else.

Tomasz


Benjamin Walsh <benjamin.walsh@...>
 

On Mon, Apr 25, 2016 at 02:08:00PM +0200, Tomasz Bursztyka wrote:
Hi Benjamin,

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.
Since nobody is suggesting anything here are some alternatives:

sys_timer_callback*: quite long imo
sys_callback: doesn't say much
sys_timeout: I would favor this one
Me too.

I would prefer sys_callback I think: you add a callback with a timeout
shows that there is a timeout/timer IMHO.
A timeout says all without people to read parameters. At a timeout
usually, it means you want to do something, thus the callback. It's
not suprizing timeout keyword is usually preferred in some user-land
libraries on Linux or else.
Don't you think it would be confusing for a user that we have two timer
APIs and a timeout API ?


Luiz Augusto von Dentz
 

Hi Ben,

On Mon, Apr 25, 2016 at 6:50 PM, Benjamin Walsh
<benjamin.walsh(a)windriver.com> wrote:
On Mon, Apr 25, 2016 at 02:08:00PM +0200, Tomasz Bursztyka wrote:
Hi Benjamin,

2. I am not sure about the naming. This is yet another timer library,
when we already have nano timers and micro timers. What makes it special
is its callback functionality. Can we try to get that into the name ?
Sure we can add a timeout or callback to the name, but IMO this would
be much more useful than nano_timer thus why I would like to promote
it over it.
Since nobody is suggesting anything here are some alternatives:

sys_timer_callback*: quite long imo
sys_callback: doesn't say much
sys_timeout: I would favor this one
Me too.

I would prefer sys_callback I think: you add a callback with a timeout
shows that there is a timeout/timer IMHO.
A timeout says all without people to read parameters. At a timeout
usually, it means you want to do something, thus the callback. It's
not suprizing timeout keyword is usually preferred in some user-land
libraries on Linux or else.
Don't you think it would be confusing for a user that we have two timer
APIs and a timeout API ?
Well that was what I mentioned that the use of nano_timer is very
limited, if you look at the unit test you will quickly realize how
hard it is to use in practice. That said Im in favor of push this
quickly and then decide the name later since we do depend in this
functionality to implement some protocol requirements in Bluetooth and
probably IP will switch to use it as well.

--
Luiz Augusto von Dentz