Date   

Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Andre Guedes <andre.guedes@...>
 

Hi Jesus,

Quoting Jesus Sanchez-Palencia (2016-02-18 12:42:07)
Just so we avoid mixing two different topics, it looks like this deserves a separate RFC.
Yes. I already had this in mind and I'm going to send the errno.h RFC later
today.

I was basically replying Daniel's comment and took the opportunity to get an
early feedback from him.

Regards,

Andre


Re: RFC: return type of functions passed to DEVICE_INIT()

Dirk Brandewie <dirk.j.brandewie@...>
 

On 02/12/2016 11:38 AM, Mitsis, Peter wrote:
See [Peter]

-----Original Message-----
From: Benjamin Walsh [mailto:benjamin.walsh(a)windriver.com]
Sent: February-12-16 1:37 PM
To: devel(a)lists.zephyrproject.org
Subject: [devel] RFC: return type of functions passed to DEVICE_INIT()

Folks,

For some reason, the signature of functions passed to the DEVICE_INIT()
<init_fn> parameter has a return type of 'int', but the return value is never
checked within _sys_device_do_config_level(). Some init functions do return an
error code, such as the ARC init code and the bluetooth init routines, but that
just gets ignored.

Question: should we have init functions of return type 'void' then ?
That would shave a few bytes in every init function if we don't have to return a
value.
[Peter] - We generally try to operate under the assumption that failures will not occur.
That being said, we do have some instances where we do check for errors, and some of these are considered fatal.

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.

How we could/should report this type of error is an open question :-).



Just my two cents.

Cheers,
Ben

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


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Jesus Sanchez-Palencia <jesus.sanchez-palencia@...>
 

Hi,


On Thu, 18 Feb 2016 09:11:59 -0200
Andre Guedes <andre.guedes(a)intel.com> wrote:

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-17 05:09:07)
DEV_NO_SUPPORT seems to cover the concept. Not sure we need a
DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.
Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.
I think ENOSYS would perfectly match what we want here since it means
"Function not implemented" (as Peter pointed in the other reply).

Speaking of which, I was talking to Dirk in the other day and we both agreed
that it would be a good idea we use errno.h codes instead of DEV_* at the
driver's layer. The main points are 1) errno.h is a well-known error convention
which pretty much all developer is familiar with, 2) errno.h codes address what
we need, 3) no need to create new codes such as DEV_NOT_IMPLEMENTED for instance,
and 4) changing the current drivers to use errno.h codes instead of DEV_* is a
feasible task.

What do you think?
Just so we avoid mixing two different topics, it looks like this deserves a separate RFC.

The original problem I raised on that code review a few weeks ago -- not having a defined return
code to state that a given driver API impl is dummy -- seems to have had some conclusion reached
here already. Looks like adding DEV_NOT_IMPLEMENTED has some appeal in the end. I also support that
meanwhile the errno idea is not discussed.


I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.
Yes, this is the challenge. I volunteer to review and fix what we have
upstream. However, we have to pay attention for this kind of things
while reviewing new patches on Gerrit to ensure they are merged with
the proper return codes.
Guidelines, folks. Let's make sure this is documented somewhere so we can just copy&paste a link on
reviews afterwards and people can refer to it when coding.

Thanks for preparing this RFC, by the way!

Regards,
jesus



Thanks,

Andre


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Andre Guedes <andre.guedes@...>
 

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-17 05:09:07)
DEV_NO_SUPPORT seems to cover the concept. Not sure we need a
DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.
Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.
I think ENOSYS would perfectly match what we want here since it means
"Function not implemented" (as Peter pointed in the other reply).

Speaking of which, I was talking to Dirk in the other day and we both agreed
that it would be a good idea we use errno.h codes instead of DEV_* at the
driver's layer. The main points are 1) errno.h is a well-known error convention
which pretty much all developer is familiar with, 2) errno.h codes address what
we need, 3) no need to create new codes such as DEV_NOT_IMPLEMENTED for instance,
and 4) changing the current drivers to use errno.h codes instead of DEV_* is a
feasible task.

What do you think?

I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.
Yes, this is the challenge. I volunteer to review and fix what we have
upstream. However, we have to pay attention for this kind of things
while reviewing new patches on Gerrit to ensure they are merged with
the proper return codes.

Thanks,

Andre


Re: RFC: return type of functions passed to DEVICE_INIT()

Iván Briano <ivan.briano at intel.com...>
 

On Wed, 17 Feb 2016 14:35:24 -0500, Benjamin Walsh wrote:
For some reason, the signature of functions passed to the
DEVICE_INIT() <init_fn> parameter has a return type of 'int', but
the return value is never checked within
_sys_device_do_config_level(). Some init functions do return an
error code, such as the ARC init code and the bluetooth init
routines, but that just gets ignored.

Question: should we have init functions of return type 'void' then
? That would shave a few bytes in every init function if we don't
have to return a value.
We generally try to operate under the assumption that failures will
not occur. That being said, we do have some instances where we do
check for errors, and some of these are considered fatal.

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.
Drivers' initialization routines can use __ASSERT() if they want to
catch an error in a debug build.
I have not received any other feedback w.r.t. this: silence implies
consent ? :)
Fine by me, on both accounts.


Re: RFC: return type of functions passed to DEVICE_INIT()

Benjamin Walsh <benjamin.walsh@...>
 

For some reason, the signature of functions passed to the
DEVICE_INIT() <init_fn> parameter has a return type of 'int', but
the return value is never checked within
_sys_device_do_config_level(). Some init functions do return an
error code, such as the ARC init code and the bluetooth init
routines, but that just gets ignored.

Question: should we have init functions of return type 'void' then
? That would shave a few bytes in every init function if we don't
have to return a value.
We generally try to operate under the assumption that failures will
not occur. That being said, we do have some instances where we do
check for errors, and some of these are considered fatal.

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.
Drivers' initialization routines can use __ASSERT() if they want to
catch an error in a debug build.
I have not received any other feedback w.r.t. this: silence implies
consent ? :)


Re: [RFC] Simplifying disable of Kconfig Debugging options

Saucedo Tejada, Genaro <genaro.saucedo.tejada@...>
 

Your proposal seems the simplest to me, the only advantage I see on the more complex one is flexibility, presets could provide settings for multiple scenarios rather than on|off.

I would add your observation as a 4th alternative and submit for consideration whether all the complexity previously proposed is worth it.

-----Original Message-----
From: Kalowsky, Daniel
Sent: Wednesday, February 17, 2016 01:07
To: Saucedo Tejada, Genaro <genaro.saucedo.tejada(a)intel.com>; devel(a)lists.zephyrproject.org
Subject: RE: [RFC] Simplifying disable of Kconfig Debugging options

-----Original Message-----
From: Saucedo Tejada, Genaro [mailto:genaro.saucedo.tejada(a)intel.com]
Sent: Friday, February 12, 2016 8:51 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] [RFC] Simplifying disable of Kconfig Debugging
options

Hello, please check this proposal and provide feedback if possible.

Background:

Currently several debugging options exists on Kconfig menu but they
are not on a single debug submenu, e.g.:
- /Networking/Bluetooth support/Bluetooth debug support/*
- /Networking/Generic networking support/Network Ethernet driver debug
- /x86 architecture/Allows the usage of GDB with the ARC processor
- /Debugging Options/*
This means changing many debug settings at once becomes difficult and
might led to developer overlooking some options.

Disabling all the debug features at once could be a frequent task when
the footprint size is constantly kept in mind during development. If
further debugging is required later, the process to re-enable several
features will follow.

Consequently we want to make toggle of many debug settings at once as
straight forward as possible.

Proposal to simplify the switching of multiple debug options at once:
Out of these proposals, it's not really clear to me how any of them would be more simple than just putting the debug options you are interested in to a PRJ file, and using that to {un}set the value at compile time.

Could you clarify why your proposals would work better?



1st approach: Original proposal suggested moving all options to a
single menu, this will minimize the navigation between submenus.

This alternative was ruled out as the debug options taken out of
original context won't let the developer know the details of the feature.

Duplicating options instead of removing them from the original context
would complicate the menu and learning to use or to modify it.

2nd approach: Group features on debug levels and differentiate the
debug features (both build and run time) from printing/logging
features. First this alternative separates printing functionality from
actual debugging features, printing would include current printk and
printf related options plus possible more complex logging features to be developed in the future.

Regarding debugging features these could be classified in multiple
levels instead of a single global switch, by reducing the debug level
a developer could decrease the foot print in a simple manner while
keeping logging/printing.

Several levels could be defined or additional categories, as an
example the following menu specifies two debug levels (1st and 4th
options) and a single additional category (printing/logging):

[*] Build kernel with debugging
[*] Generate stack usage information
[ ] Enable printing
[ ] Enable run time debugging features

By disabling "Enable run time debugging features" the most complex
features get disabled. Although not visible from '/Debugging Options'
some network debug options would be disabled, some build time options
and some low footprint impact options would remain available, logging
category would also be available. The next level (1st option) disables
all debugging options but those in drivers, as drivers usually have
very diverse implementation, logging category would still be available.

Additional categories could be network and drivers but it cannot be
guaranteed/enforced that new driver developers will follow such
pattern on Kconfig files.

Additional debug levels could produce a finer grain classification
regarding foot print and other drawbacks such as performance degradation.

It still remains an open issue turning on several options, as SELECT
on Kconfig is problematic and maybe original intent was not to turn it
all on but turn the previous selection on. Developer would have to
back up his last debugging '.config' file to re-enable.

3rd approach: as an extreme alternative, in case modifying Kconfig
menus is not viable, would be externally dealing with '.config' files.
using developer triggered scripts like merge_config.sh already part of
make build process.

Developers would be able to specify debug presets using a new option
in the per-application Makefile or passing a make command argument: (make ...
DEBUG_PRESET='network2.conf kernel.conf').

Presets already on the repository could be used and developers could
define their own.

Currently make script merges 3 configuration files: board, nanokernel
XOR microkernel and project specific conf. Additional argument would
be added to the merge command, these arguments being the developer
selected debug presets.

An example preset with some variables for illustrative purpose:

$ cat network2.conf
#Level 2 network debugging preset
#prerequisite settings
NETWORKING_WITH_LOGGING=y
STDOUT_CONSOLE=y
NET_BUF_DEBUG=y
#debug settings
NETWORKING_STATISTICS=y
NETWORKING_DEBUG_UART=y
ETHERNET_DEBUG=y
TINYDTLS_DEBUG=y
NET_BUF_DEBUG=y
BLUETOOTH_DEBUG=y
BLUETOOTH_DEBUG_COLOR=y
BLUETOOTH_DEBUG_HCI_CORE=n
BLUETOOTH_DEBUG_CONN=y
BLUETOOTH_DEBUG_KEYS=n
BLUETOOTH_DEBUG_L2CAP=y
BLUETOOTH_SMP_SELFTEST=y
BLUETOOTH_DEBUG_ATT=n
BLUETOOTH_DEBUG_GATT=n


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Kalowsky, Daniel <daniel.kalowsky@...>
 

-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Friday, February 12, 2016 8:34 AM
To: Kalowsky, Daniel <daniel.kalowsky(a)intel.com>;
devel(a)lists.zephyrproject.org
Subject: [devel] Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-12 12:53:49)
Early on when we started the driver development, the suspend/resume
functions were developed with the DEV_OK because as far as any OSPM
policy is concerned, there should be no reason these devices would stop it.
This was an not so nice way into fooling that it has happened.

Thanks for the background explanation.

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a
DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.

Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think the error
code DEV_INVALID_OP might be more suitable.
Good point. You're alternate point (in another post) of ENOSYS not matching is also valid.

I think you've convinced me. The challenge now is to make sure it's used appropriately. As we've shown, we'll need to be explicit with why some functions actually need to return DEV_OK or whatever vs DEV_NO_SUPPORT.


Anyways, if we move forward with DEV_NO_SUPPORT, I think we should fix
the comment in include/device.h since it looks a bit misleading. See below:

--[cut]--
#define DEV_NO_SUPPORT 6 /* Device type not supported */
--[cut]--

Thanks,

Andre


Re: [RFC] Simplifying disable of Kconfig Debugging options

Kalowsky, Daniel <daniel.kalowsky@...>
 

-----Original Message-----
From: Saucedo Tejada, Genaro [mailto:genaro.saucedo.tejada(a)intel.com]
Sent: Friday, February 12, 2016 8:51 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] [RFC] Simplifying disable of Kconfig Debugging options

Hello, please check this proposal and provide feedback if possible.

Background:

Currently several debugging options exists on Kconfig menu but they are not
on a single debug submenu, e.g.:
- /Networking/Bluetooth support/Bluetooth debug support/*
- /Networking/Generic networking support/Network Ethernet driver debug
- /x86 architecture/Allows the usage of GDB with the ARC processor
- /Debugging Options/*
This means changing many debug settings at once becomes difficult and
might led to developer overlooking some options.

Disabling all the debug features at once could be a frequent task when the
footprint size is constantly kept in mind during development. If further
debugging is required later, the process to re-enable several features will
follow.

Consequently we want to make toggle of many debug settings at once as
straight forward as possible.

Proposal to simplify the switching of multiple debug options at once:
Out of these proposals, it's not really clear to me how any of them would be more simple than just putting the debug options you are interested in to a PRJ file, and using that to {un}set the value at compile time.

Could you clarify why your proposals would work better?



1st approach: Original proposal suggested moving all options to a single
menu, this will minimize the navigation between submenus.

This alternative was ruled out as the debug options taken out of original
context won't let the developer know the details of the feature.

Duplicating options instead of removing them from the original context
would complicate the menu and learning to use or to modify it.

2nd approach: Group features on debug levels and differentiate the debug
features (both build and run time) from printing/logging features. First this
alternative separates printing functionality from actual debugging features,
printing would include current printk and printf related options plus possible
more complex logging features to be developed in the future.

Regarding debugging features these could be classified in multiple levels
instead of a single global switch, by reducing the debug level a developer
could decrease the foot print in a simple manner while keeping
logging/printing.

Several levels could be defined or additional categories, as an example the
following menu specifies two debug levels (1st and 4th
options) and a single additional category (printing/logging):

[*] Build kernel with debugging
[*] Generate stack usage information
[ ] Enable printing
[ ] Enable run time debugging features

By disabling "Enable run time debugging features" the most complex
features get disabled. Although not visible from '/Debugging Options' some
network debug options would be disabled, some build time options and
some low footprint impact options would remain available, logging category
would also be available. The next level (1st option) disables all debugging
options but those in drivers, as drivers usually have very diverse
implementation, logging category would still be available.

Additional categories could be network and drivers but it cannot be
guaranteed/enforced that new driver developers will follow such pattern on
Kconfig files.

Additional debug levels could produce a finer grain classification regarding
foot print and other drawbacks such as performance degradation.

It still remains an open issue turning on several options, as SELECT on Kconfig
is problematic and maybe original intent was not to turn it all on but turn the
previous selection on. Developer would have to back up his last debugging
'.config' file to re-enable.

3rd approach: as an extreme alternative, in case modifying Kconfig menus is
not viable, would be externally dealing with '.config' files.
using developer triggered scripts like merge_config.sh already part of make
build process.

Developers would be able to specify debug presets using a new option in the
per-application Makefile or passing a make command argument: (make ...
DEBUG_PRESET='network2.conf kernel.conf').

Presets already on the repository could be used and developers could define
their own.

Currently make script merges 3 configuration files: board, nanokernel XOR
microkernel and project specific conf. Additional argument would be added
to the merge command, these arguments being the developer selected
debug presets.

An example preset with some variables for illustrative purpose:

$ cat network2.conf
#Level 2 network debugging preset
#prerequisite settings
NETWORKING_WITH_LOGGING=y
STDOUT_CONSOLE=y
NET_BUF_DEBUG=y
#debug settings
NETWORKING_STATISTICS=y
NETWORKING_DEBUG_UART=y
ETHERNET_DEBUG=y
TINYDTLS_DEBUG=y
NET_BUF_DEBUG=y
BLUETOOTH_DEBUG=y
BLUETOOTH_DEBUG_COLOR=y
BLUETOOTH_DEBUG_HCI_CORE=n
BLUETOOTH_DEBUG_CONN=y
BLUETOOTH_DEBUG_KEYS=n
BLUETOOTH_DEBUG_L2CAP=y
BLUETOOTH_SMP_SELFTEST=y
BLUETOOTH_DEBUG_ATT=n
BLUETOOTH_DEBUG_GATT=n


RFC: make _fiber_start() return a handle on the fiber

Benjamin Walsh <benjamin.walsh@...>
 

Folks,

When we start a fiber via the _fiber_start() API family, we don't get
back a handle on the created fiber. The fiber identifier is actually the
start of the fiber's stack. This hasn't been a problem until now since
no API requires a handle on the fiber, except one,
fiber_delayed_start_cancel(): that API is part of a pair, where the
other API, fiber_delayed_start() starts the fiber and returns a handle.

However, Jukka asked me an API could be created that cancels a
fiber_sleep() call, something like fiber_wakeup(). The implementation of
such an API is very simple, but it requires a handle on the fiber we
want to wake up. This in turn requires the signature of the
_fiber_start() family to return a handle to the fiber that gets started.

The signature of _fiber_start() et al. would then change from a void
return type to a void * return type.

Objections, comments, etc ?

Cheers,
Ben

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


UART on Minnowboard

Szymon Janc <ext.szymon.janc@...>
 

Hi,

I have a problem with minnowboard and it's uart (I use btshell sample for
testing and trying to communicate with Bluetooth module using H5 protocol).

The problem is, that there is no transmission on the TX pin of the minnowboard
(checked with logic analyzer) and no matter if the bt module is connected or
not. What is more, the console uart is working perfectly.

I was connecting CTS/RTS pins to module, or to ground, or to nothing in
different configurations - no effect.
Changed supporting of flow control in UEFI - no effect.
Also tried changing UART_1 address and IRQ in Zephyr Kconfig to:
- Port: 0x02f8, IRQ: 3
- Port: 0x03e8, IRQ: 4
- Port: 0x02e8, IRQ: 3
And also no effect.

Minnowboard isn't broken or something, all uarts work with Ubuntu, and almost
all with Fedora (couldn't open only UART1, rest was fine). BT modules are also
fine, they work on minnowboard with ftdi usb/serial converter with Fedora and
Ubuntu.

Any advise on getting UART_1 working with zephyr?

--
BR
Szymon Janc


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Andre Guedes <andre.guedes@...>
 

Hi Dirk,

Quoting Dirk Brandewie (2016-02-12 13:32:31)
In the new model the functions would be required to be implemented, the
RFC included a null implementation for drivers to use if they had no
work to do during suspend/resume. Still a topic for discussion but the
null functions can just return success meaning I have done everything
I need to or can do :-).
Yes, sure. If we can use this 'null' implementation in drivers that don't
properly implement the suspend/resume callbacks (yet), this will fix the
issue I described in the other email.

However, the suspend/resume callbacks were just an example to bring the
discussion about having a "not implemented"-wise error code in device.h.
I didn't review all drivers, but it seems such error code would be suitable
in other situations like the interrupt-related callbacks from the
gpio_pcal9535a.c driver. These callbacks are not implemented yet and we
use DEV_INVALID_OP.

Thanks,

Andre


Re: RFC: return type of functions passed to DEVICE_INIT()

Benjamin Walsh <benjamin.walsh@...>
 

For some reason, the signature of functions passed to the DEVICE_INIT()
<init_fn> parameter has a return type of 'int', but the return value is never
checked within _sys_device_do_config_level(). Some init functions do return an
error code, such as the ARC init code and the bluetooth init routines, but that
just gets ignored.

Question: should we have init functions of return type 'void' then ?
That would shave a few bytes in every init function if we don't have to return a
value.
We generally try to operate under the assumption that failures will
not occur. That being said, we do have some instances where we do
check for errors, and some of these are considered fatal.

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.
Drivers' initialization routines can use __ASSERT() if they want to catch
an error in a debug build.


Re: RFC: return type of functions passed to DEVICE_INIT()

Mitsis, Peter <Peter.Mitsis@...>
 

See [Peter]

-----Original Message-----
From: Benjamin Walsh [mailto:benjamin.walsh(a)windriver.com]
Sent: February-12-16 1:37 PM
To: devel(a)lists.zephyrproject.org
Subject: [devel] RFC: return type of functions passed to DEVICE_INIT()

Folks,

For some reason, the signature of functions passed to the DEVICE_INIT()
<init_fn> parameter has a return type of 'int', but the return value is never
checked within _sys_device_do_config_level(). Some init functions do return an
error code, such as the ARC init code and the bluetooth init routines, but that
just gets ignored.

Question: should we have init functions of return type 'void' then ?
That would shave a few bytes in every init function if we don't have to return a
value.
[Peter] - We generally try to operate under the assumption that failures will not occur.
That being said, we do have some instances where we do check for errors, and some of these are considered fatal.

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.

Just my two cents.

Cheers,
Ben

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


RFC: return type of functions passed to DEVICE_INIT()

Benjamin Walsh <benjamin.walsh@...>
 

Folks,

For some reason, the signature of functions passed to the DEVICE_INIT()
<init_fn> parameter has a return type of 'int', but the return value is
never checked within _sys_device_do_config_level(). Some init functions
do return an error code, such as the ARC init code and the bluetooth
init routines, but that just gets ignored.

Question: should we have init functions of return type 'void' then ?
That would shave a few bytes in every init function if we don't have to
return a value.

Cheers,
Ben

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


[RFC] Simplifying disable of Kconfig Debugging options

Saucedo Tejada, Genaro <genaro.saucedo.tejada@...>
 

Hello, please check this proposal and provide feedback if possible.
 
Background:
 
Currently several debugging options exists on Kconfig menu but they are not on a single debug submenu, e.g.:
- /Networking/Bluetooth support/Bluetooth debug support/*
- /Networking/Generic networking support/Network Ethernet driver debug
- /x86 architecture/Allows the usage of GDB with the ARC processor
- /Debugging Options/*
This means changing many debug settings at once becomes difficult and might led to developer overlooking some options.
 
Disabling all the debug features at once could be a frequent task when the footprint size is constantly kept in mind during development. If further debugging is required later, the process to re-enable several features will follow.
 
Consequently we want to make toggle of many debug settings at once as straight forward as possible.
 
Proposal to simplify the switching of multiple debug options at once:
 
1st approach: Original proposal suggested moving all options to a single menu, this will minimize the navigation between submenus.
 
This alternative was ruled out as the debug options taken out of original context won't let the developer know the details of the feature.

Duplicating options instead of removing them from the original context would complicate the menu and learning to use or to modify it.
 
2nd approach: Group features on debug levels and differentiate the debug features (both build and run time) from printing/logging features. First this alternative separates printing functionality from actual debugging features, printing would include current printk and printf related options plus possible more complex logging features to be developed in the future.
 
Regarding debugging features these could be classified in multiple levels instead of a single global switch, by reducing the debug level a developer could decrease the foot print in a simple manner while keeping logging/printing.

Several levels could be defined or additional categories, as an example the following menu specifies two debug levels (1st and 4th
options) and a single additional category (printing/logging):

[*] Build kernel with debugging
[*] Generate stack usage information
[ ] Enable printing
[ ] Enable run time debugging features
 
By disabling "Enable run time debugging features" the most complex features get disabled. Although not visible from '/Debugging Options' some network debug options would be disabled, some build time options and some low footprint impact options would remain available, logging category would also be available. The next level (1st option) disables all debugging options but those in drivers, as drivers usually have very diverse implementation, logging category would still be available.
 
Additional categories could be network and drivers but it cannot be guaranteed/enforced that new driver developers will follow such pattern on Kconfig files.

Additional debug levels could produce a finer grain classification regarding foot print and other drawbacks such as performance degradation.

It still remains an open issue turning on several options, as SELECT on Kconfig is problematic and maybe original intent was not to turn it all on but turn the previous selection on. Developer would have to back up his last debugging '.config' file to re-enable.

3rd approach: as an extreme alternative, in case modifying Kconfig menus is not viable, would be externally dealing with '.config' files.
using developer triggered scripts like merge_config.sh already part of make build process.

Developers would be able to specify debug presets using a new option in the per-application Makefile or passing a make command argument: (make ... DEBUG_PRESET='network2.conf kernel.conf').

Presets already on the repository could be used and developers could define their own.

Currently make script merges 3 configuration files: board, nanokernel XOR microkernel and project specific conf. Additional argument would be added to the merge command, these arguments being the developer selected debug presets.

An example preset with some variables for illustrative purpose:

$ cat network2.conf
#Level 2 network debugging preset
#prerequisite settings
NETWORKING_WITH_LOGGING=y
STDOUT_CONSOLE=y
NET_BUF_DEBUG=y
#debug settings
NETWORKING_STATISTICS=y
NETWORKING_DEBUG_UART=y
ETHERNET_DEBUG=y
TINYDTLS_DEBUG=y
NET_BUF_DEBUG=y
BLUETOOTH_DEBUG=y
BLUETOOTH_DEBUG_COLOR=y
BLUETOOTH_DEBUG_HCI_CORE=n
BLUETOOTH_DEBUG_CONN=y
BLUETOOTH_DEBUG_KEYS=n
BLUETOOTH_DEBUG_L2CAP=y
BLUETOOTH_SMP_SELFTEST=y
BLUETOOTH_DEBUG_ATT=n
BLUETOOTH_DEBUG_GATT=n


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Andre Guedes <andre.guedes@...>
 

Hi Daniel,

Quoting Kalowsky, Daniel (2016-02-12 12:53:49)
Early on when we started the driver development, the suspend/resume functions were developed with the DEV_OK because as far as any OSPM policy is concerned, there should be no reason these devices would stop it. This was an not so nice way into fooling that it has happened.
Thanks for the background explanation.

DEV_NO_SUPPORT seems to cover the concept. Not sure we need a DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.
Yes, DEV_NO_SUPPORT might cover the concept however it brings another
connotation which is "the hardware actually lacks support" (e.g. the
controller doesn't support that feature) and for that case I think
the error code DEV_INVALID_OP might be more suitable.

Anyways, if we move forward with DEV_NO_SUPPORT, I think we should fix the
comment in include/device.h since it looks a bit misleading. See below:

--[cut]--
#define DEV_NO_SUPPORT 6 /* Device type not supported */
--[cut]--

Thanks,

Andre


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Andre Guedes <andre.guedes@...>
 

Hi Peter,

Quoting Mitsis, Peter (2016-02-12 12:39:13)
[Peter] - Is there a reason why should not use the existing error code ENOSYS? From the description taken from http://www.gnu.org/software/libc/manual/html_node/Error-Codes.html (pasted below) I would think that it would fit our purposes.
I guess the main reason is that the device driver APIs follows the error code
format from include/device.h not the errno.h. Sure we can change all drivers
to use error code from errno.h, but I don't think this is desired ATM.

Thanks,

Andre


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Dirk Brandewie <dirk.j.brandewie@...>
 

On 02/12/2016 05:52 AM, Andre Guedes wrote:
Hi all,

It seems we don't have a consensus about what error code the device driver
should return in case a given driver API is not implemented. If we take a
look at suspend() and resume() functions, which are not implemented by some
device drivers, we can find three different return codes:
- DEV_OK: used in i2c_dw.c, i2c_atmel_sam3.c and gpio_sch.c
- DEV_INVALID_OP: used in pwm_dw.c, gpio_mmio.c and gpio_atmel_sam3.c
- DEV_NO_SUPPORT: used in i2c_qmsi.c and spi_qmsi.c
We definitely need to have one answer all drivers agree on.

I had an RFC where the suspend/resume functions move to the device
level and out of the device type specific APIs. This allows the PM
app/subsystem/entity to call the PM functions without needing to
know the type of the device.

I think I had agreement on the RFC but it fell by the wayside as we
were creeping up on the release date. I will get it rebased and
published again. Once we get agreement we can appoint all the
stuckies to update the drivers accordingly.

For these situations, a new error code (DEV_NOT_IMPLEMENTED) seems to be
applicable. If a new error code is not desired, we should agree on one
and use it in all device drivers.
In the new model the functions would be required to be implemented, the
RFC included a null implementation for drivers to use if they had no
work to do during suspend/resume. Still a topic for discussion but the
null functions can just return success meaning I have done everything
I need to or can do :-).

Regards,

Andre


Re: [RFC] Add DEV_NOT_IMPLEMENTED error code

Kalowsky, Daniel <daniel.kalowsky@...>
 

-----Original Message-----
From: Andre Guedes [mailto:andre.guedes(a)intel.com]
Sent: Friday, February 12, 2016 5:52 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] [RFC] Add DEV_NOT_IMPLEMENTED error code

Hi all,

It seems we don't have a consensus about what error code the device driver
should return in case a given driver API is not implemented. If we take a look
at suspend() and resume() functions, which are not implemented by some
device drivers, we can find three different return codes:
- DEV_OK: used in i2c_dw.c, i2c_atmel_sam3.c and gpio_sch.c
- DEV_INVALID_OP: used in pwm_dw.c, gpio_mmio.c and
gpio_atmel_sam3.c
- DEV_NO_SUPPORT: used in i2c_qmsi.c and spi_qmsi.c
Early on when we started the driver development, the suspend/resume functions were developed with the DEV_OK because as far as any OSPM policy is concerned, there should be no reason these devices would stop it. This was an not so nice way into fooling that it has happened.



For these situations, a new error code (DEV_NOT_IMPLEMENTED) seems to
be applicable. If a new error code is not desired, we should agree on one and
use it in all device drivers.
DEV_NO_SUPPORT seems to cover the concept. Not sure we need a DEV_NOT_IMPLEMENTED. Or as Peter points out ENOSYS works as well.



Regards,

Andre