Hi,

As per suggestion from Gerrit comment, moving the discussion to mailing list.

Background
--------------

On Quark (SE and D2000), there are Always On Counter (free running) and Always ON Periodic Timer devices. A counter|timer driver is to be added for supporting these devices. At same time, the goal is to create an API that is generic enough for not just these two specific devices.

Proposal:
-----------

Generic counter driver API:
-------------------------------
We will have 3 routines - start, stop, and read, which takes in the device pointer - which identifies the timer. The driver header, counter.h, is under /include folder:

/**
* @brief Set up counter configuration and start it.
* @param dev Pointer to the device structure for the driver instance.
* @param config Pointer to counter configuration structure
*
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_start(struct device *dev, counter_input *config);

/**
* @brief Stop the counter.
* @param dev Pointer to the device structure for the driver instance.
*
* @retval DEV_OK If successful.
* @retval DEV_* Code otherwise.
*/
int counter_stop(struct device *dev);

/**
* @brief Read current counter value
* @param dev Pointer to the device structure for the driver instance.
*
* @return 32-bit value
*/
uint32_t counter_read(struct device *dev)

The counter_input structure can be something like below:

/**
* @brief Counter configuration.
*
* Acceptable setting in the configuration structure is hardware-specific.
*
* initial_value - Initial value to load to the counter or timer.
* callback - Callback function when timer expires.
*/
struct counter_input {
uint32_t initial_value;
void (*callback)(void);
};

Note - Using structure, tomorrow we can add more fields for a counter with more features.
For the Free Running counter in Quark, these fields would be null as it does not support interrupt or callback notification.

The hardware-specific driver implementation:
-----------------------------------

This can be implemented in /drivers directory. For example, for Quark SE and D2000, we can have below file which implements the API functionality using the AON Counter and AON Periodic Timer:

drivers/quark_aon_counter.c

Comment, feedback welcome.

On Fri, Feb 26, 2016 at 5:39 PM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

This is great stuff, it's totally worth working on porting Zephyr for STM32.

I actually recently got the Nucleo boards F103, F030 and F334 as I was
planning to
play with Zephyr on it. I'll try your branch on it and I'll take some time
to review it.
(as soon as I get some time)

The problem with the dev board that I used is that it's not really
available anymore. To the best of my knowledge, the unit I have is an
older version of this one:
https://kamami.pl/zestawy-uruchomieniowe-stm32/187712-eb-stm3206.html?search_query=stm32f103vet6&results=3
However, since there are some apparent differences in the pinout and
since I don't really have the schematics, I'll have to replace it with
something more usable. We have some Discovery (STM32F3, STM32F4) and
Nucleo (F030R8, maybe one F103) kits at the office, I guess one of
these will be my next target.

As for the code, there's really not much of it. I've typed in the
structs for RCC (Reset & Clock Control), GPIOs and UART register
mappings and wired that up.

BR,
--
Maciek Borzecki

Hi Maciek,

This is great stuff, it's totally worth working on porting Zephyr for STM32.

I actually recently got the Nucleo boards F103, F030 and F334 as I was
planning to
play with Zephyr on it. I'll try your branch on it and I'll take some
time to review it.
(as soon as I get some time)

M0, M0+, M4... all these are relevant for Zephyr I would say. I don't
know if anybody
is already working around though.

Br,

Tomasz

Hi Maciek,

I've done a PoC port of Zephyr to STM32F103VE MCU on a EB-STM_06 dev
board. The port is rather minimal, most of the configuration is
hardcoded. The MCU is configured to use internal HSI clock source, with
PLL feeding a 36MHz clock to SYSCLK and AHB/APB (tried to make it board
agnostic). The minimal UART driver assumes that USART1 will be used with
a fixed speed of 9600kbps. The the best of my knowledge, this should be
enough as a starting point for porting to other boards.

The source code lives here:
https://github.com/bboozzoo/zephyr/tree/bboozzoo/stm32f103 in
bboozzoo/stm32f103 branch.

Obligatory video: https://goo.gl/photos/AGhfXYvv8CswjRRu7 (sorry for the
background sound and overall potato quality). In the vide I flash the
firmware through STLink, the board resets and starts the hello world
sample. The output is received via a USB RS232 adapter. The sample
program has been also modified to print an increasing loop counter, just
to make sure we're not looping the same content.

If anyone feels like this is worth working on I might be able to try and
clean it up a little bit and push out to Gerrit (once I get my access
issues resolved). I also have a couple of boards with STM32F3xx and
STM32L4xx MCUs so I might give these a try, unfortunately all of these
are Cortex-M4 based.

Very cool!

Cortex-M4 should work as well, since the M4 is a superset of the M3. The
first board we supported is the Freescale FRDM-K64F, which is an M4.

Has anyone looked at ports to Cortex-M0?

For M0, you have to do an arch port as well. The M3 arch port was done
by using a couple of features that are not available on M0, basically
BASEPRI for interrupt locking instead of PRIMASK and some Thumb2
instructions that are not available on M0.

That said, it might not be too hard to make the M3 arch M0-capable, we
just haven't had the time to look very deep into it.

Cheers,
Ben

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

Hi list,

I've done a PoC port of Zephyr to STM32F103VE MCU on a EB-STM_06 dev
board. The port is rather minimal, most of the configuration is
hardcoded. The MCU is configured to use internal HSI clock source, with
PLL feeding a 36MHz clock to SYSCLK and AHB/APB (tried to make it board
agnostic). The minimal UART driver assumes that USART1 will be used with
a fixed speed of 9600kbps. The the best of my knowledge, this should be
enough as a starting point for porting to other boards.

The source code lives here:
https://github.com/bboozzoo/zephyr/tree/bboozzoo/stm32f103 in
bboozzoo/stm32f103 branch.

Obligatory video: https://goo.gl/photos/AGhfXYvv8CswjRRu7 (sorry for the
background sound and overall potato quality). In the vide I flash the
firmware through STLink, the board resets and starts the hello world
sample. The output is received via a USB RS232 adapter. The sample
program has been also modified to print an increasing loop counter, just
to make sure we're not looping the same content.

If anyone feels like this is worth working on I might be able to try and
clean it up a little bit and push out to Gerrit (once I get my access
issues resolved). I also have a couple of boards with STM32F3xx and
STM32L4xx MCUs so I might give these a try, unfortunately all of these
are Cortex-M4 based.

Has anyone looked at ports to Cortex-M0?

Cheers,
--
Maciek Borzecki

Hi,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

On 16-02-25 02:05 PM, Vinicius Costa Gomes wrote:

Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?

Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.

I agree, Galileo is a special case and it's initialization code may/will
share enough code with development part. I think, pinmux_galileo_priv.c
will implement those common functions, am I right?

Exactly, from what I can see, it will be only one common function.

The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).

At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)

As far as I see, FRDM k64f looks like something in between Galileo and
Quark SE in terms of complexity.

I see. I wouldn't like that these _priv.{c,h} idiom would become the
norm, but two instances may be acceptable. If there are more cases like
this, I would consider providing a pinmux library, as was suggested
earlier in the thread, but this is better left for when this becomes a
real issue.

- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".

We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.

The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.

Agreed,
Regards,
Dmitriy

Cheers,
--
Vinicius

Cheers,
--
Vinicius

Problem: there are two similar mechanisms in nanokernel: nano_timers and
nano_timeouts. In order to optimize codeand space it's better to have one
mechanism for both.

Proposal: Implement nano_timer through timeout mechanism. This aproach
allows to get rid of heavier nanokernel lifo component in favor of more
lightweight nanokernel timeout implementation.

The _nano_timeout structure gets two additional arguments: void
*user_data_ptr and bool expired, the flag that is set to true when the
timeout is expired.
_nano_timeout structure now can be safely renamed to nano_timer, as it
has everything required by nano_timer API.
In this case, if a function like fiber_sleep() is invoked, the
user_data_ptr is set to NULL.
For the nano_timer API user_data_ptr is used to store the user data
pointer. When the nano_timer_init() function is called, it gets the
_nano_timer structure and initializes it as before. On the
nano_timer_start() call the nano_timer structure is being added to the
sorted list via _nano_timeout_add() function.
nano_timer_test() checks the expired flag of the nano_timer structure.
if it is true, the function just returns the user_data_ptr, otherwise,
if the timeout_in_ticks is not TICKS_NONE, it puts the fiber or task to
sleep by calling _Swap().

Question: is it needed to wait for a specified timeout, if
timeout_in_ticks argument is neither TICKS_NONE nor TICKS_UNLIMITED?

_nano_timeout_handle_one_timeout(), in turn, sets the expired flag to
true and if fiber or task is waiting, marks it runnable.
fiberRtnValueSet is used to set the return value to user_data_ptr.


Problem: in microkernel system clock ticks are processed in the k_server
fiber, which does not allow using timeouts during the initialization
before k_server fiber starts.

In order to allow using timeouts during the system initialization
_do_sys_clock_tick_announce pointer has to be modified to point at the
_nano_sys_clock_tick_announce() until the microkernel _k_server() fiber
starts. This way the _do_sys_clock_tick_announce may be initialized as a
pointer to
_nano_sys_clock_tick_announce() routine by default.

This modification needs to be done with changing priority of the driver
initialization routines that use timeout. Such routines need to be
invoked not on PRIMARY, but on NANOKERNEL initialization level.

--
Dmitriy Korovkin

On 16-02-25 02:05 PM, Vinicius Costa Gomes wrote:

Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?

Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.

I agree, Galileo is a special case and it's initialization code may/will
share enough code with development part. I think, pinmux_galileo_priv.c
will implement those common functions, am I right?

The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).

At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)

As far as I see, FRDM k64f looks like something in between Galileo and
Quark SE in terms of complexity.

- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".

We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.

The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.

Agreed,
Regards,
Dmitriy

Cheers,
--
Vinicius

Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?

Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.

The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).

At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)

- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".

We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.

The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.


Cheers,
--
Vinicius

It would be good to have design changes for pinmux outlined in one text. Are we talking about code consolidation for Quark SE/D2000 or the global change for all pinmuxes? Are we touching the part referred as "PINMUX_DEV"?
If we are just changing code for two SoCs, I have no problem with it at all. If we are changing the architecture, we really need the new approach clean, as the following concerns come up:
- pinmuxing may be implemented on a SoC or on a board level (Quark SE vs. Galileo), as well as other architectures may implement it on a different way (FRDM k64F, which is coming).
- reconfiguring pinmux from an application is not usual practice, but quite possible. Cutting this functionality off may make problems in case of "we have this pinmux configuration by default, but for this particular sample project we need to change it".
- pinmux_initialize() may and sometimes should not be implemented by calling set(), get() and other API functions, but implemented individually, just not to slow down the booting process.

Hi all,

Quoting Vinicius Costa Gomes (2016-02-24 18:34:16)

After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.

I like it.

The only thing I am thinking about is polluting the drivers/pinmux/
directory when there are more than a couple of board variants. But this
may not be a problem.

I would only propose to have a different directory for the 'pinmux_dev'
driver. From what I understand, there would be no code shared between
the pinmux "board" driver and the pinmux "dev" driver (the only
information needed seems to be what is already provided by
'include/pinmux.h').

Yes, I also like this proposal and I think we should move with it instead
of with the proposal I did.

I have just one comment regarding the 'pinmux board driver'. The way I see
it, what we are calling 'pinmux board driver' is more like a board-specific
initialization code than a driver per se. And, as a board-specific code, I
think we should land it in board/.

For 'pinmux dev driver', yes, it totally make sense to me that we should
land them in driver/ and the user can build it if his/her *application*
requires it.

Regards,

Andre

On 02/25/2016 07:47 AM, Benjamin Walsh wrote:

On Thu, Feb 25, 2016 at 07:05:49AM -0800, Dirk Brandewie wrote:

On 02/24/2016 02:13 PM, Saucedo Tejada, Genaro wrote:

From 9baee79d211bfb94aeed970c55f31cd3c4b2a8ad Mon Sep 17 00:00:00 2001
From: Genaro Saucedo Tejada <genaro.saucedo.tejada(a)intel.com>
Date: Fri, 19 Feb 2016 17:10:28 -0600
Subject: [PATCH] Log macros unified in a common API

Introduces a header to concentrate logging macro definitions for all
code to
reuse, change aims to provide all currently existing logging
functionality so
every C file can replace it's compile-unit definitions by common code.

Later enhancements to log can now be performed in a single file.

Features:
* Optional printing of thread pointer
* Optional printing of colored messages
* Optional printing of a label to indicate logging level (info, error,
warning, debug)
* Caller function name printing
* Incremental log levels
* One point log disable

I like this in general we need a common set of debug output macros
that all drivers can use. Currently we have a *bunch* of different
versions of the same thing in use throughout the driver tree.

I don't like the naming, we already have a kernel event logging API
people may assume that the two facilities are connected based on the name.

Can we change the names to debug_* or something else?

Again, do we want to take over _another_ namespace ? Maybe it should be
sys_debug or sys_dbg or sys_dbg_log ?

The sample implementation is adding all these in logging.h, which is a
public API file:

DBG, ERR, INF, WRN, THREAD_FN_CALL, IS_LOGGING_ACTIVE

which are very generic names. Don't forget that the application writer
is operating in the same C namespace, and we have been trying to keep
the namespaces the kernel owns to a minimum to minimize the number or
potential clashes:

It's the MACRO names the need fixed up IMO. Any functions that are
added should be in the sys_* space sure. I didn't see any new C
functions being added.

See doc/collaboration/code/naming_conventions.rst.

On Thu, Feb 25, 2016 at 07:05:49AM -0800, Dirk Brandewie wrote:

On 02/24/2016 02:13 PM, Saucedo Tejada, Genaro wrote:

From 9baee79d211bfb94aeed970c55f31cd3c4b2a8ad Mon Sep 17 00:00:00 2001
From: Genaro Saucedo Tejada <genaro.saucedo.tejada(a)intel.com>
Date: Fri, 19 Feb 2016 17:10:28 -0600
Subject: [PATCH] Log macros unified in a common API

Introduces a header to concentrate logging macro definitions for all
code to
reuse, change aims to provide all currently existing logging
functionality so
every C file can replace it's compile-unit definitions by common code.

Later enhancements to log can now be performed in a single file.

Features:
* Optional printing of thread pointer
* Optional printing of colored messages
* Optional printing of a label to indicate logging level (info, error,
warning, debug)
* Caller function name printing
* Incremental log levels
* One point log disable

I like this in general we need a common set of debug output macros
that all drivers can use. Currently we have a *bunch* of different
versions of the same thing in use throughout the driver tree.

I don't like the naming, we already have a kernel event logging API
people may assume that the two facilities are connected based on the name.

Can we change the names to debug_* or something else?

Again, do we want to take over _another_ namespace ? Maybe it should be
sys_debug or sys_dbg or sys_dbg_log ?

The sample implementation is adding all these in logging.h, which is a
public API file:

DBG, ERR, INF, WRN, THREAD_FN_CALL, IS_LOGGING_ACTIVE

which are very generic names. Don't forget that the application writer
is operating in the same C namespace, and we have been trying to keep
the namespaces the kernel owns to a minimum to minimize the number or
potential clashes:

See doc/collaboration/code/naming_conventions.rst.

On 02/24/2016 01:26 PM, Thomas, Ramesh wrote:

On Mon, 2016-02-22 at 07:39 -0800, Dirk Brandewie wrote:

On 02/19/2016 05:31 PM, Thomas, Ramesh wrote:

On Fri, 2016-02-19 at 10:05 -0800, Daniel Leung wrote:

On Fri, Feb 19, 2016 at 08:31:48AM -0800, Dirk Brandewie wrote:

On 02/18/2016 03:04 PM, Daniel Leung wrote:

On Thu, Feb 18, 2016 at 05:54:56PM -0500, Benjamin Walsh wrote:

My take on it is that for Zephyr a failed device initialization
should be considered a fatal event. My expectation is that the
Zephyr user will only be enabling relevant (and important) devices to
their project. If one of these devices should fail, then that is a
serious system error and _NanoFatalErrorHandler() should be invoked.

If this train of thought holds up to scrutiny, and if the aim is to
save a few bytes then I would think that it would be better to have
the device initialization routines return a failure code and have
_sys_device_do_config_level() check for it and invoke the fatal error
handler upon the detection of failure. Otherwise we duplicate the
overhead of calling the fatal error handler in each device
initialization routine.

Sorry for the slow response. I agree with Peter here I think we should
be checking the return value and doing something useful with the
result. Maybe not _NanoFatalErrorHandler() but something notifying
the application that something bad happened. A given device not
initializing may not be fatal to the the whole application, just one
feature is currently unavailable.

For the kind of systems we are targeting, do we really expect the
application to handle devices not initializing correctly, being designed
so that parts are disabled if some parts of the initialization fail
(devices or others), or do we expect applications to require everything
to be present for them to function correctly ? I would have thought the
latter, but I can be convinced.

Delving into the realm of the hypothetical :-)

What about devices that have drivers in the system but are not present
(pluggable) or can't initialize because some resource external to the
device can't be contacted (network server).

The application may be able to still do useful work albeit with reduced
functionality.

Then, if the latter, do we expect the application catching the errors at
runtime when deployed or during development (basically catching software
errors mostly) not malfunctionning hardware. Here, I was thinking the
latter as well, which is why I was proposing __ASSERT() calls catching
initialization errors in debug loads only. And this fits with one of the
core values of the OS, which is small footprint.

Both models are useful for different reasons :-D

Any of those could be a valid approach I think, but we have to decide on
one. And right now, we have the worst since we return those error codes
which are meant for runtime handling, but they just go into the void.

Agreed we need to pick and stay with it for some amount of time until
we see a few real uses/applications/platforms.

OK, following your proposal below, what we could put in place is
standardizing on error codes that init routines must return if they want
the kernel init system to automatically trigger a fatal error.

Then, we could also allow configuring out the error handling if someone
needs to squeeze that last amount of space. One more Kconfig option! :)
The error handling would be enabled by default of course.

For those non-fatal errors, what should we do for runtime driver behaviors?
Should the drivers themselves fail API calls? Or should we let
device_get_binding() return NULL?

How about something like this:

diff --git a/kernel/nanokernel/device.c b/kernel/nanokernel/device.c
index f86f95f..82774c4 100644
--- a/kernel/nanokernel/device.c
+++ b/kernel/nanokernel/device.c
@@ -58,7 +58,7 @@ struct device *device_get_binding(char *name)
struct device *info;

for (info = __device_init_start; info != __device_init_end; info++) {
- if (!strcmp(name, info->config->name)) {
+ if (!strcmp(name, info->config->name) && info->driver_api) {
return info;
}
}

if there is a non-fatal error the driver will not mark itself available
and we can get rid of the null checks in the API headers since the user of the
driver will never get a reference to it if init() did not complete successfully

Why not create a dedicated status field in device object?

It would take more RAM :-) Not sure exactly what you would want to
return in the status? If device init() fails there is nothing the
application code can do about it other that fail gracefully.

I do agree with you that the app cannot do much in failures. But then
kernel need not do even what you propose with failing binding.
Let the app figure out when it tries to use the port.

If get binding fails the app will know the device failed or is
otherwise unavailable. The return code from init() tells the kernel
whether it's a fatal error and to call _NanoFatalErrorHandler() or
continue initialization.

Also drivers can provide
apis for diagnosis if they need e.g. applications that can be remotely
diagnosed and fixed.

Additionally we can provide an API to query status of devices. This
will help query status of devices it does not bind to.

If you are not binding to a device why do you need the status? If
there is a good use case propose the API with the patch ;-)

The API proposal was based on your proposal using get_binding(). Binding
will not work for devices that do not have a name e.g. loapic/ioapic.

What do you need to control from the application in loapic/ioapic?

But, I guess first we need to see if there is a real need for an error
logging mechanism with kernel involvment :-) It should be noted that
some error logging can be achieved by driver api itself (without kernel
help) if the application needs.

This should work. We need to rework some of the drivers as they make
the driver_api assignment unconditionally.


----------
Daniel Leung

On 02/24/2016 02:13 PM, Saucedo Tejada, Genaro wrote:

From 9baee79d211bfb94aeed970c55f31cd3c4b2a8ad Mon Sep 17 00:00:00 2001
From: Genaro Saucedo Tejada <genaro.saucedo.tejada(a)intel.com>
Date: Fri, 19 Feb 2016 17:10:28 -0600
Subject: [PATCH] Log macros unified in a common API

Introduces a header to concentrate logging macro definitions for all
code to
reuse, change aims to provide all currently existing logging
functionality so
every C file can replace it's compile-unit definitions by common code.

Later enhancements to log can now be performed in a single file.

Features:
* Optional printing of thread pointer
* Optional printing of colored messages
* Optional printing of a label to indicate logging level (info, error,
warning, debug)
* Caller function name printing
* Incremental log levels
* One point log disable

I like this in general we need a common set of debug output macros
that all drivers can use. Currently we have a *bunch* of different
versions of the same thing in use throughout the driver tree.

I don't like the naming, we already have a kernel event logging API
people may assume that the two facilities are connected based on the name.

Can we change the names to debug_* or something else?

Change-Id: Ibe542d69912778703bb43c7afef0889f0591ba6a
Signed-off-by: Genaro Saucedo Tejada <genaro.saucedo.tejada(a)intel.com>
---
drivers/grove/lcd_rgb.c | 14 +---
include/bluetooth/log.h | 28 +++----
include/logging.h | 166
++++++++++++++++++++++++++++++++++++++++
misc/debug/Kconfig | 66 ++++++++++++++++
net/bluetooth/Kconfig | 3 +-
net/bluetooth/hci_core.c | 5 --
tests/bluetooth/tester/prj.conf | 2 +
7 files changed, 250 insertions(+), 34 deletions(-)
create mode 100644 include/logging.h

diff --git a/drivers/grove/lcd_rgb.c b/drivers/grove/lcd_rgb.c
index 58b86c5..7ef6a78 100644
--- a/drivers/grove/lcd_rgb.c
+++ b/drivers/grove/lcd_rgb.c
@@ -29,17 +29,9 @@
#define GROVE_LCD_DISPLAY_ADDR (0x3E)
#define GROVE_RGB_BACKLIGHT_ADDR (0x62)

-#ifndef CONFIG_GROVE_DEBUG
-#define DBG(...) { ; }
-#else
-#if defined(CONFIG_STDOUT_CONSOLE)
-#include <stdio.h>
-#define DBG printf
-#else
-#include <misc/printk.h>
-#define DBG printk
-#endif /* CONFIG_STDOUT_CONSOLE */
-#endif /* CONFIG_GROVE_DEBUG */
+/* module specific logging check */
+#define LOG_THIS_MODULE CONFIG_GROVE_DEBUG
+#include <logging.h>

struct command {
uint8_t control;
diff --git a/include/bluetooth/log.h b/include/bluetooth/log.h
index e47453a..e5abbab 100644
--- a/include/bluetooth/log.h
+++ b/include/bluetooth/log.h
@@ -26,25 +26,19 @@
extern "C" {
#endif

-#if defined(CONFIG_BLUETOOTH_DEBUG_COLOR)
-#define BT_COLOR_OFF "\x1B[0m"
-#define BT_COLOR_RED "\x1B[0;31m"
-#define BT_COLOR_YELLOW "\x1B[0;33m"
-#else
-#define BT_COLOR_OFF ""
-#define BT_COLOR_RED ""
-#define BT_COLOR_YELLOW ""
-#endif
-
#if defined(CONFIG_BLUETOOTH_DEBUG)
+
+/* module specific logging check */
+#define LOG_THIS_MODULE CONFIG_BLUETOOTH_DEBUG_HCI_CORE
+/* specify logging domain */
+#define LOG_DMN "bt"
+#include <logging.h>
+
#include <nanokernel.h>
-#define BT_DBG(fmt, ...) printf("bt: %s (%p): " fmt "\n", __func__, \
- sys_thread_self_get(), ##__VA_ARGS__)
-#define BT_ERR(fmt, ...) printf("bt: %s: %s" fmt "%s\n", __func__, \
- BT_COLOR_RED, ##__VA_ARGS__,
BT_COLOR_OFF)
-#define BT_WARN(fmt, ...) printf("bt: %s: %s" fmt "%s\n", __func__, \
- BT_COLOR_YELLOW, ##__VA_ARGS__,
BT_COLOR_OFF)
-#define BT_INFO(fmt, ...) printf("bt: " fmt "\n", ##__VA_ARGS__)
+#define BT_DBG(fmt, ...) DBG(fmt, ##__VA_ARGS__)
+#define BT_ERR(fmt, ...) ERR(fmt, ##__VA_ARGS__)
+#define BT_WARN(fmt, ...) WRN(fmt, ##__VA_ARGS__)
+#define BT_INFO(fmt, ...) INF(fmt, ##__VA_ARGS__)
#define BT_ASSERT(cond) if (!(cond)) { \
BT_ERR("assert: '" #cond "' failed");
\
}
diff --git a/include/logging.h b/include/logging.h
new file mode 100644
index 0000000..d778da9
--- /dev/null
+++ b/include/logging.h
@@ -0,0 +1,166 @@
+/*
+ * Copyright (c) 2015 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.
+ */
+
+/** @file logging.h
+ * @brief Logging macros.
+ */
+#ifndef __LOGGING_H
+#define __LOGGING_H
+
+#if !defined LOG_THIS_MODULE
+#error DO NOT include this header without defining LOG_THIS_MODULE
+#endif
+
+#if defined CONFIG_USE_COMPILE_LOG && (1 == LOG_THIS_MODULE)
+
+#define IS_LOGGING_ACTIVE 1
+#define THREAD_FN_CALL sys_thread_self_get()
+
+/* decide print func */
+#if defined(CONFIG_STDOUT_CONSOLE)
+#include <stdio.h>
+#define LOG_FN printf
+#else
+#include <misc/printk.h>
+#define LOG_FN printk
+#endif /* CONFIG_STDOUT_CONSOLE */
+
+/* Should use color? */
+#if defined(CONFIG_USE_LOG_COLOR)
+#define LOG_COLOR_OFF "\x1B[0m"
+#define LOG_COLOR_RED "\x1B[0;31m"
+#define LOG_COLOR_YELLOW "\x1B[0;33m"
+#else
+#define LOG_COLOR_OFF ""
+#define LOG_COLOR_RED ""
+#define LOG_COLOR_YELLOW ""
+#endif /* CONFIG_BLUETOOTH_DEBUG_COLOR */
+
+/* Should use log lv labels? */
+#if defined(CONFIG_USE_LOG_LV_TAGS)
+#define LOG_LV_ERR "ERR "
+#define LOG_LV_WRN "WRN "
+#define LOG_LV_INF "INF "
+#define LOG_LV_DBG "DBG "
+#else
+#define LOG_LV_ERR ""
+#define LOG_LV_WRN ""
+#define LOG_LV_INF ""
+#define LOG_LV_DBG ""
+#endif /* CONFIG_USE_LOG_LV_TAGS */
+
+/* Log domain name */
+#if defined LOG_DMN
+#define LOG_DMN_TAG LOG_DMN ":\t"
+#else
+#define LOG_DMN_TAG ""
+#endif /* LOG_DMN */
+
+/* Should thread be printed on log? */
+#if defined CONFIG_PRINT_THREAD_LOG
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t%p: %s" /* label func thread */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, THREAD_FN_CALL, log_color, ##__VA_ARGS__,
color_off)
+#else
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t: %s" /* label func */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, log_color, ##__VA_ARGS__, color_off)
+#endif /* CONFIG_PRINT_THREAD_LOG */
+
+#define LOG_NO_COLOR(log_lv, log_format, ...)
\
+ LOG_FN_CALL(log_lv, "", log_format, "", ##__VA_ARGS__)
+#define LOG_COLOR(log_lv, log_color, log_format, ...)
\
+ LOG_FN_CALL(log_lv, log_color, log_format, LOG_COLOR_OFF,
\
+ ##__VA_ARGS__)
+
+/**
+ * @def INF
+ *
+ * @brief Writes information to log
+ *
+ * @details Lowest logging lv, these messages are not disabled unless
whole
+ * logging gets disabled.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define INF(...) LOG_NO_COLOR(LOG_LV_INF, ##__VA_ARGS__)
+
+#if defined(CONFIG_LOG_LV_ERR) || defined(CONFIG_LOG_LV_WRN) \
+ || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def ERR
+ *
+ * @brief Writes Errors to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_ERR
or higiher,
+ * it's meant to report fatal errors that can't be recovered from.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define ERR(...) LOG_COLOR(LOG_LV_ERR, LOG_COLOR_RED, ##__VA_ARGS__)
+#else
+#define ERR(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_WRN) || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def WRN
+ *
+ * @brief Writes warnings to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_WRN
or higiher,
+ * it's meant to print unexpedted situations that are not necesarily
an error.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define WRN(...) LOG_COLOR(LOG_LV_WRN, LOG_COLOR_YELLOW,
##__VA_ARGS__)
+#else
+#define WRN(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def DBG
+ *
+ * @brief Writes debugging information to log
+ *
+ * @details highest logging level, available if CONFIG_USE_COMPILE_LOG
is
+ * CONFIG_LOG_LV_DBG, it's meant to print developer information.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define DBG(...) LOG_NO_COLOR(LOG_LV_DBG, ##__VA_ARGS__)
+#else
+#define DBG(...) { ; }
+#endif
+
+#else
+#define IS_LOGGING_ACTIVE 0
+/* create dummy macros */
+#define DBG(...) { ; }
+#define ERR(...) { ; }
+#define WRN(...) { ; }
+#define INF(...) { ; }
+
+#endif /*USE_COMPILE_LOG*/
+
+#endif /* __LOGGING_H */
diff --git a/misc/debug/Kconfig b/misc/debug/Kconfig
index 51a3d3f..716a361 100644
--- a/misc/debug/Kconfig
+++ b/misc/debug/Kconfig
@@ -16,6 +16,72 @@
# limitations under the License.
#

+config USE_COMPILE_LOG
+ bool
+ prompt "Enable Logging"
+ default n
+ help
+ Global switch for logging.
+
+config USE_LOG_LV_TAGS
+ bool
+ prompt "Prepend level tags to logs"
+ depends on USE_COMPILE_LOG
+ default y
+ help
+ Log lines will beging with one of INF, ERR, WRN or DBG,
depending on the
+ macro used at C code.
+
+config USE_LOG_COLOR
+ bool
+ prompt "Use colored logs"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Use color in the logs. This requires an ANSI capable
terminal.
+
+config PRINT_THREAD_LOG
+ bool
+ prompt "Include thread on log messages"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Each log line will include the pointer of the thread that
printed it
+
+choice
+depends on USE_COMPILE_LOG
+prompt "Log level"
+
+config LOG_LV_INF
+ bool
+ prompt "Info"
+ help
+ Print just unfiltered logging.
+
+config LOG_LV_ERR
+ bool
+ prompt "Error"
+ help
+ Do not filter printing of ERR logging macro, it is meant to
report fatal
+ errors.
+
+config LOG_LV_WRN
+ bool
+ prompt "Warning"
+ help
+ Do not filter printing of WRN and above logging macros, WRN
marcro is
+ meant to print messages generated under unexpedted
circumstances, not
+ necesarily errors.
+
+config LOG_LV_DBG
+ bool
+ prompt "Debug"
+ help
+ Enable printing of all logging macros, DBG is meant to print
developer
+ information.
+
+endchoice
+

menu "Safe memory access"

diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 8dcbbcf..222199d 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -218,7 +218,8 @@ config BLUETOOTH_DEBUG
serial console.

if BLUETOOTH_DEBUG
-config BLUETOOTH_DEBUG_COLOR
+config BLUETOOTH_DEBUG_COLOR
+ select USE_LOG_COLOR
bool "Use colored logs"
default y
help
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d5b6d24..00ad8d2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -50,11 +50,6 @@
#include "smp.h"
#endif /* CONFIG_BLUETOOTH_SMP */

-#if !defined(CONFIG_BLUETOOTH_DEBUG_HCI_CORE)
-#undef BT_DBG
-#define BT_DBG(fmt, ...)
-#endif
-
/* Stacks for the fibers */
static BT_STACK_NOINIT(rx_fiber_stack,
CONFIG_BLUETOOTH_RX_STACK_SIZE);
static BT_STACK_NOINIT(rx_prio_fiber_stack, 256);
diff --git a/tests/bluetooth/tester/prj.conf
b/tests/bluetooth/tester/prj.conf
index 617e07c..af53ace 100644
--- a/tests/bluetooth/tester/prj.conf
+++ b/tests/bluetooth/tester/prj.conf
@@ -1,5 +1,7 @@
CONFIG_UART_PIPE=y
CONFIG_CONSOLE_HANDLER=y
+CONFIG_USE_COMPILE_LOG=y
+CONFIG_LOG_LV_DBG=y
CONFIG_BLUETOOTH=y
CONFIG_BLUETOOTH_LE=y
CONFIG_BLUETOOTH_CENTRAL=y
--
2.5.0

Dirk Brandewie <dirk.j.brandewie(a)intel.com> writes:

[...]

yeah better idea :-D when the quark_se gets more design wins things
would get crowded.

Now the question is who is signing up to do the refactoring? :-)

I already started this. Hoping to have something to show later today or
tomorrow.

And all concerns that were raised about the increases in ROM size and boot
time overhead seems to be addressed.


Cheers,
--
Vinicius

Cheers,
--
Vinicius

On 02/24/2016 01:34 PM, Vinicius Costa Gomes wrote:

Hi Dirk,

Dirk Brandewie <dirk.j.brandewie(a)intel.com> writes:

[...]

After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.

I like it.

The only thing I am thinking about is polluting the drivers/pinmux/
directory when there are more than a couple of board variants. But this
may not be a problem.

I would only propose to have a different directory for the 'pinmux_dev'
driver. From what I understand, there would be no code shared between
the pinmux "board" driver and the pinmux "dev" driver (the only
information needed seems to be what is already provided by
'include/pinmux.h').

yeah better idea :-D when the quark_se gets more design wins things
would get crowded.

Now the question is who is signing up to do the refactoring? :-)

And all concerns that were raised about the increases in ROM size and boot
time overhead seems to be addressed.


Cheers,
--
Vinicius

Hi Genaro,

The idea is nice, at least it will prevent to declare debug macros
everywhere.

However, split the patch:
- first provide a patch with the logging.h and it's related Kconfig file
- and only then whatever usage of it in further patch.

I have small comments about logging.h and Kconfig:

diff --git a/include/logging.h b/include/logging.h
new file mode 100644
index 0000000..d778da9
--- /dev/null
+++ b/include/logging.h
@@ -0,0 +1,166 @@
+/*
+ * Copyright (c) 2015 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.
+ */
+
+/** @file logging.h
+ * @brief Logging macros.
+ */
+#ifndef __LOGGING_H
+#define __LOGGING_H
+
+#if !defined LOG_THIS_MODULE
+#error DO NOT include this header without defining LOG_THIS_MODULE
+#endif

I wouldn't use LOG_THIS_MODULE also, rather something like LOG_ENABLE
(Ok taste issue here, but a short name is nicer imo)
And I do want to be able to include logging.h even if don't define it.

So instead of the above just do:

#ifdef LOG_ENABLE

and add a relevant #endif below.

Then I could use it like:

#ifdef CONFIG_SPI_DEBUG
#define LOG_ENABLE 1
#endif
#include <logging.h>

It avoids thes superfluous lines
#else
#define LOG_ENABLE 0

+
+#if defined CONFIG_USE_COMPILE_LOG && (1 == LOG_THIS_MODULE)
+
+#define IS_LOGGING_ACTIVE 1
+#define THREAD_FN_CALL sys_thread_self_get()
+
+/* decide print func */
+#if defined(CONFIG_STDOUT_CONSOLE)
+#include <stdio.h>
+#define LOG_FN printf
+#else
+#include <misc/printk.h>
+#define LOG_FN printk
+#endif /* CONFIG_STDOUT_CONSOLE */
+
+/* Should use color? */
+#if defined(CONFIG_USE_LOG_COLOR)
+#define LOG_COLOR_OFF "\x1B[0m"
+#define LOG_COLOR_RED "\x1B[0;31m"
+#define LOG_COLOR_YELLOW "\x1B[0;33m"
+#else
+#define LOG_COLOR_OFF ""
+#define LOG_COLOR_RED ""
+#define LOG_COLOR_YELLOW ""
+#endif /* CONFIG_BLUETOOTH_DEBUG_COLOR */
+
+/* Should use log lv labels? */
+#if defined(CONFIG_USE_LOG_LV_TAGS)
+#define LOG_LV_ERR "ERR "
+#define LOG_LV_WRN "WRN "
+#define LOG_LV_INF "INF "
+#define LOG_LV_DBG "DBG "
+#else
+#define LOG_LV_ERR ""
+#define LOG_LV_WRN ""
+#define LOG_LV_INF ""
+#define LOG_LV_DBG ""
+#endif /* CONFIG_USE_LOG_LV_TAGS */
+
+/* Log domain name */
+#if defined LOG_DMN
+#define LOG_DMN_TAG LOG_DMN ":\t"
+#else
+#define LOG_DMN_TAG ""
+#endif /* LOG_DMN */
+
+/* Should thread be printed on log? */
+#if defined CONFIG_PRINT_THREAD_LOG
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t%p: %s" /* label func thread */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, THREAD_FN_CALL, log_color, ##__VA_ARGS__,
color_off)
+#else
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t: %s" /* label func */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, log_color, ##__VA_ARGS__, color_off)
+#endif /* CONFIG_PRINT_THREAD_LOG */
+
+#define LOG_NO_COLOR(log_lv, log_format, ...)
\
+ LOG_FN_CALL(log_lv, "", log_format, "", ##__VA_ARGS__)
+#define LOG_COLOR(log_lv, log_color, log_format, ...)
\
+ LOG_FN_CALL(log_lv, log_color, log_format, LOG_COLOR_OFF,
\
+ ##__VA_ARGS__)
+
+/**
+ * @def INF
+ *
+ * @brief Writes information to log
+ *
+ * @details Lowest logging lv, these messages are not disabled unless
whole
+ * logging gets disabled.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define INF(...) LOG_NO_COLOR(LOG_LV_INF, ##__VA_ARGS__)
+
+#if defined(CONFIG_LOG_LV_ERR) || defined(CONFIG_LOG_LV_WRN) \
+ || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def ERR
+ *
+ * @brief Writes Errors to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_ERR
or higiher,
+ * it's meant to report fatal errors that can't be recovered from.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define ERR(...) LOG_COLOR(LOG_LV_ERR, LOG_COLOR_RED, ##__VA_ARGS__)
+#else
+#define ERR(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_WRN) || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def WRN
+ *
+ * @brief Writes warnings to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_WRN
or higiher,
+ * it's meant to print unexpedted situations that are not necesarily
an error.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define WRN(...) LOG_COLOR(LOG_LV_WRN, LOG_COLOR_YELLOW,
##__VA_ARGS__)
+#else
+#define WRN(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def DBG
+ *
+ * @brief Writes debugging information to log
+ *
+ * @details highest logging level, available if CONFIG_USE_COMPILE_LOG
is
+ * CONFIG_LOG_LV_DBG, it's meant to print developer information.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define DBG(...) LOG_NO_COLOR(LOG_LV_DBG, ##__VA_ARGS__)
+#else
+#define DBG(...) { ; }
+#endif
+
+#else
+#define IS_LOGGING_ACTIVE 0
+/* create dummy macros */
+#define DBG(...) { ; }
+#define ERR(...) { ; }
+#define WRN(...) { ; }
+#define INF(...) { ; }
+
+#endif /*USE_COMPILE_LOG*/
+
+#endif /* __LOGGING_H */
diff --git a/misc/debug/Kconfig b/misc/debug/Kconfig
index 51a3d3f..716a361 100644
--- a/misc/debug/Kconfig
+++ b/misc/debug/Kconfig
@@ -16,6 +16,72 @@
# limitations under the License.
#

+config USE_COMPILE_LOG

Better something like LOG_COMPILE_TIME
Thus it will ease grep on CONFIG_LOG_* for instance.

+ bool
+ prompt "Enable Logging"

Just do bool "Enable Logging"
No need of the prompt. You can apply that at all options below.

+ default n
+ help
+ Global switch for logging.
+
+config USE_LOG_LV_TAGS

LOG_USE_LV_TAGS

+ bool
+ prompt "Prepend level tags to logs"
+ depends on USE_COMPILE_LOG
+ default y
+ help
+ Log lines will beging with one of INF, ERR, WRN or DBG,
depending on the
+ macro used at C code.
+
+config USE_LOG_COLOR

LOG_USE_COLOR

+ bool
+ prompt "Use colored logs"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Use color in the logs. This requires an ANSI capable
terminal.
+
+config PRINT_THREAD_LOG

LOG_PRINT_THREAD

+ bool
+ prompt "Include thread on log messages"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Each log line will include the pointer of the thread that
printed it
+
+choice
+depends on USE_COMPILE_LOG
+prompt "Log level"
+
+config LOG_LV_INF
+ bool
+ prompt "Info"
+ help
+ Print just unfiltered logging.
+
+config LOG_LV_ERR
+ bool
+ prompt "Error"
+ help
+ Do not filter printing of ERR logging macro, it is meant to
report fatal
+ errors.
+
+config LOG_LV_WRN
+ bool
+ prompt "Warning"
+ help
+ Do not filter printing of WRN and above logging macros, WRN
marcro is
+ meant to print messages generated under unexpedted
circumstances, not
+ necesarily errors.
+
+config LOG_LV_DBG
+ bool
+ prompt "Debug"
+ help
+ Enable printing of all logging macros, DBG is meant to print
developer
+ information.
+
+endchoice
+

In the end we only have to deal with CONFIG_LOG_* prefixed options.

Tomasz

Hi Genaro,

On Thu, Feb 25, 2016 at 12:13 AM, Saucedo Tejada, Genaro
<genaro.saucedo.tejada(a)intel.com> wrote:

From 9baee79d211bfb94aeed970c55f31cd3c4b2a8ad Mon Sep 17 00:00:00 2001
From: Genaro Saucedo Tejada <genaro.saucedo.tejada(a)intel.com>
Date: Fri, 19 Feb 2016 17:10:28 -0600
Subject: [PATCH] Log macros unified in a common API

Introduces a header to concentrate logging macro definitions for all
code to
reuse, change aims to provide all currently existing logging
functionality so
every C file can replace it's compile-unit definitions by common code.

Later enhancements to log can now be performed in a single file.

Features:
* Optional printing of thread pointer
* Optional printing of colored messages
* Optional printing of a label to indicate logging level (info, error,
warning, debug)
* Caller function name printing
* Incremental log levels
* One point log disable

I welcome these changes but Im not so sure logging levels are actually
necessary if one can define different domains, at least in case of
Bluetooth when you turn on debug for one particular domain you just
get everything, if that is too much perhaps the domain needs to be
split in sub-domains, anyway since your changes was based in Bluetooth
and we did not have levels in the first place I assume it is no needed
right now and can be added later provided someone make a case.